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);
> > + }
> > +}
