diff mbox series

[2/4] mac80211: Free mpath object when rhashtable insertion fails

Message ID E1gtvgu-0006bT-4G@gondobar (mailing list archive)
State Superseded
Delegated to: Johannes Berg
Headers show
Series mac80211: Fix incorrect usage of rhashtable walk API | expand

Commit Message

Herbert Xu Feb. 13, 2019, 2:39 p.m. UTC
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(-)

Comments

Johannes Berg Feb. 13, 2019, 3:04 p.m. UTC | #1
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
Herbert Xu Feb. 14, 2019, 9:41 a.m. UTC | #2
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!
Herbert Xu Feb. 14, 2019, 2:04 p.m. UTC | #3
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 mbox series

Patch

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