Messages in this thread | | | Date | Fri, 28 Jun 2013 10:24:55 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH v2] rculist: list_first_or_null_rcu() should use list_entry_rcu() |
| |
On Wed, Jun 26, 2013 at 10:27:53AM -0700, Tejun Heo wrote: > 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 __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. > > Signed-off-by: Tejun Heo <tj@kernel.org> > Reported-by: Fengguang Wu <fengguang.wu@intel.com> > Cc: Dipankar Sarma <dipankar@in.ibm.com> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Li Zefan <lizefan@huawei.com> > Cc: Patrick McHardy <kaber@trash.net> > Cc: stable@vger.kernel.org > --- > 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.
Fair enough!
But why drop the parens around "ptr"?
Thanx, Paul
> include/linux/rculist.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > --- a/include/linux/rculist.h > +++ b/include/linux/rculist.h > @@ -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; \ > }) > > /** >
| |