2013-10-11 23:17:10

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13

Hello!

This series features updates to allow sparse to do a better job of
statically analyzing RCU usage:

1. Add a comment indicating that despite appearances,
rcu_assign_pointer() really only evaluates its arguments once,
as a cpp macro should.

2-13. Apply ACCESS_ONCE() to avoid a number of rcu_assign_pointer()
calls that would otherwise suffer sparse false positives given
patch #13 below.

14. Apply ACCESS_ONCE() to rcu_assign_pointer()'s target to prevent
comiler mischief. Also require that the source pointer be from
the kernel address space. Sometimes it can be from the RCU address
space, which necessitates the remaining patches in this series.
Which, it must be admitted, apply to a very small fraction of
the rcu_assign_pointer() invocations in the kernel. This commit
courtesy of Josh Triplett.

Changes from v2:

o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that
the pointers are all --rcu and already visible to readers,
as suggested by Eric Dumazet and Josh Triplett.

o Place the commit adding the rcu_assign_pointer()'s ACCESS_ONCE()
at the end to allow better bisectability, as suggested by Josh
Triplett.

o Add a comment to rcu_assign_pointer() noting that it only evaluates
its arguments once, as suggested by Josh Triplett.

Changes from v1:

o Fix grammar nit in commit logs.

Thanx, Paul


b/drivers/net/bonding/bond_alb.c | 3 ++-
b/drivers/net/bonding/bond_main.c | 8 +++++---
b/include/linux/rcupdate.h | 20 +++++++++++++++++++-
b/kernel/notifier.c | 3 ++-
b/net/bridge/br_mdb.c | 2 +-
b/net/bridge/br_multicast.c | 4 ++--
b/net/decnet/dn_route.c | 8 +++++---
b/net/ipv4/ip_sockglue.c | 3 ++-
b/net/ipv6/ip6_gre.c | 3 ++-
b/net/ipv6/ip6_tunnel.c | 3 ++-
b/net/ipv6/sit.c | 3 ++-
b/net/mac80211/sta_info.c | 7 ++++---
b/net/wireless/scan.c | 32 ++++++++++++++++++--------------
13 files changed, 66 insertions(+), 33 deletions(-)

2013-10-11 23:17:48

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 14/14] rcu: Make rcu_assign_pointer's assignment volatile and type-safe

From: Josh Triplett <[email protected]>

rcu_assign_pointer needs to use ACCESS_ONCE to make the assignment to
the destination pointer volatile, to protect against compilers too
clever for their own good.

In addition, since rcu_assign_pointer force-casts the source pointer to
add the __rcu address space (overriding any existing address space), add
an explicit check that the source pointer has the __kernel address space
to start with.

This new check produces warnings like this, when attempting to assign
from a __user pointer:

test.c:25:9: warning: incorrect type in argument 2 (different address spaces)
test.c:25:9: expected struct foo *<noident>
test.c:25:9: got struct foo [noderef] <asn:1>*badsrc

Signed-off-by: Josh Triplett <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
---
include/linux/rcupdate.h | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f4da2175cde0..24206a8b8a42 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -506,8 +506,17 @@ static inline void rcu_preempt_sleep_check(void)
#ifdef __CHECKER__
#define rcu_dereference_sparse(p, space) \
((void)(((typeof(*p) space *)p) == p))
+/* The dummy first argument in __rcu_assign_pointer_typecheck makes the
+ * typechecked pointer the second argument, matching rcu_assign_pointer itself;
+ * this avoids confusion about argument numbers in warning messages. */
+#define __rcu_assign_pointer_check_kernel(v) \
+ do { \
+ extern void __rcu_assign_pointer_typecheck(int, typeof(*(v)) __kernel *); \
+ __rcu_assign_pointer_typecheck(0, v); \
+ } while (0)
#else /* #ifdef __CHECKER__ */
#define rcu_dereference_sparse(p, space)
+#define __rcu_assign_pointer_check_kernel(v) do { } while (0)
#endif /* #else #ifdef __CHECKER__ */

#define __rcu_access_pointer(p, space) \
@@ -551,7 +560,8 @@ static inline void rcu_preempt_sleep_check(void)
#define __rcu_assign_pointer(p, v, space) \
do { \
smp_wmb(); \
- (p) = (typeof(*v) __force space *)(v); \
+ __rcu_assign_pointer_check_kernel(v); \
+ ACCESS_ONCE(p) = (typeof(*(v)) __force space *)(v); \
} while (0)


