| Message ID | E1gtvgu-0006bT-4G@gondobar |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | mac80211: Fix incorrect usage of rhashtable walk API | expand |
On Wed, 2019-02-13 at 22:39 +0800, Herbert Xu wrote: > + if (ret != -EEXIST) > return ERR_PTR(ret); Surely that should still be "if (ret && ret != -EEXIST)" otherwise you return for the success case too? or "else if"? I'd probably say we should combine all those ifs into something like this? if (ret == 0) { sdata->u.mesh.mesh_paths_generation++; return new_mpath; } else if (ret == -EEXIST) { kfree(new_mpath); return mpath; } else { kfree(new_mpath); return NULL; } Yes, that's a small change in behaviour as to when the generation counter is updated, but I'm pretty sure it really only needs updating when we inserted something, not if we found an old mpath. johannes
On Wed, Feb 13, 2019 at 04:04:29PM +0100, Johannes Berg wrote: > On Wed, 2019-02-13 at 22:39 +0800, Herbert Xu wrote: > > + if (ret != -EEXIST) > > return ERR_PTR(ret); > > Surely that should still be "if (ret && ret != -EEXIST)" otherwise you > return for the success case too? or "else if"? > > I'd probably say we should combine all those ifs into something like this? > > > if (ret == 0) { > sdata->u.mesh.mesh_paths_generation++; > return new_mpath; > } else if (ret == -EEXIST) { > kfree(new_mpath); > return mpath; > } else { > kfree(new_mpath); > return NULL; > } > > > Yes, that's a small change in behaviour as to when the generation > counter is updated, but I'm pretty sure it really only needs updating > when we inserted something, not if we found an old mpath. You are right. Let me fix this and repost. Thanks!
On Wed, Feb 13, 2019 at 04:04:29PM +0100, Johannes Berg wrote: > > Yes, that's a small change in behaviour as to when the generation > counter is updated, but I'm pretty sure it really only needs updating > when we inserted something, not if we found an old mpath. I decided not to do this in my patch as it's not directly related to the kfree issue. But I agree that this makes more sense and we should make that change in another patch. Thanks!
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c index 884a0d212e8b..db5a1aef22db 100644 --- a/net/mac80211/mesh_pathtbl.c +++ b/net/mac80211/mesh_pathtbl.c @@ -436,17 +436,18 @@ struct mesh_path *mesh_path_add(struct ieee80211_sub_if_data *sdata, } while (unlikely(ret == -EEXIST && !mpath)); spin_unlock_bh(&tbl->walk_lock); - if (ret && ret != -EEXIST) + if (ret) + kfree(new_mpath); + + if (ret != -EEXIST) return ERR_PTR(ret); /* At this point either new_mpath was added, or we found a * matching entry already in the table; in the latter case * free the unnecessary new entry. */ - if (ret == -EEXIST) { - kfree(new_mpath); + if (ret == -EEXIST) new_mpath = mpath; - } sdata->u.mesh.mesh_paths_generation++; return new_mpath; } @@ -481,6 +482,9 @@ int mpp_path_add(struct ieee80211_sub_if_data *sdata, hlist_add_head_rcu(&new_mpath->walk_list, &tbl->walk_head); spin_unlock_bh(&tbl->walk_lock); + if (ret) + kfree(new_mpath); + sdata->u.mesh.mpp_paths_generation++; return ret; }
When rhashtable insertion fails the mesh table code doesn't free the now-orphan mesh path object. This patch fixes that. Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> --- net/mac80211/mesh_pathtbl.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)