Re: [PATCH] net: hinic3: unregister event callback on remove

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

- [Medium] The commit message asserts a live use-after-free ("If an event
  is delivered after the NIC private data has been released, the…

Pre-existing issues:
- [Medium] hinic3_del_one_adev()
  (drivers/net/ethernet/huawei/hinic3/hinic3_lld.c:94-113) implements…

--

net: hinic3: unregister event callback on remove

This adds a hinic3_adev_event_unregister() call at the start of
hinic3_nic_remove() so the auxiliary device event callback is cleared before
the NIC device is torn down, matching what the probe failure paths already
do.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_main.c b/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
> index 0a888fe4c975b..42e2ad179a9f5 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_main.c
> @@ -499,6 +499,8 @@ static void hinic3_nic_remove(struct auxiliary_device *adev)
>  	if (!hinic3_support_nic(nic_dev->hwdev))
>  		return;
>  
> +	hinic3_adev_event_unregister(adev);
> +

[Severity: Medium]
Is there an event delivery path in this tree that can actually reach
hinic3_nic_event()?

The commit message says:

    If an event is delivered after the NIC private data has been released,
    the callback may dereference the freed nic_dev and netdev objects.

Grepping drivers/net/ethernet/huawei/hinic3/ shows the only accesses to
struct hinic3_adev::event are the two assignments:

  hinic3_lld.c:hinic3_adev_event_register() {
	hadev = container_of(adev, struct hinic3_adev, adev);
	hadev->event = event_handler;
  }

  hinic3_lld.c:hinic3_adev_event_unregister() {
	hadev = container_of(adev, struct hinic3_adev, adev);
	hadev->event = NULL;
  }

Nothing loads and calls hadev->event.  The hardware event paths that do
exist dispatch through aeqs->aeq_cb[] / ceqs->ceq_cb[] in hinic3_eqs.c, and
hinic3_nic_event() is referenced only as the argument to the
hinic3_adev_event_register() call in hinic3_nic_probe().

If that is correct, then this change restores register/unregister symmetry
but does not close a reachable failure.  Should the Fixes: tag for
17fcb3dc12bb and the Cc: stable@vger.kernel.org be dropped, and the message
reworded as a symmetry or cleanup change?  Alternatively, could the concrete
dispatch path that makes the dereference reachable be named in the message?

One further note for whenever a dispatcher is added: clearing a plain
function pointer gives no barrier against a call that is already in flight,
so the symmetry alone would not be sufficient there either.

[Severity: Medium]
This isn't a bug introduced by this patch, but the event-versus-teardown
exclusion that this ordering change leans on is a hand-rolled bit lock in
hinic3_del_one_adev():

  hinic3_lld.c:hinic3_del_one_adev() {
	timeout = read_poll_timeout(test_and_set_bit, state, !state,
				    HINIC3_EVENT_POLL_SLEEP_US,
				    HINIC3_EVENT_POLL_TIMEOUT_US,
				    false, svc_type, &pci_adapter->state);

	hadev = pci_adapter->hadev[svc_type];
	auxiliary_device_delete(&hadev->adev);
	auxiliary_device_uninit(&hadev->adev);
	pci_adapter->hadev[svc_type] = NULL;
	if (!timeout)
		clear_bit(svc_type, &pci_adapter->state);
  }

The bit svc_type of pci_adapter->state is set on entry and cleared on exit
to guard a code region, which is invisible to lockdep and provides no
ordering or fairness guarantees.

Does this loop also fail open?  On -ETIMEDOUT the bit was never acquired,
yet auxiliary_device_delete() / auxiliary_device_uninit() run anyway, which
drives hinic3_nic_remove() and free_netdev() while, by the code's own
assumption, another context still owns the bit.

On that same timeout path clear_bit() is skipped, so the bit stays set for
that service type.  Would every later add or delete of that service then
burn the full HINIC3_EVENT_POLL_TIMEOUT_US poll and fail open again?

Since the callers already hold pci_adapter->pdev_mutex, could that mutex
plus a synchronous flush or cancel of the event source replace the bit
entirely?

Today the only accesses to pci_adapter->state are the test_and_set_bit and
clear_bit above, so the first test_and_set_bit always observes the bit clear
and the timeout branch is not reachable, but that changes as soon as event
dispatch is added.

>  	netdev = nic_dev->netdev;
>  	unregister_netdev(netdev);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916032427.2899987-1-lgs201920130244%40gmail.com