: Fix GFP_KERNEL allocation under read lock.

The mesh_path_add() read-locks the pathtbl_resize_lock and calls kmalloc with GFP_KERNEL mask.

Fix it and move the endadd2 label lower. It should be _before_ the if() beyond, but it makes no sense for it being there, so I move it right after this if().

---
 net/mac80211/mesh_pathtbl.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

index 5845dc2..727aa52 100644
@@ -158,14 +158,14 @@ int mesh_path_add(u8 *dst, struct net_device *dev)
 	if (atomic_add_unless(&sdata->u.sta.mpaths, 1, MESH_MAX_MPATHS) == 0)
 		return -ENOSPC;
 
-	read_lock(&pathtbl_resize_lock);
-
 	new_mpath = kzalloc(sizeof(struct mesh_path), GFP_KERNEL);
 	if (!new_mpath) {
 		atomic_dec(&sdata->u.sta.mpaths);
 		err = -ENOMEM;
 		goto endadd2;
 	}
+
+	read_lock(&pathtbl_resize_lock);
 	memcpy(new_mpath->dst, dst, ETH_ALEN);
 	new_mpath->dev = dev;
 	new_mpath->flags = 0;

@@ -202,7 +202,6 @@ int mesh_path_add(u8 *dst, struct net_device *dev)
 
 endadd:
 	spin_unlock(&mesh_paths->hashwlock[hash_idx]);
-endadd2:
 	read_unlock(&pathtbl_resize_lock);
 	if (!err && grow) {
 		struct mesh_table *oldtbl, *newtbl;

@@ -219,6 +218,7 @@ endadd2:
 		mesh_table_free(oldtbl, false);
 		write_unlock(&pathtbl_resize_lock);
 	}
+endadd2:
 	return err;
 }
 

1.5.3.4

Hi Pavel,

regarding:

[PATCH 1/4][MAC80211]: Fix GFP_KERNEL allocation under read lock [PATCH 2/4][MAC80211]: Fix not checked kmalloc() result

What about doing both allocations in succession to local variables, share the failure path if an error occours an kfree them unconditionally like this?

new_node = kmalloc(sizeof(struct mpath_node), GFP_KERNEL); new_mpath = kzalloc(sizeof(struct mesh_path), GFP_KERNEL);

if (!new_node || !new_mpath) {
	kfree(new_mpath);
	kfree(new_node);
	atomic_dec(&sdata->u.sta.mpaths);
	err = -ENOMEM;
	goto endadd2;
}

...
read_lock(...);
...

Rationale: Allocations are always likely to fail/succeed in close succession.

Best Regards

Ingo Oeser

I do not quite like doing so. Since this relies on fact that kfree bears NULL pointers. But if we ever switch from kmalloc to kmem_cache_alloc, this will result in an oops.

And as far as sharing the error paths are concerned, I make such thing in the 4th patch in a more classical manner.

From: Pavel Emelyanov <[email protected]>
Date: Tue, 06 May 2008 21:46:38 +0400

The whole reason we made kfree allow NULL points is so that checks for it would be ommitted at kfree calls sides, whether they be direct or indirect.

Adding the check for some theoretical-or-not future change is rediculious.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hm... I really thought that this check in kfree is just for sanity against some 3rd part code. But why kmem_cache_free() is not such then?

Well, this makes sense. Shall I resubmit the set?

Thanks,
Pavel

From: Pavel Emelyanov <[email protected]>
Date: Wed, 07 May 2008 09:54:53 +0400

That's not what the check was added for.

If you look in the history, there are lots of changes that are of the form "don't check for NULL, kfree() does that" and it decreases kernel text size. That was why the check was added to kfree(), so we could do that.

There have been some discussions about kmem_cache_free() NULL checks, but that tends to keep getting stalled in the mud for whatever reason.

Whether you should resubmit your patch is up to the wireless folks :-)

If you don't do it Ingo Oeser's way then it looks like you would need a patch like the one below. And with the patch below, the last hunk of your patch will probably need to be fixed-up to match.

Should I apply the patch below and wait for you to rebase and resubmit yours?

---

From: John W. Linville <[email protected]>
Subject: [PATCH] mac80211 mesh: add missing check for kmalloc failure

---
 net/mac80211/mesh_pathtbl.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index 5845dc2..ab54f77 100644
@@ -171,6 +171,12 @@ int mesh_path_add(u8 *dst, struct net_device *dev)
 	new_mpath->flags = 0;
 	skb_queue_head_init(&new_mpath->frame_queue);
 	new_node = kmalloc(sizeof(struct mpath_node), GFP_KERNEL);
+	if (!new_node) {
+		atomic_dec(&sdata->u.sta.mpaths);
+		kfree(new_mpath);
+		err = -ENOMEM;
+		goto endadd2;
+	}
 	new_node->mpath = new_mpath;
 	new_mpath->timer.data = (unsigned long) new_mpath;
 	new_mpath->timer.function = mesh_path_timer;

1.5.3.3

-- 
John W. Linville
[email protected]

Ah, sorry -- I guess I should have read the whole series before commenting... :-)

John

John W. Linville
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

If the system is under enough pressure to fail the first allocation, do we really want to try another one just to free things again right away? I'm with Pavel here, let's rather clean up the failure code path.

johannes