Re: [Intel-wired-lan] [PATCH iwl-net] idpf: fix adapter NULL pointer dereference on reboot

["Tantilov, Emil S" <emil.s.tantilov@intel.com>]

On 3/9/2025 11:22 PM, Simon Horman wrote:
> On Thu, Mar 06, 2025 at 04:39:56PM -0800, Emil Tantilov wrote:
>> Driver calls idpf_remove() from idpf_shutdown(), which can end up
>> calling idpf_remove() again when disabling SRIOV.
>>
>> echo 1 > /sys/class/net/<netif>/device/sriov_numvfs
>> reboot
>>
>> BUG: kernel NULL pointer dereference, address: 0000000000000020
>> ...
>> RIP: 0010:idpf_remove+0x22/0x1f0 [idpf]
>> ...
>> ? idpf_remove+0x22/0x1f0 [idpf]
>> ? idpf_remove+0x1e4/0x1f0 [idpf]
>> pci_device_remove+0x3f/0xb0
>> device_release_driver_internal+0x19f/0x200
>> pci_stop_bus_device+0x6d/0x90
>> pci_stop_and_remove_bus_device+0x12/0x20
>> pci_iov_remove_virtfn+0xbe/0x120
>> sriov_disable+0x34/0xe0
>> idpf_sriov_configure+0x58/0x140 [idpf]
>> idpf_remove+0x1b9/0x1f0 [idpf]
>> idpf_shutdown+0x12/0x30 [idpf]
>> pci_device_shutdown+0x35/0x60
>> device_shutdown+0x156/0x200
>> ...
>>
>> Replace the direct idpf_remove() call in idpf_shutdown() with
>> idpf_vc_core_deinit() and idpf_deinit_dflt_mbx(), which perform
>> the bulk of the cleanup, such as stopping the init task, freeing IRQs,
>> destroying the vports and freeing the mailbox.
> 
> Hi Emil,
> 
> I think it would be worth adding some commentary on the rest of
> the clean-up performed by idpf_remove() and why it is correct
The main reason behind the change is to avoid calling sriov_disable(), 
which ends up calling idpf_remove() again via pci_device_remove(). The 
idpf_remove() will crash in that situation as it attempts to access 
adapter pointer, which was already freed.

> to no longer do so directly from a call to idpf_remove() from
> idpf_shutdown() (IOW, it isn't clear to me :).
I assume you are asking what portion of the idpf_remove() will not be 
present in idpf_shutdown() as result? Aside from not calling 
sriov_disable(), there is a small cleanup of stale netdevs and the 
destruction of WQs, which did not seem like would be needed on shutdown. 
Then again, I was not able to find documentation on what steps are 
required for shutdown and mostly checked on how other drivers handle it 
(where there is no 1:1 overlap between shutdown and remove), and applied 
similar steps to idpf. Ideally I do not wish to do more than is needed 
for that flow.

> 
> ...