On Feb 17, 2025, at 15:41, Wei Yang <[email protected]> wrote:
>
> On Mon, Feb 17, 2025 at 10:22:53AM +0800, Alan Huang wrote:
>> On Feb 17, 2025, at 10:12, Boqun Feng <[email protected]> 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 <[email protected]>
>>> [...]
>>>
>>>
>>> ------------------>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 <[email protected]>
>>> Link:
>>> https://lore.kernel.org/r/[email protected]
>>> Signed-off-by: Boqun Feng <[email protected]>
>>> ---
>>> 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.
>
> 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)
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
>
> 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
