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

[Wei Yang <richard.weiyang@gmail.com>]

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