On Mon, Feb 17, 2025 at 09:18:42AM +0000, Wei Yang wrote:
> 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?
>
Yeah, I think it make more sense with e returned, and you can add some
comments at the return statement like:
return e; // as long as the lock of e is held, e is valid.
, but feel free to use whatever you see fit.
Regards,
Boqun
> >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
