Re: [PATCH rdma v7 7/7] RDMA/zrdma: Implement device and port query operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/zrdma: broken conditional RTNL locking in zxdh_get_drvinfo()
--

commit f5a32e13f5abe916870114665b95d03769a17e3b
Author: Yanze Zhang <zhang.yanze@zte.com.cn>

RDMA/zrdma: Implement device and port query operations

This commit implements RDMA device and port query callbacks, including device
attributes, port attributes, PKey, link layer, port immutable data, and
firmware version string extraction for the zrdma driver.

> diff --git a/drivers/infiniband/hw/zrdma/zrdma_verbs.c b/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> index abac97e694138..ceaad155fc5ae 100644
> --- a/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> +++ b/drivers/infiniband/hw/zrdma/zrdma_verbs.c

[ ... ]

> @@ -352,6 +354,252 @@ static int zxdh_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
>  	return ret;
>  }
>  
> +/* Fetch the driver info for @zdev->netdev, or for its first lower device
> + * when it is a bond. The lower device list is protected by rtnl_lock, and
> + * the core may call this from a netdev notifier that already holds the
> + * lock, so it is only taken when it is not held already.
> + */
> +static bool zxdh_get_drvinfo(struct zxdh_device *zdev,
> +			     struct ethtool_drvinfo *info)
> +{
> +	struct net_device *slave = NULL, *dev;
> +	struct list_head *iter;
> +	bool locked;
> +	bool ret = true;
> +
> +	memset(info, 0, sizeof(*info));
> +
> +	locked = !rtnl_is_locked();
> +	if (locked)
> +		rtnl_lock();
> +
> +	if (zdev->netdev->priv_flags & IFF_BONDING)
> +		netdev_for_each_lower_dev(zdev->netdev, slave, iter)
> +			break;
> +
> +	dev = slave ? slave : zdev->netdev;
> +	if (dev->ethtool_ops && dev->ethtool_ops->get_drvinfo)
> +		dev->ethtool_ops->get_drvinfo(dev, info);
> +	else
> +		ret = false;
> +
> +	if (locked)
> +		rtnl_unlock();

[Severity: High]
Can this conditional locking in zxdh_get_drvinfo() cause an unlocked
traversal of zdev->netdev->adj_list.lower and a use-after-free on slave when
another task holds rtnl_mutex?

In net/core/rtnetlink.c, rtnl_is_locked() checks whether any task on any cpu
currently holds rtnl_mutex, rather than whether current owns it:

int rtnl_is_locked(void)
{
	return mutex_is_locked(&rtnl_mutex);
}

When zxdh_get_drvinfo() is called through zxdh_query_device() or
zxdh_get_dev_fw_str() while another task on another cpu holds rtnl_mutex
(for example, while unlinking a bonding lower device via
netdev_upper_dev_unlink() or unregistering a slave net_device),
rtnl_is_locked() returns true.

As a result, zxdh_get_drvinfo() skips rtnl_lock() and calls
netdev_for_each_lower_dev() -> netdev_lower_get_next() and
dev->ethtool_ops->get_drvinfo(dev, info) without holding rtnl_mutex, racing
directly with the task modifying the lower device list or tearing down the
slave device.

Since zxdh_get_drvinfo() is only called from process context through
zxdh_query_device() and zxdh_get_dev_fw_str(), neither of which holds
rtnl_mutex, should zxdh_get_drvinfo() unconditionally call rtnl_lock() and
rtnl_unlock()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027021119.2325798-1-zhang.yanze@zte.com.cn?part=7