Re: [PATCH net-next V9 02/14] devlink: Add helpers to lock nested-in instances

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
devlink: Add helpers to lock nested-in instances

Upcoming code will need to obtain a reference to locked nested-in
devlink instances. Add helpers to lock, obtain an already locked
reference and unlock/unref the nested-in instance.

> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index eeb6a71f5f56e..db11248df7120 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c

[ ... ]

> +/* Returns the nested in devlink object and validates its lock is held. */
> +struct devlink *devlink_nested_in_get_locked(struct devlink_rel *rel)
> +{
> +	struct devlink *devlink;
> +	unsigned long index;
> +
> +	if (!rel)
> +		return NULL;
> +	index = rel->nested_in.devlink_index;
> +	devlink = xa_find(&devlinks, &index, index, DEVLINK_REGISTERED);
> +	if (devlink)
> +		devl_assert_locked(devlink);

Can this dereference a freed pointer if the lock is not actually held?

xa_find() acquires and releases the RCU read lock internally. When it returns,
the devlink pointer is no longer protected by RCU or a reference count.

If a caller invokes this helper without holding the devlink lock (which is the
exact scenario the assertion is designed to detect), another thread could
concurrently unregister and free the devlink.

If the RCU grace period expires before the assertion executes,
devl_assert_locked() would dereference freed memory. Should the lookup and
assertion be enclosed within an explicit rcu_read_lock() block to safely
assert the lock?

> +	return devlink;
> +}
> +
> +void devlink_nested_in_put_unlock(struct devlink_rel *rel)
> +{
> +	struct devlink *devlink = devlink_nested_in_get_locked(rel);

Could this corrupt the mutex state and cause a refcount underflow?

This helper takes a devlink_rel pointer instead of the devlink pointer
acquired by devlink_nested_in_get_lock(), and performs a secondary global
lookup to find the devlink.

If a caller mistakenly calls this in an error cleanup path where they did not
actually acquire the lock, the global xa_find() will still locate the
registered devlink. This would execute devl_unlock() and devlink_put() on a
devlink the current thread does not own.

Would it be safer for unlock/put helpers to take the exact pointer returned by
the lock/get helper to ensure safe resource cleanup?

> +
> +	if (devlink) {
> +		devl_unlock(devlink);
> +		devlink_put(devlink);
> +	}
> +}
-- 
pw-bot: cr