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
