2011-05-12 12:25:39

by Johannes Berg

[permalink] [raw]
Subject: mesh RCU issues

I was reviewing sparse RCU warnings in mac80211...

This seems to be some kind of bad joke:

void mesh_path_timer(unsigned long data)
{
struct ieee80211_sub_if_data *sdata;
struct mesh_path *mpath;

rcu_read_lock();
mpath = (struct mesh_path *) data;
mpath = rcu_dereference(mpath);
if (!mpath)
goto endmpathtimer;

????

And indeed I don't see a del_timer_sync() when the mesh path is freed.
But this is _clearly_ totally bogus. Somebody please fix ASAP.

johannes



2011-05-14 03:53:00

by Javier Cardona

[permalink] [raw]
Subject: Re: mesh RCU issues

On Fri, May 13, 2011 at 5:00 PM, Johannes Berg
<[email protected]> wrote:
> On Fri, 2011-05-13 at 13:28 -0700, Javier Cardona wrote:
>
>> diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
>> index 83ce48e..1db8bba 100644
>> --- a/net/mac80211/mesh_pathtbl.c
>> +++ b/net/mac80211/mesh_pathtbl.c
> [snip]
>
> With this patch, I get the warnings below.

Thanks. Any ideas on how to fix sparse? I'd like to see those too :)

> The locking ones are definitely genuine bugs,

I'm not sure I fully understand the sparse message... would this fix
the two locking warnings?

diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index 83ce48e..fbf0c28 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -376,8 +376,10 @@ void mesh_mpath_table_grow(void)

rcu_read_lock();
newtbl = mesh_table_alloc(rcu_dereference(mesh_paths)->size_order + 1);
- if (!newtbl)
+ if (!newtbl) {
+ rcu_read_unlock();
return;
+ }
write_lock_bh(&pathtbl_resize_lock);
oldtbl = mesh_paths;
if (mesh_table_grow(mesh_paths, newtbl) < 0) {
@@ -400,8 +402,10 @@ void mesh_mpp_table_grow(void)

rcu_read_lock();
newtbl = mesh_table_alloc(rcu_dereference(mpp_paths)->size_order + 1);
- if (!newtbl)
+ if (!newtbl) {
+ rcu_read_unlock();
return;
+ }
write_lock_bh(&pathtbl_resize_lock);
oldtbl = mpp_paths;
if (mesh_table_grow(mpp_paths, newtbl) < 0) {



Javier

2011-05-13 23:30:36

by Johannes Berg

[permalink] [raw]
Subject: Re: mesh RCU issues

On Fri, 2011-05-13 at 13:28 -0700, Javier Cardona wrote:

> >> Isn't the call to del_timer_sync() you are looking for in
> >> mesh_path_node_reclaim() ?
> >
> > Hmm, indeed, but it looks like mesh_path_node_free() also frees a node,
> > no? I'd only found the latter function freeing it and got worried.
>
> Ah, I see. Yes, looks like the timer should be deleted there as well.
> Patch will follow.

Thanks!

> > You'll want to apply
> > http://johannes.sipsolutions.net/patches/kernel/all/LATEST/NNN-mac80211-rcu-annotations.patch to get rid of all the other spurious RCU warnings with CONFIG_SPARSE_RCU.
>
> (You probably meant CONFIG_SPARSE_RCU_POINTER)

Yeah, sorry.

> I must be doing something wrong because I don't see the RCU warnings
> after making {mesh,mpp}_paths __rcu. I only see two "different
> address spaces" errors that are fixed in the patch below.
> I do see a bunch of 'warning' errors:
>
> /home/javier/dev/wireless-testing/arch/x86/include/asm/uaccess_32.h:199:2:
> error: attribute 'warning': unknown attribute

Oh, damn. You're running into a sparse issue: upon encountering an
error, it aborts parsing/checking. So since you're getting an error from
an include file, you never see any warnings from the rest of the file.

>
>
> diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
> index 83ce48e..1db8bba 100644
> --- a/net/mac80211/mesh_pathtbl.c
> +++ b/net/mac80211/mesh_pathtbl.c
> @@ -36,8 +36,8 @@ struct mpath_node {
> struct mesh_path *mpath;
> };
>
> -static struct mesh_table *mesh_paths;
> -static struct mesh_table *mpp_paths; /* Store paths for MPP&MAP */
> +static struct mesh_table __rcu *mesh_paths;
> +static struct mesh_table __rcu *mpp_paths; /* Store paths for MPP&MAP */
>
> int mesh_paths_generation;
>
> @@ -513,7 +513,7 @@ void mesh_plink_broken(struct sta_info *sta)
> for_each_mesh_entry(mesh_paths, p, node, i) {
> mpath = node->mpath;
> spin_lock_bh(&mpath->state_lock);
> - if (mpath->next_hop == sta &&
> + if (rcu_dereference(mpath->next_hop) == sta &&
> mpath->flags & MESH_PATH_ACTIVE &&
> !(mpath->flags & MESH_PATH_FIXED)) {
> mpath->flags &= ~MESH_PATH_ACTIVE;
> @@ -549,7 +549,7 @@ void mesh_path_flush_by_nexthop(struct sta_info *sta)
>
> for_each_mesh_entry(mesh_paths, p, node, i) {
> mpath = node->mpath;
> - if (mpath->next_hop == sta)
> + if (rcu_dereference(mpath->next_hop) == sta)
> mesh_path_del(mpath->dst, mpath->sdata);
> }
> }
>
> Thoughts?

I'll try if this is sufficient but it seemed there were more warnings
when I tried.

johannes


2011-05-14 08:24:36

by Johannes Berg

[permalink] [raw]
Subject: Re: mesh RCU issues

On Fri, 2011-05-13 at 20:52 -0700, Javier Cardona wrote:

> > With this patch, I get the warnings below.
>
> Thanks. Any ideas on how to fix sparse? I'd like to see those too :)

No, not really, what version are you using? I'm on 64 bit so I don't
have uaccess_32.h included I guess, I suppose you could just go to that
file and comment out the contents of the copy_from_user_overflow()
function that seems to be causing it grief.

> > The locking ones are definitely genuine bugs,
>
> I'm not sure I fully understand the sparse message... would this fix
> the two locking warnings?
[snip]

Sure, those locking fixes should fix the bugs and get rid of the
warnings unless there was another code path leaving w/o unlock.

johannes


2011-05-12 22:26:58

by Javier Cardona

[permalink] [raw]
Subject: Re: mesh RCU issues

Johannes,

On Thu, May 12, 2011 at 5:25 AM, Johannes Berg
<[email protected]> wrote:
> I was reviewing sparse RCU warnings in mac80211...
>
> This seems to be some kind of bad joke:
>
> void mesh_path_timer(unsigned long data)
> {
> ? ? ? ?struct ieee80211_sub_if_data *sdata;
> ? ? ? ?struct mesh_path *mpath;
>
> ? ? ? ?rcu_read_lock();
> ? ? ? ?mpath = (struct mesh_path *) data;
> ? ? ? ?mpath = rcu_dereference(mpath);
> ? ? ? ?if (!mpath)
> ? ? ? ? ? ? ? ?goto endmpathtimer;
>
> ????

Thanks for cleaning that up.

> And indeed I don't see a del_timer_sync() when the mesh path is freed.

Isn't the call to del_timer_sync() you are looking for in
mesh_path_node_reclaim() ?

> But this is _clearly_ totally bogus. Somebody please fix ASAP.

I'll run sparse and see if I see other rcu warnings that I can fix.

Thanks,

j


> johannes
>
>



--
Javier Cardona
cozybit Inc.
http://www.cozybit.com

2011-05-13 20:29:02

by Javier Cardona

[permalink] [raw]
Subject: Re: mesh RCU issues

On Fri, May 13, 2011 at 12:13 AM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2011-05-12 at 15:26 -0700, Javier Cardona wrote:
>
>> > And indeed I don't see a del_timer_sync() when the mesh path is freed.
>>
>> Isn't the call to del_timer_sync() you are looking for in
>> mesh_path_node_reclaim() ?
>
> Hmm, indeed, but it looks like mesh_path_node_free() also frees a node,
> no? I'd only found the latter function freeing it and got worried.

Ah, I see. Yes, looks like the timer should be deleted there as well.
Patch will follow.

>> > But this is _clearly_ totally bogus. Somebody please fix ASAP.
>>
>> I'll run sparse and see if I see other rcu warnings that I can fix.
>
> Thanks. I think most of the use is probably OK or "just" missing
> rcu_dereference() wrappers. The global mesh_paths and mpp_paths
> variables should also be __rcu I think, but that caused so many warnings
> that I gave up for now, and I didn't quite understand what was going on.
>
> You'll want to apply
> http://johannes.sipsolutions.net/patches/kernel/all/LATEST/NNN-mac80211-rcu-annotations.patch to get rid of all the other spurious RCU warnings with CONFIG_SPARSE_RCU.

(You probably meant CONFIG_SPARSE_RCU_POINTER)

I must be doing something wrong because I don't see the RCU warnings
after making {mesh,mpp}_paths __rcu. I only see two "different
address spaces" errors that are fixed in the patch below.
I do see a bunch of 'warning' errors:

/home/javier/dev/wireless-testing/arch/x86/include/asm/uaccess_32.h:199:2:
error: attribute 'warning': unknown attribute



diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index 83ce48e..1db8bba 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -36,8 +36,8 @@ struct mpath_node {
struct mesh_path *mpath;
};

-static struct mesh_table *mesh_paths;
-static struct mesh_table *mpp_paths; /* Store paths for MPP&MAP */
+static struct mesh_table __rcu *mesh_paths;
+static struct mesh_table __rcu *mpp_paths; /* Store paths for MPP&MAP */

int mesh_paths_generation;

@@ -513,7 +513,7 @@ void mesh_plink_broken(struct sta_info *sta)
for_each_mesh_entry(mesh_paths, p, node, i) {
mpath = node->mpath;
spin_lock_bh(&mpath->state_lock);
- if (mpath->next_hop == sta &&
+ if (rcu_dereference(mpath->next_hop) == sta &&
mpath->flags & MESH_PATH_ACTIVE &&
!(mpath->flags & MESH_PATH_FIXED)) {
mpath->flags &= ~MESH_PATH_ACTIVE;
@@ -549,7 +549,7 @@ void mesh_path_flush_by_nexthop(struct sta_info *sta)

for_each_mesh_entry(mesh_paths, p, node, i) {
mpath = node->mpath;
- if (mpath->next_hop == sta)
+ if (rcu_dereference(mpath->next_hop) == sta)
mesh_path_del(mpath->dst, mpath->sdata);
}
}

Thoughts?

j

--
Javier Cardona
cozybit Inc.
http://www.cozybit.com

2011-05-14 00:00:48

by Johannes Berg

[permalink] [raw]
Subject: Re: mesh RCU issues

On Fri, 2011-05-13 at 13:28 -0700, Javier Cardona wrote:

> diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
> index 83ce48e..1db8bba 100644
> --- a/net/mac80211/mesh_pathtbl.c
> +++ b/net/mac80211/mesh_pathtbl.c
[snip]

With this patch, I get the warnings below. The locking ones are
definitely genuine bugs, but the missing rcu_dereference() ones you
already fixed were bugs too, albeit only on the Alpha architecture (and
maybe elsewhere due to compiler optimisations?)

johannes

CHECK /home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:335:48: warning: incorrect type in argument 3 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:335:48: expected struct mesh_table *tbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:335:48: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] mesh_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:348:13: warning: incorrect type in argument 2 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:348:13: expected struct atomic_t [usertype] *v
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:348:13: got struct atomic_t [noderef] <asn:4>*<noident>
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:382:16: warning: incorrect type in assignment (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:382:16: expected struct mesh_table *oldtbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:382:16: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] mesh_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:383:29: warning: incorrect type in argument 1 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:383:29: expected struct mesh_table *oldtbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:383:29: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] mesh_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:406:16: warning: incorrect type in assignment (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:406:16: expected struct mesh_table *oldtbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:406:16: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] mpp_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:407:29: warning: incorrect type in argument 1 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:407:29: expected struct mesh_table *oldtbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:407:29: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] mpp_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:459:48: warning: incorrect type in argument 3 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:459:48: expected struct mesh_table *tbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:459:48: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] [assigned] mpp_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:472:13: warning: incorrect type in argument 2 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:472:13: expected struct atomic_t [usertype] *v
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:472:13: got struct atomic_t [noderef] <asn:4>*<noident>
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:600:49: warning: incorrect type in argument 3 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:600:49: expected struct mesh_table *tbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:600:49: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] [assigned] mesh_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:612:37: warning: incorrect type in argument 1 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:612:37: expected struct atomic_t [usertype] *v
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:612:37: got struct atomic_t [noderef] <asn:4>*<noident>
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:748:20: warning: incorrect type in assignment (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:748:20: expected struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] [assigned] mesh_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:748:20: got struct mesh_table *
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:755:19: warning: incorrect type in assignment (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:755:19: expected struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] [assigned] mpp_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:755:19: got struct mesh_table *
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:757:33: warning: incorrect type in argument 1 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:757:33: expected struct mesh_table *tbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:757:33: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] [assigned] mesh_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:794:25: warning: incorrect type in argument 1 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:794:25: expected struct mesh_table *tbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:794:25: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] [assigned] mesh_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:795:25: warning: incorrect type in argument 1 (different address spaces)
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:795:25: expected struct mesh_table *tbl
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:795:25: got struct mesh_table [noderef] <asn:4>*static [addressable] [toplevel] [assigned] mpp_paths
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:266:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:266:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:266:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:266:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:336:19: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:338:23: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:349:17: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:349:47: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:354:25: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:363:25: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:373:6: warning: context imbalance in 'mesh_mpath_table_grow' - different lock contexts for basic block
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:397:6: warning: context imbalance in 'mesh_mpp_table_grow' - different lock contexts for basic block
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:460:19: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:462:23: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:473:17: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:473:46: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:476:25: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:485:25: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:513:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:513:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:513:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:513:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:550:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:550:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:550:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:550:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:564:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:564:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:564:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:564:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:601:19: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:603:23: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:621:25: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:751:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:752:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:753:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:760:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:761:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:762:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:775:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:775:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:775:9: warning: dereference of noderef expression
/home/johannes/sys/wireless-testing/net/mac80211/mesh_pathtbl.c:775:9: warning: dereference of noderef expression



2011-05-13 07:13:17

by Johannes Berg

[permalink] [raw]
Subject: Re: mesh RCU issues

On Thu, 2011-05-12 at 15:26 -0700, Javier Cardona wrote:

> > And indeed I don't see a del_timer_sync() when the mesh path is freed.
>
> Isn't the call to del_timer_sync() you are looking for in
> mesh_path_node_reclaim() ?

Hmm, indeed, but it looks like mesh_path_node_free() also frees a node,
no? I'd only found the latter function freeing it and got worried.

> > But this is _clearly_ totally bogus. Somebody please fix ASAP.
>
> I'll run sparse and see if I see other rcu warnings that I can fix.

Thanks. I think most of the use is probably OK or "just" missing
rcu_dereference() wrappers. The global mesh_paths and mpp_paths
variables should also be __rcu I think, but that caused so many warnings
that I gave up for now, and I didn't quite understand what was going on.

You'll want to apply
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/NNN-mac80211-rcu-annotations.patch to get rid of all the other spurious RCU warnings with CONFIG_SPARSE_RCU.

Thanks,
Johannes


2011-05-12 13:03:34

by Johannes Berg

[permalink] [raw]
Subject: [PATCH] mac80211: remove pointless mesh path timer RCU code

From: Johannes Berg <[email protected]>

The code here to RCU-dereference a pointer that's
on the stack is totally pointless, RCU isn't magic
(like say Java's weak references are), so the code
can't work like whoever wrote it thought it might.

Remove it so readers don't get confused. Note that
it seems that a bug is there anyway: I don't see
any code that cancels the timer when a mesh path
struct is destroyed.

Signed-off-by: Johannes Berg <[email protected]>
---
net/mac80211/mesh_hwmp.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)

--- a/net/mac80211/mesh_hwmp.c 2011-05-12 15:01:20.000000000 +0200
+++ b/net/mac80211/mesh_hwmp.c 2011-05-12 15:01:24.000000000 +0200
@@ -967,20 +967,11 @@ endlookup:

void mesh_path_timer(unsigned long data)
{
- struct ieee80211_sub_if_data *sdata;
- struct mesh_path *mpath;
+ struct mesh_path *mpath = (void *) data;
+ struct ieee80211_sub_if_data *sdata = mpath->sdata;

- rcu_read_lock();
- mpath = (struct mesh_path *) data;
- mpath = rcu_dereference(mpath);
- if (!mpath)
- goto endmpathtimer;
- sdata = mpath->sdata;
-
- if (sdata->local->quiescing) {
- rcu_read_unlock();
+ if (sdata->local->quiescing)
return;
- }

spin_lock_bh(&mpath->state_lock);
if (mpath->flags & MESH_PATH_RESOLVED ||
@@ -997,8 +988,6 @@ void mesh_path_timer(unsigned long data)
}

spin_unlock_bh(&mpath->state_lock);
-endmpathtimer:
- rcu_read_unlock();
}

void