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

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

[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

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

[Wei Yang]

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

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

[Boqun Feng]

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;
 			}
 		}
-- 

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

[Alan Huang]

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;
> }
> }
> -- 
> 

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

[Boqun Feng]

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;
> > }
> > }
> > -- 
> > 
> 

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

[Wei Yang]

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

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

[Alan Huang]

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

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

[Wei Yang]

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

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

[Boqun Feng]

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

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

[Wei Yang]

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