Re: [PATCH net 3/4] idpf: handle NULL adev in idpf_idc_vdev_mtu_event

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
idpf: handle NULL adev in idpf_idc_vdev_mtu_event

This change caches vdev_info->adev with READ_ONCE() and returns early
when the cached pointer is NULL, so that device_lock()/device_unlock()
operate on the same captured auxiliary_device and do not dereference a
NULL adev field that was cleared by the teardown path.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_idc.c b/drivers/net/ethernet/intel/idpf/idpf_idc.c
> index b7d6b08fc89e..9f764135507c 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_idc.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_idc.c
> @@ -162,9 +162,12 @@ void idpf_idc_vdev_mtu_event(struct iidc_rdma_vport_dev_info *vdev_info,
>  
>  	set_bit(event_type, event.type);
>  
> -	device_lock(&vdev_info->adev->dev);
> -	adev = vdev_info->adev;
> -	if (!adev || !adev->dev.driver)
> +	adev = READ_ONCE(vdev_info->adev);
> +	if (!adev)
> +		return;
> +
> +	device_lock(&adev->dev);

[High]
Does caching the pointer with READ_ONCE() actually make this safe against
the teardown path, or does it just narrow a NULL-deref into a
use-after-free on the mutex embedded in adev->dev?

Looking at the teardown side, idpf_idc_vport_dev_ctrl(adapter, false)
calls idpf_idc_vport_dev_down(), which calls idpf_unplug_aux_dev() on
vdev_info->adev:

    idpf_unplug_aux_dev(adev)
        auxiliary_device_delete(adev);
        auxiliary_device_uninit(adev);   /* put_device -> release -> kfree */

and only afterwards assigns vport->vdev_info->adev = NULL.

The MTU path runs under rtnl_lock and the vport_ctrl_lock, but
ig3rdma_core_remove() (which drives the IDC teardown via the auxiliary
bus) does not appear to take either of those, so the two paths are not
serialized against each other.

With the new code:

    adev = READ_ONCE(vdev_info->adev);
    if (!adev)
        return;
    device_lock(&adev->dev);

if the teardown path frees adev between the READ_ONCE() load and the
device_lock() call (it can, because the kfree happens before vdev_info->adev
is set to NULL, and there is no get_device()/put_device() or RCU barrier
on this path), then device_lock() operates on a freed struct device and
its embedded mutex.

Would the correct fix here pair the load with get_device()/put_device()
across the locked region, NULL out vdev_info->adev before the
auxiliary_device_uninit() in the teardown path, and add a
synchronize_rcu()-style barrier (or a shared lock) so the MTU path cannot
observe a freed adev?

>  iadrv = container_of(adev->dev.driver,
>  			     struct iidc_rdma_vport_auxiliary_drv,
> @@ -172,7 +175,7 @@ void idpf_idc_vdev_mtu_event(struct iidc_rdma_vport_dev_info *vdev_info,
>  	if (iadrv->event_handler)
>  		iadrv->event_handler(vdev_info, &event);
>  unlock:
> -	device_unlock(&vdev_info->adev->dev);
> +	device_unlock(&adev->dev);
>  }
-- 
This is an AI-generated review.