diff mbox

mesh RCU issues

Message ID BANLkTi=R_Eyz8+iZsk+=Gs4vaNo-0eUVng@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Javier Cardona May 13, 2011, 8:28 p.m. UTC 
On Fri, May 13, 2011 at 12:13 AM, Johannes Berg
<johannes@sipsolutions.net> 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




Thoughts?

j

Comments

Johannes Berg May 13, 2011, 11:30 p.m. UTC | #1 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg May 14, 2011, midnight UTC | #2 
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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
        }
 }