--
1.8.1.5

2013-10-11 23:17:50

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 05/14] decnet: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the use in
dn_insert_route() is legitimate: It is assigning a pointer to an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Thomas Graf <[email protected]>
Cc: Gao feng <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/decnet/dn_route.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index fe32388ea24f..a6ef8b025035 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -344,8 +344,9 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
if (compare_keys(&rth->fld, &rt->fld)) {
/* Put it first */
*rthp = rth->dst.dn_next;
- rcu_assign_pointer(rth->dst.dn_next,
- dn_rt_hash_table[hash].chain);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(rth->dst.dn_next) =
+ dn_rt_hash_table[hash].chain;
rcu_assign_pointer(dn_rt_hash_table[hash].chain, rth);

dst_use(&rth->dst, now);
@@ -358,7 +359,8 @@ static int dn_insert_route(struct dn_route *rt, unsigned int hash, struct dn_rou
rthp = &rth->dst.dn_next;
}

- rcu_assign_pointer(rt->dst.dn_next, dn_rt_hash_table[hash].chain);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(rt->dst.dn_next) = dn_rt_hash_table[hash].chain;
rcu_assign_pointer(dn_rt_hash_table[hash].chain, rt);

dst_use(&rt->dst, now);
--
1.8.1.5

2013-10-11 23:17:45

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 02/14] notifiers: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the use in
notifier_chain_unregister() is legitimate: It is deleting an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
---
kernel/notifier.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/notifier.c b/kernel/notifier.c
index 2d5cc4ccff7f..197eb70805a4 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -51,7 +51,8 @@ static int notifier_chain_unregister(struct notifier_block **nl,
{
while ((*nl) != NULL) {
if ((*nl) == n) {
- rcu_assign_pointer(*nl, n->next);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(*nl) = n->next;
return 0;
}
nl = &((*nl)->next);
--
1.8.1.5

2013-10-11 23:18:22

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 04/14] wireless: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the uses in
cfg80211_combine_bsses() and cfg80211_bss_update() are legitimate:
They are assigning a pointer to an element from an RCU-protected list,
and all elements of this list are already visible to caller.

This commit therefore silences these false positives by laundering
the pointers using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/wireless/scan.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index eeb71480f1af..ac3a47abf195 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -670,8 +670,8 @@ static bool cfg80211_combine_bsses(struct cfg80211_registered_device *dev,
list_add(&bss->hidden_list, &new->hidden_list);
bss->pub.hidden_beacon_bss = &new->pub;
new->refcount += bss->refcount;
- rcu_assign_pointer(bss->pub.beacon_ies,
- new->pub.beacon_ies);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(bss->pub.beacon_ies) = new->pub.beacon_ies;
}

return true;
@@ -705,11 +705,12 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,

old = rcu_access_pointer(found->pub.proberesp_ies);

- rcu_assign_pointer(found->pub.proberesp_ies,
- tmp->pub.proberesp_ies);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(found->pub.proberesp_ies) =
+ tmp->pub.proberesp_ies;
/* Override possible earlier Beacon frame IEs */
- rcu_assign_pointer(found->pub.ies,
- tmp->pub.proberesp_ies);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(found->pub.ies) = tmp->pub.proberesp_ies;
if (old)
kfree_rcu((struct cfg80211_bss_ies *)old,
rcu_head);
@@ -739,13 +740,14 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,

old = rcu_access_pointer(found->pub.beacon_ies);

- rcu_assign_pointer(found->pub.beacon_ies,
- tmp->pub.beacon_ies);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(found->pub.beacon_ies) = tmp->pub.beacon_ies;

/* Override IEs if they were from a beacon before */
if (old == rcu_access_pointer(found->pub.ies))
- rcu_assign_pointer(found->pub.ies,
- tmp->pub.beacon_ies);
+ /* Both --rcu & visible, ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(found->pub.ies) =
+ tmp->pub.beacon_ies;

/* Assign beacon IEs to all sub entries */
list_for_each_entry(bss, &found->hidden_list,
@@ -755,8 +757,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
ies = rcu_access_pointer(bss->pub.beacon_ies);
WARN_ON(ies != old);

- rcu_assign_pointer(bss->pub.beacon_ies,
- tmp->pub.beacon_ies);
+ /* Both --rcu & visible, ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(bss->pub.beacon_ies) =
+ tmp->pub.beacon_ies;
}

if (old)
@@ -803,8 +806,9 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
list_add(&new->hidden_list,
&hidden->hidden_list);
hidden->refcount++;
- rcu_assign_pointer(new->pub.beacon_ies,
- hidden->pub.beacon_ies);
+ /* Both --rcu & visible, ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(new->pub.beacon_ies) =
+ hidden->pub.beacon_ies;
}
} else {
/*
--
1.8.1.5

2013-10-11 23:18:20

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 11/14] bridge/br_mdb: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the use in
__br_mdb_del() is legitimate: They are assigning a pointer to an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences these false positives by laundering
the pointers using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/bridge/br_mdb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 85a09bb5ca51..de7197ba8695 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -447,7 +447,7 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
if (p->port->state == BR_STATE_DISABLED)
goto unlock;

- rcu_assign_pointer(*pp, p->next);
+ ACCESS_ONCE(*pp) = p->next; /* OK: Both --rcu and visible. */
hlist_del_init(&p->mglist);
del_timer(&p->timer);
call_rcu_bh(&p->rcu, br_multicast_free_pg);
--
1.8.1.5

2013-10-11 23:19:15

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 09/14] ipv6/sit: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the use in
ipip6_tunnel_unlink() is legitimate: It is assigning a pointer to an
element from an RCU-protected list, and all elements of this list are
already visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Alexey Kuznetsov <[email protected]>
Cc: James Morris <[email protected]>
Cc: Hideaki YOSHIFUJI <[email protected]>
Cc: Patrick McHardy <[email protected]>
Cc: [email protected]
---
net/ipv6/sit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 7ee5cb96db34..9b976a4b463d 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -157,7 +157,8 @@ static void ipip6_tunnel_unlink(struct sit_net *sitn, struct ip_tunnel *t)
(iter = rtnl_dereference(*tp)) != NULL;
tp = &iter->next) {
if (t == iter) {
- rcu_assign_pointer(*tp, t->next);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(*tp) = t->next;
break;
}
}
--
1.8.1.5

2013-10-11 23:19:12

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 10/14] mac80211: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the uses in
sta_info_hash_del() are legitimate: They are assigning a pointer to an
element from an RCU-protected list, and all elements of this list are
already visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "John W. Linville" <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/mac80211/sta_info.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index aeb967a0aeed..494f03c0831f 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -74,8 +74,8 @@ static int sta_info_hash_del(struct ieee80211_local *local,
if (!s)
return -ENOENT;
if (s == sta) {
- rcu_assign_pointer(local->sta_hash[STA_HASH(sta->sta.addr)],
- s->hnext);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(local->sta_hash[STA_HASH(sta->sta.addr)]) = s->hnext;
return 0;
}

@@ -84,7 +84,8 @@ static int sta_info_hash_del(struct ieee80211_local *local,
s = rcu_dereference_protected(s->hnext,
lockdep_is_held(&local->sta_mtx));
if (rcu_access_pointer(s->hnext)) {
- rcu_assign_pointer(s->hnext, sta->hnext);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(s->hnext) = sta->hnext;
return 0;
}

--
1.8.1.5

2013-10-11 23:19:10

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 08/14] ipv6/ip6_gre: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the use in
ip6gre_tunnel_unlink() is legitimate: It is assigning a pointer to an
element from an RCU-protected list, and all elements of this list are
already visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Alexey Kuznetsov <[email protected]>
Cc: James Morris <[email protected]>
Cc: Hideaki YOSHIFUJI <[email protected]>
Cc: Patrick McHardy <[email protected]>
Cc: [email protected]
---
net/ipv6/ip6_gre.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 6b26e9feafb9..7bc9e1b3283e 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -276,7 +276,8 @@ static void ip6gre_tunnel_unlink(struct ip6gre_net *ign, struct ip6_tnl *t)
(iter = rtnl_dereference(*tp)) != NULL;
tp = &iter->next) {
if (t == iter) {
- rcu_assign_pointer(*tp, t->next);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(*tp) = t->next;
break;
}
}
--
1.8.1.5

2013-10-11 23:19:07

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 07/14] ipv6/ip6_tunnel: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the use in
ip6_tnl_unlink() is legitimate: It is assigning a pointer to an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Alexey Kuznetsov <[email protected]>
Cc: James Morris <[email protected]>
Cc: Hideaki YOSHIFUJI <[email protected]>
Cc: Patrick McHardy <[email protected]>
Cc: [email protected]
---
net/ipv6/ip6_tunnel.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 61355f7f4da5..2bea7a4e49ed 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -245,7 +245,8 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t)
(iter = rtnl_dereference(*tp)) != NULL;
tp = &iter->next) {
if (t == iter) {
- rcu_assign_pointer(*tp, t->next);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(*tp) = t->next;
break;
}
}
--
1.8.1.5

2013-10-11 23:19:06

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 13/14] bonding/bond_alb.c: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the use in
bond_alb_handle_active_change() is legitimate: It is assigning a pointer
to an element from an RCU-protected list, and all elements of this list
are already visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Alexey Kuznetsov <[email protected]>
Cc: James Morris <[email protected]>
Cc: Hideaki YOSHIFUJI <[email protected]>
Cc: Patrick McHardy <[email protected]>
Cc: [email protected]
---
drivers/net/bonding/bond_alb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 91f179d5135c..67d3b2893aa3 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1667,7 +1667,8 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
}

swap_slave = bond->curr_active_slave;
- rcu_assign_pointer(bond->curr_active_slave, new_slave);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(bond->curr_active_slave) = new_slave;

if (!new_slave || list_empty(&bond->slave_list))
return;
--
1.8.1.5

2013-10-11 23:21:12

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 01/14] rcu: Add comment on evaluate-once properties of rcu_assign_pointer().

From: "Paul E. McKenney" <[email protected]>

The rcu_assign_pointer() macro, as with most cpp macros, must not evaluate
its argument more than once. And it in fact does not. But this might
not be obvious to the casual observer, because one of the arguments
appears no less than three times. However, but one expansion is only
visible to sparse (__CHECKER__), and one lives inside a typeof (where
it will never be evaluated), so this is in fact safe.

This commit therefore adds a comment making this explicit.

Reported-by: Josh Triplett <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
---
include/linux/rcupdate.h | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index f1f1bc39346b..f4da2175cde0 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -911,6 +911,14 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
* rcu_assign_pointer() is a very bad thing that results in
* impossible-to-diagnose memory corruption. So please be careful.
* See the RCU_INIT_POINTER() comment header for details.
+ *
+ * Note that rcu_assign_pointer() evaluates each of its arguments only
+ * once, appearances notwithstanding. One of the "extra" evaluations
+ * is in typeof() and the other visible only to sparse (__CHECKER__),
+ * neither of which actually execute the argument. As with most cpp
+ * macros, this execute-arguments-only-once property is important, so
+ * please be careful when making changes to rcu_assign_pointer() and the
+ * other macros that it invokes.
*/
#define rcu_assign_pointer(p, v) \
__rcu_assign_pointer((p), (v), __rcu)
--
1.8.1.5

2013-10-11 23:21:10

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 06/14] ipv4/ip_socketglue: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the use in
ip_ra_control() is legitimate: It is assigning a pointer to an element
from an RCU-protected list, and all elements of this list are already
visible to caller.

This commit therefore silences this false positive by laundering the
pointer using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Alexey Kuznetsov <[email protected]>
Cc: James Morris <[email protected]>
Cc: Hideaki YOSHIFUJI <[email protected]>
Cc: Patrick McHardy <[email protected]>
Cc: [email protected]
---
net/ipv4/ip_sockglue.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index d9c4f113d709..a0e7f176e9c8 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -269,7 +269,8 @@ int ip_ra_control(struct sock *sk, unsigned char on,
}
/* dont let ip_call_ra_chain() use sk again */
ra->sk = NULL;
- rcu_assign_pointer(*rap, ra->next);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(*rap) = ra->next;
spin_unlock_bh(&ip_ra_lock);

if (ra->destructor)
--
1.8.1.5

2013-10-11 23:21:08

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 12/14] bonding/bond_main: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the uses in
bond_change_active_slave(), bond_enslave(), and __bond_release_one()
are legitimate: They are assigning a pointer to an element from an
RCU-protected list (or a NULL pointer), and all elements of this list
are already visible to caller.

This commit therefore silences these false positives either by laundering
the pointers using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett, or by using RCU_INIT_POINTER() for NULL pointer assignments.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/net/bonding/bond_main.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 72df399c4ab3..e4270ae1c0a8 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -890,7 +890,8 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
if (new_active)
bond_set_slave_active_flags(new_active);
} else {
- rcu_assign_pointer(bond->curr_active_slave, new_active);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(bond->curr_active_slave) = new_active;
}

