* [PATCH] doc/RCU/listRCU: fix an example code snippets @ 2025-01-01 8:23 Wei Yang 2025-01-23 1:48 ` Wei Yang 2025-02-17 2:12 ` Boqun Feng 0 siblings, 2 replies; 10+ messages in thread From: Wei Yang @ 2025-01-01 8:23 UTC (permalink / raw) To: paulmck, frederic, neeraj.upadhyay; +Cc: rcu, linux-doc, Wei Yang 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> --- Hope my understanding is correct. --- 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); + rcu_read_unlock(); return state; } } -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-01-01 8:23 [PATCH] doc/RCU/listRCU: fix an example code snippets Wei Yang @ 2025-01-23 1:48 ` Wei Yang 2025-02-17 2:12 ` Boqun Feng 1 sibling, 0 replies; 10+ messages in thread From: Wei Yang @ 2025-01-23 1:48 UTC (permalink / raw) To: Wei Yang; +Cc: paulmck, frederic, neeraj.upadhyay, rcu, linux-doc Ping 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> > >--- >Hope my understanding is correct. >--- > 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); >+ rcu_read_unlock(); > return state; > } > } >-- >2.34.1 -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-01-01 8:23 [PATCH] doc/RCU/listRCU: fix an example code snippets Wei Yang 2025-01-23 1:48 ` Wei Yang @ 2025-02-17 2:12 ` Boqun Feng 2025-02-17 2:22 ` Alan Huang 1 sibling, 1 reply; 10+ messages in thread From: Boqun Feng @ 2025-02-17 2:12 UTC (permalink / raw) To: Wei Yang; +Cc: paulmck, frederic, neeraj.upadhyay, rcu, linux-doc 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); + rcu_read_unlock(); return state; } } -- ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-02-17 2:12 ` Boqun Feng @ 2025-02-17 2:22 ` Alan Huang 2025-02-17 2:35 ` Boqun Feng 2025-02-17 7:41 ` Wei Yang 0 siblings, 2 replies; 10+ messages in thread From: Alan Huang @ 2025-02-17 2:22 UTC (permalink / raw) To: Boqun Feng; +Cc: Wei Yang, paulmck, frederic, neeraj.upadhyay, rcu, linux-doc 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. > + rcu_read_unlock(); > return state; > } > } > -- > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-02-17 2:22 ` Alan Huang @ 2025-02-17 2:35 ` Boqun Feng 2025-02-17 7:41 ` Wei Yang 1 sibling, 0 replies; 10+ messages in thread From: Boqun Feng @ 2025-02-17 2:35 UTC (permalink / raw) To: Alan Huang; +Cc: Wei Yang, paulmck, frederic, neeraj.upadhyay, rcu, linux-doc 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. > Good catch! I think we then don't need this patch? Wei, are you able to confirm Alan's reasoning here? Regards, Boqun > > + rcu_read_unlock(); > > return state; > > } > > } > > -- > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-02-17 2:22 ` Alan Huang 2025-02-17 2:35 ` Boqun Feng @ 2025-02-17 7:41 ` Wei Yang 2025-02-17 8:02 ` Alan Huang 1 sibling, 1 reply; 10+ messages in thread From: Wei Yang @ 2025-02-17 7:41 UTC (permalink / raw) To: Alan Huang Cc: Boqun Feng, Wei Yang, paulmck, frederic, neeraj.upadhyay, rcu, linux-doc 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. 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. 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-02-17 7:41 ` Wei Yang @ 2025-02-17 8:02 ` Alan Huang 2025-02-17 9:18 ` Wei Yang 0 siblings, 1 reply; 10+ messages in thread From: Alan Huang @ 2025-02-17 8:02 UTC (permalink / raw) To: Wei Yang; +Cc: Boqun Feng, paulmck, frederic, neeraj.upadhyay, rcu, linux-doc 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. > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-02-17 8:02 ` Alan Huang @ 2025-02-17 9:18 ` Wei Yang 2025-02-17 22:30 ` Boqun Feng 0 siblings, 1 reply; 10+ messages in thread From: Wei Yang @ 2025-02-17 9:18 UTC (permalink / raw) To: Alan Huang Cc: Wei Yang, Boqun Feng, paulmck, frederic, neeraj.upadhyay, rcu, linux-doc 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-02-17 9:18 ` Wei Yang @ 2025-02-17 22:30 ` Boqun Feng 2025-02-18 0:25 ` Wei Yang 0 siblings, 1 reply; 10+ messages in thread From: Boqun Feng @ 2025-02-17 22:30 UTC (permalink / raw) To: Wei Yang; +Cc: Alan Huang, paulmck, frederic, neeraj.upadhyay, rcu, linux-doc 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.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? > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] doc/RCU/listRCU: fix an example code snippets 2025-02-17 22:30 ` Boqun Feng @ 2025-02-18 0:25 ` Wei Yang 0 siblings, 0 replies; 10+ messages in thread From: Wei Yang @ 2025-02-18 0:25 UTC (permalink / raw) To: Boqun Feng Cc: Wei Yang, Alan Huang, paulmck, frederic, neeraj.upadhyay, rcu, linux-doc On Mon, Feb 17, 2025 at 02:30:59PM -0800, Boqun Feng wrote: >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.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? >> > >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. > Thanks, let me prepare one. >Regards, >Boqun -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-02-18 0:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-01 8:23 [PATCH] doc/RCU/listRCU: fix an example code snippets Wei Yang 2025-01-23 1:48 ` Wei Yang 2025-02-17 2:12 ` Boqun Feng 2025-02-17 2:22 ` Alan Huang 2025-02-17 2:35 ` Boqun Feng 2025-02-17 7:41 ` Wei Yang 2025-02-17 8:02 ` Alan Huang 2025-02-17 9:18 ` Wei Yang 2025-02-17 22:30 ` Boqun Feng 2025-02-18 0:25 ` Wei Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox