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

[Cosmin Ratiu <cratiu@nvidia.com>]

On Mon, 2026-03-30 at 19:08 -0700, Jakub Kicinski wrote:
> 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?

This function is meant to be invoked by callers already holding all
locks from a given instance up the nesting tree. Calling it outside
this context could indeed lead to a race as described, where another
entity unregisters a devlink about-to-be-asserted on.

All current callers hold the locks, but to make it more robust, I will
add the rcu_read_lock/unlock section around the find+assert.

> 
> > +	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?

2 issues here:
1) Mistakenly calling this without having acquired the lock. This is
akin to saying mutex_unlock is dangerous if the lock isn't held.
Technically true, but moot.
2) The rel argument: It is intentional, so that all 3 functions are
symmetrical.

> 
> > +
> > +	if (devlink) {
> > +		devl_unlock(devlink);
> > +		devlink_put(devlink);
> > +	}
> > +}