On Mon, Feb 17, 2025 at 04:02:53PM +0800, Alan Huang wrote:
>On Feb 17, 2025, at 15:41, Wei Yang <richard.weiy...@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.f...@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.weiy...@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.weiy...@gmail.com>
>>>> Link:
>>>> https://lore.kernel.org/r/20250101082306.10404-1-richard.weiy...@gmail.com
>>>> Signed-off-by: Boqun Feng <boqun.f...@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
