From: Cosmin Ratiu <cratiu@nvidia.com>

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.

Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Reviewed-by: Carolina Jubran <cjubran@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 net/devlink/core.c          | 42 +++++++++++++++++++++++++++++++++++++
 net/devlink/devl_internal.h |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/net/devlink/core.c b/net/devlink/core.c
index eeb6a71f5f56..db11248df712 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -67,6 +67,48 @@ static void __devlink_rel_put(struct devlink_rel *rel)
 		devlink_rel_free(rel);
 }
 
+struct devlink *devlink_nested_in_get_lock(struct devlink_rel *rel)
+{
+	struct devlink *devlink;
+
+	if (!rel)
+		return NULL;
+	devlink = devlinks_xa_get(rel->nested_in.devlink_index);
+	if (!devlink)
+		return NULL;
+	devl_lock(devlink);
+	if (devl_is_registered(devlink))
+		return devlink;
+	devl_unlock(devlink);
+	devlink_put(devlink);
+	return NULL;
+}
+
+/* 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);
+	return devlink;
+}
+
+void devlink_nested_in_put_unlock(struct devlink_rel *rel)
+{
+	struct devlink *devlink = devlink_nested_in_get_locked(rel);
+
+	if (devlink) {
+		devl_unlock(devlink);
+		devlink_put(devlink);
+	}
+}
+
 static void devlink_rel_nested_in_notify_work(struct work_struct *work)
 {
 	struct devlink_rel *rel = container_of(work, struct devlink_rel,
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 7dfb7cdd2d23..3b4364677b18 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -136,6 +136,9 @@ typedef void devlink_rel_notify_cb_t(struct devlink *devlink, u32 obj_index);
 typedef void devlink_rel_cleanup_cb_t(struct devlink *devlink, u32 obj_index,
 				      u32 rel_index);
 
+struct devlink *devlink_nested_in_get_lock(struct devlink_rel *rel);
+struct devlink *devlink_nested_in_get_locked(struct devlink_rel *rel);
+void devlink_nested_in_put_unlock(struct devlink_rel *rel);
 void devlink_rel_nested_in_clear(u32 rel_index);
 int devlink_rel_nested_in_add(u32 *rel_index, u32 devlink_index,
 			      u32 obj_index, devlink_rel_notify_cb_t *notify_cb,
-- 
2.44.0

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

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

On 3/31/2026 5:20 AM, Cosmin Ratiu wrote:
> 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.
> 

Hmm. I'm struggling to follow this. If you already expect the parent to
hold the nested devlink's lock, it must have a pointer to this devlink
instance. In that case, why would you even need
devlink_nested_in_get_locked in the first place?

> 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.
> 

You should at least do that. It is the correct pattern when dealing with
reference counting data structures from the xarray. That's already how
devlinks_xa_get() is implemented.

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

If the caller already held the lock, why is devlink_nested_in_put_unlock
calling the devl_unlock instead of the caller anyways? That seems
confusing. Wouldn't the normal pattern be to
devlink_nested_in_get_lock()? Oh, that is a separate function. Ok I see.

>> 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.
> 

IMO it would make more sense for the put version to be a put on the
returned devlink pointer. I guess its not symmetrical, but it removes
the need to perform the second lookup and it makes it easier to reason
about the pointer you're releasing being the same one.

Having put take different arguments from get is the usual pattern for
such a behavior.

Also devlink_nested_in_get_locked() doesn't increase the ref count so it
is sort of "relying" on the caller already having a reference to it,
which makes me think its not very useful. The only valid way to call
this function as it exists now safely is to already hold a reference to
the object, which also already requires you to have a valid pointer
making me wonder why you'd ever need to call it in the first place.

The only example you have is to make devlink_nested_in_put_unlock() take
a devlink_rel pointer as its argument instead of just calling it on the
pointer returned by devlink_nested_in_get_lock().

This implementation seems confusing and likely to lead to errors.

Thanks,
Jake

On Tue, 2026-03-31 at 16:55 -0700, Jacob Keller wrote:
> On 3/31/2026 5:20 AM, Cosmin Ratiu wrote:
> > 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.
> > 
> 
> Hmm. I'm struggling to follow this. If you already expect the parent
> to
> hold the nested devlink's lock, it must have a pointer to this
> devlink
> instance. In that case, why would you even need
> devlink_nested_in_get_locked in the first place?

After some more intense staring, I realized that intermediate instances
don't actually need to be locked, only the ancestor needs to. With that
in mind, the code get simplified:
- devlink_nested_in_get_locked and devlink_nested_in_put_unlock can be
removed.
- recursive unlocking in devl_rate_unlock is gone.

> 
> > 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.
> > 
> 
> You should at least do that. It is the correct pattern when dealing
> with
> reference counting data structures from the xarray. That's already
> how
> devlinks_xa_get() is implemented.
> 
> > > 
> > > > +	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.
> > > 
> 
> If the caller already held the lock, why is
> devlink_nested_in_put_unlock
> calling the devl_unlock instead of the caller anyways? That seems
> confusing. Wouldn't the normal pattern be to
> devlink_nested_in_get_lock()? Oh, that is a separate function. Ok I
> see.
> 
> > > 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.
> > 
> 
> IMO it would make more sense for the put version to be a put on the
> returned devlink pointer. I guess its not symmetrical, but it removes
> the need to perform the second lookup and it makes it easier to
> reason
> about the pointer you're releasing being the same one.
> 
> Having put take different arguments from get is the usual pattern for
> such a behavior.
> 
> Also devlink_nested_in_get_locked() doesn't increase the ref count so
> it
> is sort of "relying" on the caller already having a reference to it,
> which makes me think its not very useful. The only valid way to call
> this function as it exists now safely is to already hold a reference
> to
> the object, which also already requires you to have a valid pointer
> making me wonder why you'd ever need to call it in the first place.
> 
> The only example you have is to make devlink_nested_in_put_unlock()
> take
> a devlink_rel pointer as its argument instead of just calling it on
> the
> pointer returned by devlink_nested_in_get_lock().
> 
> This implementation seems confusing and likely to lead to errors.

I hope the next version will be more suitable.
Thank you for the comments and suggestions.

> 
> Thanks,
> Jake

On 4/1/2026 3:22 AM, Cosmin Ratiu wrote:
> On Tue, 2026-03-31 at 16:55 -0700, Jacob Keller wrote:
>> On 3/31/2026 5:20 AM, Cosmin Ratiu wrote:
>>> 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.
>>>
>>
>> Hmm. I'm struggling to follow this. If you already expect the parent
>> to
>> hold the nested devlink's lock, it must have a pointer to this
>> devlink
>> instance. In that case, why would you even need
>> devlink_nested_in_get_locked in the first place?
> 
> After some more intense staring, I realized that intermediate instances
> don't actually need to be locked, only the ancestor needs to. With that
> in mind, the code get simplified:
> - devlink_nested_in_get_locked and devlink_nested_in_put_unlock can be
> removed.
> - recursive unlocking in devl_rate_unlock is gone.
> 
That seems like it would be better, as long as we can prove correctness
of every access. Thanks!

-Jake