rculist: list_first_or_null_rcu() should use list_entry_rcu()
PATCH 1/1 rculist: list_first_or_null_rcu() should use list_entry_rcu()
list_first_or_null() should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in two ways.
* It compares __kernel @__ptr with __rcu @__next triggering the following sparse warning.
net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)
* It doesn't perform rcu_dereference*() and computes the entry address using container_of() directly from the __rcu pointer which is inconsitent with other rculist interface. As a result, all three in-kernel users - net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.
Fix it by making list_first_or_null_rcu() dereference ->next directly and then use list_entry_rcu() on it like other rculist accessors.
--- include/linux/rculist.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
include/linux/rculist.h +3 -2
@@ -267,8 +267,9 @@ static inline void list_splice_init_rcu(*/ #define list_first_or_null_rcu (ptr, type, member) \ ({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + struct list_head *__next = __ptr->next; \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \ })/**
Quoted 18 lines · Tejun Heo
list_first_or_null()should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in two ways.* It compares
__kernel@__ptr with__rcu@__next triggering the following sparse warning.
net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)* It doesn't perform rcu_dereference*() and computes the entry address using
container_of()directly from the__rcupointer which is inconsitent with other rculist interface. As a result, all three in-kernel users -net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.Fix it by making
list_first_or_null_rcu()dereference ->next directly and then uselist_entry_rcu()on it like other rculist accessors.
Quoted 8 lines
Signed-off-by Tejun Heo <[email protected]>Reported-by Fengguang Wu <[email protected]>Cc Dipankar Sarma <[email protected]>Cc "Paul E. McKenney" <[email protected]>Cc "David S. Miller" <[email protected]>Cc Li Zefan <[email protected]>Cc Patrick McHardy <[email protected]>
Paul, ping.
Signature · 1 line
tejun
Quoted 34 lines · Tejun Heo
list_first_or_null()should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in two ways.* It compares
__kernel@__ptr with__rcu@__next triggering the following sparse warning.
net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)* It doesn't perform rcu_dereference*() and computes the entry address using
container_of()directly from the__rcupointer which is inconsitent with other rculist interface. As a result, all three in-kernel users -net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.Fix it by making
list_first_or_null_rcu()dereference ->next directly and then uselist_entry_rcu()on it like other rculist accessors.Signed-off-by Tejun Heo <[email protected]>Reported-by Fengguang Wu <[email protected]>Cc Dipankar Sarma <[email protected]>Cc "Paul E. McKenney" <[email protected]>Cc "David S. Miller" <[email protected]>Cc Li Zefan <[email protected]>Cc Patrick McHardy <[email protected]>--- include/linux/rculist.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)include/linux/rculist.h +0
-0@@ -267,8 +267,9 @@ static inline void list_splice_init_rcu(*/
include/linux/rculist.h static inline void list_splice_init_rcu( · line 275
Quoted 10 lines
include/linux/rculist.h +3
-2#define list_first_or_null_rcu (ptr, type, member) \ ({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + struct list_head *__next = __ptr->next; \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \ })/**
I am a bit uneasy with this, and would feel better if the volatile cast was on the very first fetch of the ->next pointer.
Is there some reason why my unease is ill-founded?
Thanx, Paul
Hello, Paul.
Quoted 11 lines · Paul E. McKenney
include/linux/rculist.h static inline void list_splice_init_rcu( · line 272
Quoted 7 lines
#define
list_first_or_null_rcu(ptr, type, member)\include/linux/rculist.h +3
-2({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + struct list_head *__next = __ptr->next; \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \I am a bit uneasy with this, and would feel better if the volatile cast was on the very first fetch of the ->next pointer.
Quoted 1 line
Is there some reason why my unease is ill-founded?
Do you mean something like the following?
struct list_head *__next = ACCESS_ONCE(__ptr->next); \ likely(__ptr != __next) ? \ list_entry_rcu(__next, type, member) : NULL; \
Yeah, that looks right to me.
Thanks.
Signature · 1 line
tejun
Quoted 16 lines · Tejun Heo
Hello, Paul.
Quoted 11 lines · Paul E. McKenney
include/linux/rculist.h static inline void list_splice_init_rcu( · line 272
Quoted 7 lines
#define
list_first_or_null_rcu(ptr, type, member)\include/linux/rculist.h +3
-2({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + struct list_head *__next = __ptr->next; \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \I am a bit uneasy with this, and would feel better if the volatile cast was on the very first fetch of the ->next pointer.
Quoted 1 line
Is there some reason why my unease is ill-founded?
Quoted 7 lines
Do you mean something like the following?
struct list_head *__next = ACCESS_ONCE(__ptr->next); \ likely(__ptr != __next) ? \ list_entry_rcu(__next, type, member) : NULL; \Yeah, that looks right to me.
I would feel much better about this! Does it avoid warnings in your use cases?
Thanx, Paul
Quoted 10 lines · Paul E. McKenney
Quoted 7 lines
Do you mean something like the following?
struct list_head *__next = ACCESS_ONCE(__ptr->next); \ likely(__ptr != __next) ? \ list_entry_rcu(__next, type, member) : NULL; \Yeah, that looks right to me.
I would feel much better about this! Does it avoid warnings in your use cases?
Yeah, it does, and more importantly it adds the missing read barrier during RCU deref. I'll give it another test and post the updated version.
Thanks a lot!
Signature · 1 line
tejun
PATCH v2 1/1 rculist: list_first_or_null_rcu() should use list_entry_rcu()
In reply to Tejun Heolist_first_or_null() should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in several ways.
* It compares __kernel @__ptr with __rcu @__next triggering the following sparse warning.
net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)
* It doesn't perform rcu_dereference*() and computes the entry address using container_of() directly from the __rcu pointer which is inconsitent with other rculist interface. As a result, all three in-kernel users - net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.
* While ->next dereference passes through list_next_rcu(), the compiler is still free to fetch ->next more than once and thus nullify the "__ptr != __next" condition check.
Fix it by making list_first_or_null_rcu() dereference ->next directly using ACCESS_ONCE() and then use list_entry_rcu() on it like other rculist accessors.
v2: Paul pointed out that the compiler may fetch the pointer more than
once nullifying the condition check. ACCESS_ONCE() added on ->next dereference.
---
Paul, I was mistaken. For list_first_or_null_rcu(), @ptr is constant. It's a value which can't change and usually not even a l-value. ACCESS_ONCE() is necessary when dereferencing @ptr->next, which may change while list_first_or_null_rcu() is in progress.
Thanks.
include/linux/rculist.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
include/linux/rculist.h +4 -3
@@ -266,9 +266,10 @@ static inline void list_splice_init_rcu(* primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock(). */ #define list_first_or_null_rcu (ptr, type, member) \ - ({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + ({struct list_head *__ptr = ptr; \ + struct list_head *__next = ACCESS_ONCE(__ptr->next); \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \ })/**
Quoted 35 lines · Tejun Heo
list_first_or_null()should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in several ways.* It compares
__kernel@__ptr with__rcu@__next triggering the following sparse warning.
net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)* It doesn't perform rcu_dereference*() and computes the entry address using
container_of()directly from the__rcupointer which is inconsitent with other rculist interface. As a result, all three in-kernel users -net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.* While ->next dereference passes through
list_next_rcu(), the compiler is still free to fetch ->next more than once and thus nullify the "__ptr!=__next" condition check.Fix it by making
list_first_or_null_rcu()dereference ->next directly usingACCESS_ONCE()and then uselist_entry_rcu()on it like other rculist accessors.v2: Paul pointed out that the compiler may fetch the pointer more than
once nullifying the condition check.ACCESS_ONCE()added on ->next dereference.Signed-off-by Tejun Heo <[email protected]>Reported-by Fengguang Wu <[email protected]>Cc Dipankar Sarma <[email protected]>Cc "Paul E. McKenney" <[email protected]>Cc "David S. Miller" <[email protected]>Cc Li Zefan <[email protected]>Cc Patrick McHardy <[email protected]>
Quoted 7 lines
---
Paul, I was mistaken. Forlist_first_or_null_rcu(), @ptr is constant. It's a value which can't change and usually not even a l-value.ACCESS_ONCE()is necessary when dereferencing @ptr->next, which may change whilelist_first_or_null_rcu()is in progress.Thanks.
Fair enough!
But why drop the parens around "ptr"?
Thanx, Paul
Quoted 20 lines
include/linux/rculist.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)include/linux/rculist.h +4
-3@@ -266,9 +266,10 @@ static inline void list_splice_init_rcu(* primitives such as list_add_rcu() as long as it's guarded by rcu_read_lock(). */ #define list_first_or_null_rcu (ptr, type, member) \ - ({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + ({struct list_head *__ptr = ptr; \ + struct list_head *__next = ACCESS_ONCE(__ptr->next); \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \ })/**
Quoted 1 line · Paul E. McKenney
But why drop the parens around "ptr"?
Oops, when did that happen? :) Will post an updated version right away.
Thanks.
Signature · 1 line
tejun
PATCH v3 1/1 rculist: list_first_or_null_rcu() should use list_entry_rcu()
In reply to Tejun Heolist_first_or_null() should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in several ways.
* It compares __kernel @__ptr with __rcu @__next triggering the following sparse warning.
net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)
* It doesn't perform rcu_dereference*() and computes the entry address using container_of() directly from the __rcu pointer which is inconsitent with other rculist interface. As a result, all three in-kernel users - net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.
* While ->next dereference passes through list_next_rcu(), the compiler is still free to fetch ->next more than once and thus nullify the "__ptr != __next" condition check.
Fix it by making list_first_or_null_rcu() dereference ->next directly using ACCESS_ONCE() and then use list_entry_rcu() on it like other rculist accessors.
v2: Paul pointed out that the compiler may fetch the pointer more than
once nullifying the condition check. ACCESS_ONCE() added on ->next dereference.
v3: Restored () around macro param which was accidentally removed.
Spotted by Paul.
--- include/linux/rculist.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
include/linux/rculist.h +3 -2
@@ -267,8 +267,9 @@ static inline void list_splice_init_rcu(struct list_head *list,*/ #define list_first_or_null_rcu (ptr, type, member) \ ({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + struct list_head *__next = ACCESS_ONCE(__ptr->next); \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \ })/**
Quoted 30 lines · Tejun Heo
list_first_or_null()should test whether the list is empty and return pointer to the first entry if not in a RCU safe manner. It's broken in several ways.* It compares
__kernel@__ptr with__rcu@__next triggering the following sparse warning.
net/core/dev.c:4331:17: error: incompatible types in comparison expression (different address spaces)* It doesn't perform rcu_dereference*() and computes the entry address using
container_of()directly from the__rcupointer which is inconsitent with other rculist interface. As a result, all three in-kernel users -net/core/dev.c, macvlan, cgroup - are buggy. They dereference the pointer w/o going through read barrier.* While ->next dereference passes through
list_next_rcu(), the compiler is still free to fetch ->next more than once and thus nullify the "__ptr!=__next" condition check.Fix it by making
list_first_or_null_rcu()dereference ->next directly usingACCESS_ONCE()and then uselist_entry_rcu()on it like other rculist accessors.v2: Paul pointed out that the compiler may fetch the pointer more than
once nullifying the condition check.ACCESS_ONCE()added on ->next dereference.v3: Restored () around macro param which was accidentally removed.
Spotted by Paul.
Quoted 8 lines
Signed-off-by Tejun Heo <[email protected]>Reported-by Fengguang Wu <[email protected]>Cc Dipankar Sarma <[email protected]>Cc "Paul E. McKenney" <[email protected]>Cc "David S. Miller" <[email protected]>Cc Li Zefan <[email protected]>Cc Patrick McHardy <[email protected]>
Quoted 21 lines
--- include/linux/rculist.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)include/linux/rculist.h +3
-2@@ -267,8 +267,9 @@ static inline void list_splice_init_rcu(struct list_head *list,*/ #define list_first_or_null_rcu (ptr, type, member) \ ({struct list_head *__ptr = (ptr); \ - struct list_head __rcu *__next = list_next_rcu(__ptr); \ - likely(__ptr != __next) ? container_of(__next, type, member) : NULL; \ + struct list_head *__next = ACCESS_ONCE(__ptr->next); \ + likely(__ptr != __next) ? \ + list_entry_rcu(__next, type, member) : NULL; \ })/**
Hello, Paul.
Quoted 1 line · Paul E. McKenney
Reviewed-by Paul E. McKenney <[email protected]>
How should this patch be routed? Probably best to route with other rcu changes?
Thanks.
Signature · 1 line
tejun
Quoted 7 lines · Tejun Heo
Hello, Paul.
Quoted 1 line · Paul E. McKenney
Reviewed-by Paul E. McKenney <[email protected]>How should this patch be routed? Probably best to route with other rcu changes?
Fair point, queued for 3.12. Thank you, Tejun!
Thanx, Paul