On 4/2/19 12:25 AM, Saeed Mahameed wrote: > On Mon, 2019-04-01 at 20:24 +0800, Firo Yang wrote: >> This crash is triggered by a user-after-free since lake of >> the synchronization of a race condition between >> be_update_queues() modifying multi-purpose channels of >> network device and be_tx_timeout(). >> >> BUG: unable to handle kernel NULL pointer dereference at (null) >> Call Trace: >> be_tx_timeout+0xa5/0x360 [be2net] >> dev_watchdog+0x1d8/0x210 >> call_timer_fn+0x32/0x140 >> >> To fix it, detach the interface before modifying >> multi-purpose channels of network device. >> >> Signed-off-by: Firo Yang <fy...@suse.com> >> --- >> drivers/net/ethernet/emulex/benet/be_main.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c >> b/drivers/net/ethernet/emulex/benet/be_main.c >> index d5026909dec5..25d0128bf684 100644 >> --- a/drivers/net/ethernet/emulex/benet/be_main.c >> +++ b/drivers/net/ethernet/emulex/benet/be_main.c >> @@ -4705,6 +4705,8 @@ int be_update_queues(struct be_adapter >> *adapter) >> struct net_device *netdev = adapter->netdev; >> int status; >> >> + netif_device_detach(netdev); >> + > > This will reduce the probability, but will not do the trick. > since this will not guarantee that the dev_watchdog is disabled. Great catch!
>
> what you need is proper locking mechanism and/or scheduling the
> tx_timeout handling out of atomic context if a mutex will be required.
Thank you; I will modify this patch.
Best,
Firo
>
> netif_device_detach is a too heavy hammer for such synchronizations
> tasks.
>
>> if (netif_running(netdev))
>> be_close(netdev);
>>
>> @@ -4719,21 +4721,21 @@ int be_update_queues(struct be_adapter
>> *adapter)
>> be_clear_queues(adapter);
>> status = be_cmd_if_destroy(adapter, adapter->if_handle, 0);
>> if (status)
>> - return status;
>> + goto out;
>>
>> if (!msix_enabled(adapter)) {
>> status = be_msix_enable(adapter);
>> if (status)
>> - return status;
>> + goto out;
>> }
>>
>> status = be_if_create(adapter);
>> if (status)
>> - return status;
>> + goto out;
>>
>> status = be_setup_queues(adapter);
>> if (status)
>> - return status;
>> + goto out;
>>
>> be_schedule_worker(adapter);
>>
>> @@ -4748,6 +4750,8 @@ int be_update_queues(struct be_adapter
>> *adapter)
>> if (netif_running(netdev))
>> status = be_open(netdev);
>>
>> +out:
>> + netif_device_attach(netdev);
>> return status;
>> }
>>
pEpkey.asc
Description: application/pgp-keys