if (bond->params.mode == BOND_MODE_ACTIVEBACKUP) {
@@ -1601,7 +1602,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
* so we can change it without calling change_active_interface()
*/
if (!bond->curr_active_slave && new_slave->link == BOND_LINK_UP)
- rcu_assign_pointer(bond->curr_active_slave, new_slave);
+ /* Both --rcu and visible, so ACCESS_ONCE() is OK. */
+ ACCESS_ONCE(bond->curr_active_slave) = new_slave;

break;
} /* switch(bond_mode) */
@@ -1801,7 +1803,7 @@ static int __bond_release_one(struct net_device *bond_dev,
}

if (all) {
- rcu_assign_pointer(bond->curr_active_slave, NULL);
+ RCU_INIT_POINTER(bond->curr_active_slave, NULL);
} else if (oldcurrent == slave) {
/*
* Note that we hold RTNL over this sequence, so there
--
1.8.1.5

2013-10-11 23:21:06

by Paul E. McKenney

Subject: [PATCH v3 tip/core/rcu 03/14] bridge: Apply ACCESS_ONCE() to avoid sparse false positive

From: "Paul E. McKenney" <[email protected]>

The sparse checking for rcu_assign_pointer() was recently upgraded
to reject non-__kernel address spaces. This also rejects __rcu,
which is almost always the right thing to do. However, the uses in
br_multicast_del_pg() and br_multicast_new_port_group() are legitimate:
They are assigning a pointer to an element from an RCU-protected list,
and all elements of this list are already visible to caller.

This commit therefore silences these false positives by laundering
the pointers using ACCESS_ONCE() as suggested by Eric Dumazet and Josh
Triplett.

Reported-by: kbuild test robot <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/bridge/br_multicast.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index d1c578630678..bcc4bbc16498 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -267,7 +267,7 @@ static void br_multicast_del_pg(struct net_bridge *br,
if (p != pg)
continue;

- rcu_assign_pointer(*pp, p->next);
+ ACCESS_ONCE(*pp) = p->next; /* OK: Both --rcu and visible. */
hlist_del_init(&p->mglist);
del_timer(&p->timer);
call_rcu_bh(&p->rcu, br_multicast_free_pg);
@@ -646,7 +646,7 @@ struct net_bridge_port_group *br_multicast_new_port_group(
p->addr = *group;
p->port = port;
p->state = state;
- rcu_assign_pointer(p->next, next);
+ ACCESS_ONCE(p->next) = next; /* OK: Both --rcu and visible. */
hlist_add_head(&p->mglist, &port->mglist);
setup_timer(&p->timer, br_multicast_port_group_expired,
(unsigned long)p);
--
1.8.1.5

2013-10-12 06:53:48

by Josh Triplett

Subject: Re: [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13

On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote:
> Changes from v2:
>
> o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that
> the pointers are all --rcu and already visible to readers,
> as suggested by Eric Dumazet and Josh Triplett.

Hang on a moment. Do *none* of these cases need write memory barriers?

- Josh Triplet

2013-10-12 17:13:56

by Paul E. McKenney

Subject: Re: [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13

On Fri, Oct 11, 2013 at 11:53:27PM -0700, Josh Triplett wrote:
> On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote:
> > Changes from v2:
> >
> > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that
> > the pointers are all --rcu and already visible to readers,
> > as suggested by Eric Dumazet and Josh Triplett.
>
> Hang on a moment. Do *none* of these cases need write memory barriers?

Sigh. Some afternoons it doesn't pay to touch the keyboard.

Thank you for catching this. I will fix, but at this point, I am thinking
in terms of 3.14 rather than 3.13 for this series.

Thanx, Paul

2013-10-12 17:39:33

by Hannes Frederic Sowa

Subject: Re: [Bridge] [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13

On Sat, Oct 12, 2013 at 10:13:45AM -0700, Paul E. McKenney wrote:
> On Fri, Oct 11, 2013 at 11:53:27PM -0700, Josh Triplett wrote:
> > On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote:
> > > Changes from v2:
> > >
> > > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that
> > > the pointers are all --rcu and already visible to readers,
> > > as suggested by Eric Dumazet and Josh Triplett.
> >
> > Hang on a moment. Do *none* of these cases need write memory barriers?
>
> Sigh. Some afternoons it doesn't pay to touch the keyboard.
>
> Thank you for catching this. I will fix, but at this point, I am thinking
> in terms of 3.14 rather than 3.13 for this series.

Some of them looked safe. You could also replace --rcu with __rcu in the
comments while at it.

Thanks,

Hannes

2013-10-12 17:43:58

by Hannes Frederic Sowa

Subject: Re: [Bridge] [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13

On Sat, Oct 12, 2013 at 07:39:30PM +0200, Hannes Frederic Sowa wrote:
> On Sat, Oct 12, 2013 at 10:13:45AM -0700, Paul E. McKenney wrote:
> > On Fri, Oct 11, 2013 at 11:53:27PM -0700, Josh Triplett wrote:
> > > On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote:
> > > > Changes from v2:
> > > >
> > > > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that
> > > > the pointers are all --rcu and already visible to readers,
> > > > as suggested by Eric Dumazet and Josh Triplett.
> > >
> > > Hang on a moment. Do *none* of these cases need write memory barriers?
> >
> > Sigh. Some afternoons it doesn't pay to touch the keyboard.
> >
> > Thank you for catching this. I will fix, but at this point, I am thinking
> > in terms of 3.14 rather than 3.13 for this series.
>
> Some of them looked safe. You could also replace --rcu with __rcu in the
> comments while at it.

Most of them deal with management, maybe a rtnl_assign_pointer with lockdep
check for rtnl lock could help to not clean up the wrong bits.

I don't know if rtnl_assign_pointer is that a could name as it does not really
explain why the barrier is not needed there. :/

Greetings,

Hannes

2013-10-13 11:15:47

by Paul E. McKenney

Subject: Re: [Bridge] [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13

On Sat, Oct 12, 2013 at 07:39:30PM +0200, Hannes Frederic Sowa wrote:
> On Sat, Oct 12, 2013 at 10:13:45AM -0700, Paul E. McKenney wrote:
> > On Fri, Oct 11, 2013 at 11:53:27PM -0700, Josh Triplett wrote:
> > > On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote:
> > > > Changes from v2:
> > > >
> > > > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that
> > > > the pointers are all --rcu and already visible to readers,
> > > > as suggested by Eric Dumazet and Josh Triplett.
> > >
> > > Hang on a moment. Do *none* of these cases need write memory barriers?
> >
> > Sigh. Some afternoons it doesn't pay to touch the keyboard.
> >
> > Thank you for catching this. I will fix, but at this point, I am thinking
> > in terms of 3.14 rather than 3.13 for this series.
>
> Some of them looked safe. You could also replace --rcu with __rcu in the
> comments while at it.

Sigh! Will do, thank you!

Thanx, Paul

2013-10-13 11:17:29

by Paul E. McKenney

Subject: Re: [Bridge] [PATCH v3 tip/core/rcu 0/14] Sparse-related updates for 3.13

On Sat, Oct 12, 2013 at 07:43:54PM +0200, Hannes Frederic Sowa wrote:
> On Sat, Oct 12, 2013 at 07:39:30PM +0200, Hannes Frederic Sowa wrote:
> > On Sat, Oct 12, 2013 at 10:13:45AM -0700, Paul E. McKenney wrote:
> > > On Fri, Oct 11, 2013 at 11:53:27PM -0700, Josh Triplett wrote:
> > > > On Fri, Oct 11, 2013 at 04:16:59PM -0700, Paul E. McKenney wrote:
> > > > > Changes from v2:
> > > > >
> > > > > o Switch from rcu_assign_pointer() to ACCESS_ONCE() given that
> > > > > the pointers are all --rcu and already visible to readers,
> > > > > as suggested by Eric Dumazet and Josh Triplett.
> > > >
> > > > Hang on a moment. Do *none* of these cases need write memory barriers?
> > >
> > > Sigh. Some afternoons it doesn't pay to touch the keyboard.
> > >
> > > Thank you for catching this. I will fix, but at this point, I am thinking
> > > in terms of 3.14 rather than 3.13 for this series.
> >
> > Some of them looked safe. You could also replace --rcu with __rcu in the
> > comments while at it.
>
> Most of them deal with management, maybe a rtnl_assign_pointer with lockdep
> check for rtnl lock could help to not clean up the wrong bits.
>
> I don't know if rtnl_assign_pointer is that a could name as it does not really
> explain why the barrier is not needed there. :/

Beyond a certain point, I need to let people who know more about Linux's
networking implementation handle this sort of thing.

Thanx, Paul