Re: [PATCH] doc/RCU/listRCU: fix an example code snippets

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Feb 17, 2025 at 04:02:53PM +0800, Alan Huang wrote:
>On Feb 17, 2025, at 15:41, Wei Yang <richard.weiyang@gmail.com> wrote:
>> 
>> On Mon, Feb 17, 2025 at 10:22:53AM +0800, Alan Huang wrote:
>>> On Feb 17, 2025, at 10:12, Boqun Feng <boqun.feng@gmail.com> wrote:
>>>> 
>>>> Hi Wei,
>>>> 
>>>> The change loosk good to me, thanks!
>>>> 
>>>> I queued the patch for futher reviews and tests with some changes in the
>>>> commit log (for title formating and a bit more explanation), please see
>>>> below.
>>>> 
>>>> Regards,
>>>> Boqun
>>>> 
>>>> On Wed, Jan 01, 2025 at 08:23:06AM +0000, Wei Yang wrote:
>>>>> The example code for "Eliminating Stale Data" looks not correct:
>>>>> 
>>>>> * rcu_read_unlock() should put after kstrdup()
>>>>> * spin_unlock() should be called before return
>>>>> 
>>>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>>> [...]
>>>> 
>>>> 
>>>> ------------------>8
>>>> Subject: [PATCH] doc/RCU/listRCU: Fix an example code snippets
>>>> 
>>>> The example code for "Eliminating Stale Data" looks not correct:
>>>> 
>>>> * rcu_read_unlock() should put after kstrdup(), because otherwise
>>>> entry may get freed while kstrdup() is being called.
>>>> 
>>>> * spin_unlock() should be called before return, otherwise the
>>>> function would return with the lock of the entry held.
>>>> 
>>>> Hence fix these.
>>>> 
>>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>>> Link: https://lore.kernel.org/r/20250101082306.10404-1-richard.weiyang@gmail.com
>>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>>>> ---
>>>> Documentation/RCU/listRCU.rst | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/Documentation/RCU/listRCU.rst b/Documentation/RCU/listRCU.rst
>>>> index ed5c9d8c9afe..8df50fcd69fd 100644
>>>> --- a/Documentation/RCU/listRCU.rst
>>>> +++ b/Documentation/RCU/listRCU.rst
>>>> @@ -348,9 +348,10 @@ to accomplish this would be to add a ``deleted`` flag and a ``lock`` spinlock to
>>>> rcu_read_unlock();
>>>> return AUDIT_BUILD_CONTEXT;
>>>> }
>>>> - rcu_read_unlock();
>>>> if (state == AUDIT_STATE_RECORD)
>>>> *key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
>>>> + spin_unlock(&e->lock);
>>> 
>>> According to the above quick quiz, we should return with the lock held.
>>> 
>> 
>> Thanks, I think you have some reason.
>> 
>> If my understanding is correct, the example here is to emphasize we could
>> still access the value out of critical section but with spinlock held.
>
>This example is intended to highlight how we can eliminate stale data.
>

Yes, you are more accurate.

>> 
>> In current example, we don't return e(struct audit_entry) from
>> audit_filter_task(). So no one suppose to release the spinlock again. This
>> looks to be a mistake.
>
>Then the example code should return e instead. ( *key is also undefined)
>

So you prefer a version with e returned?

Boqun

What's your preference?

>If you have some time, I’d recommend [1]
>
>[1] Using Read-Copy-Update Techniques for System V IPC in the Linux 2.5
>Kernel
>

Thanks, would take a look.

>> 
>> My suggestion is to release the lock after kstrdup() to make the example more
>> intact. But with a comment to explain the purpose here.
>> 
>> Also I found we miss the second parameter key here.
>> 
>> diff --git a/Documentation/RCU/listRCU.rst b/Documentation/RCU/listRCU.rst
>> index ed5c9d8c9afe..a3e7f8ff3a81 100644
>> --- a/Documentation/RCU/listRCU.rst
>> +++ b/Documentation/RCU/listRCU.rst
>> @@ -334,7 +334,7 @@ If the system-call audit module were to ever need to reject stale data, one way
>> to accomplish this would be to add a ``deleted`` flag and a ``lock`` spinlock to the
>> ``audit_entry`` structure, and modify audit_filter_task() as follows::
>> 
>> - static enum audit_state audit_filter_task(struct task_struct *tsk)
>> + static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
>> {
>> struct audit_entry *e;
>> enum audit_state   state;
>> @@ -349,8 +349,11 @@ to accomplish this would be to add a ``deleted`` flag and a ``lock`` spinlock to
>> return AUDIT_BUILD_CONTEXT;
>> }
>> rcu_read_unlock();
>> + /* With spinlock held, it is ok to access 'e' out
>> +                                 * of critial section */
>> if (state == AUDIT_STATE_RECORD)
>> *key = kstrdup(e->rule.filterkey, GFP_ATOMIC);
>> + spin_unlock(&e->lock);
>> return state;
>> }
>> }
>> 
>> Does it make sense to you?
>> 
>> 
>> -- 
>> Wei Yang
>> Help you, Help me
>

-- 
Wei Yang
Help you, Help me