linux-kernel - Re: [PATCH] rculist : Introduce list/hlist_for_each_entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <D24A0BBD-9A1A-4480-B36A-691C3AD1ADD5@joelfernandes.org>
Date:   Wed, 08 Jul 2020 08:33:49 -0400
From:   joel@...lfernandes.org
To:     madhuparnabhowmik10@...il.com, paulmck@...nel.org
CC:     rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, frextrite@...il.com,
        Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>
Subject: Re: [PATCH] rculist : Introduce list/hlist_for_each_entry_srcu() macros



On July 3, 2020 10:08:28 AM EDT, madhuparnabhowmik10@...il.com wrote:
>From: Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>
>
>list/hlist_for_each_entry_rcu() provides an optional cond argument
>to specify the lock held in the updater side.
>However for SRCU read side, not providing the cond argument results
>into false positive as whether srcu_read_lock is held or not is not
>checked implicitly. Therefore, on read side the lockdep expression
>srcu_read_lock_held(srcu struct) can solve this issue.
>
>However, the function still fails to check the cases where srcu
>protected list is traversed with rcu_read_lock() instead of
>srcu_read_lock(). Therefore, to remove the false negative,
>this patch introduces two new list traversal primitives :
>list_for_each_entry_srcu() and hlist_for_each_entry_srcu().
>
>Both of the functions have non-optional cond argument
>as it is required for both read and update side, and simply checks
>if the cond is true.

Looks ok to me. Could you update the comment below to also clarify that in regular read side usage, the traversal can be done by also passing the expression srcu_read_lock_held which is also a lockdep expression.

Could you post the user-patches along with it? That gives more context to reviewers.

Thanks!

- Joel


>
>Suggested-by: Paolo Bonzini <pbonzini@...hat.com>
>Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>
>---
> include/linux/rculist.h | 44 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
>diff --git a/include/linux/rculist.h b/include/linux/rculist.h
>index df587d181844..04a7e5791c39 100644
>--- a/include/linux/rculist.h
>+++ b/include/linux/rculist.h
>@@ -63,9 +63,17 @@ static inline void INIT_LIST_HEAD_RCU(struct
>list_head *list)
> 	RCU_LOCKDEP_WARN(!(cond) && !rcu_read_lock_any_held(),		\
> 			 "RCU-list traversed in non-reader section!");	\
> 	})
>+
>+#define __list_check_srcu(cond)					 \
>+	({								 \
>+	RCU_LOCKDEP_WARN(!(cond),					 \
>+		"RCU-list traversed without holding the required lock!");\
>+	})
> #else
> #define __list_check_rcu(dummy, cond, extra...)				\
> 	({ check_arg_count_one(extra); })
>+
>+#define __list_check_srcu(cond)
> #endif
> 
> /*
>@@ -383,6 +391,23 @@ static inline void
>list_splice_tail_init_rcu(struct list_head *list,
> 		&pos->member != (head);					\
> 		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> 
>+/**
>+ * list_for_each_entry_srcu	-	iterate over rcu list of given type
>+ * @pos:	the type * to use as a loop cursor.
>+ * @head:	the head for your list.
>+ * @member:	the name of the list_head within the struct.
>+ * @cond:	lockdep expression for the lock required to traverse the
>list.
>+ *
>+ * This list-traversal primitive may safely run concurrently with
>+ * the _rcu list-mutation primitives such as list_add_rcu()
>+ * as long as the traversal is guarded by srcu_read_lock().
>+ */
>+#define list_for_each_entry_srcu(pos, head, member, cond)		\
>+	for (__list_check_srcu(cond),					\
>+	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
>+		&pos->member != (head);					\
>+		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
>+
> /**
>  * list_entry_lockless - get the struct for this entry
>  * @ptr:        the &struct list_head pointer.
>@@ -681,6 +706,25 @@ static inline void hlist_add_behind_rcu(struct
>hlist_node *n,
> 		pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> 			&(pos)->member)), typeof(*(pos)), member))
> 
>+/**
>+ * hlist_for_each_entry_srcu - iterate over rcu list of given type
>+ * @pos:	the type * to use as a loop cursor.
>+ * @head:	the head for your list.
>+ * @member:	the name of the hlist_node within the struct.
>+ * @cond:	lockdep expression for the lock required to traverse the
>list.
>+ *
>+ * This list-traversal primitive may safely run concurrently with
>+ * the _rcu list-mutation primitives such as hlist_add_head_rcu()
>+ * as long as the traversal is guarded by srcu_read_lock().
>+ */
>+#define hlist_for_each_entry_srcu(pos, head, member, cond)		\
>+	for (__list_check_srcu(cond),					\
>+	     pos =
>hlist_entry_safe(rcu_dereference_raw(hlist_first_rcu(head)),\
>+			typeof(*(pos)), member);			\
>+		pos;							\
>+		pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
>+			&(pos)->member)), typeof(*(pos)), member))
>+
> /**
>* hlist_for_each_entry_rcu_notrace - iterate over rcu list of given
>type (for tracing)
>  * @pos:	the type * to use as a loop cursor.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.