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().
Signed-off-by: Pavel Emelyanov <[email protected]>
---
net/mac80211/mesh_pathtbl.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index 5845dc2..727aa52 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -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
David Miller wrote:
> From: Pavel Emelyanov <[email protected]>
> Date: Tue, 06 May 2008 21:46:38 +0400
>
>> 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.
>
> 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.
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?
> Adding the check for some theoretical-or-not future change is
> rediculious.
Well, this makes sense. Shall I resubmit the set?
Thanks,
Pavel
From: Pavel Emelyanov <[email protected]>
Date: Tue, 06 May 2008 21:46:38 +0400
> 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.
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.
On Tue, May 06, 2008 at 10:59:50PM -0700, David Miller wrote:
> 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
Signed-off-by: John W. Linville <[email protected]>
---
net/mac80211/mesh_pathtbl.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index 5845dc2..ab54f77 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -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]
Ingo Oeser wrote:
> Hi Pavel,
>
> regarding:
>
> [PATCH 1/4][MAC80211]: Fix GFP_KERNEL allocation under read lock
> [PATCH 2/4][MAC80211]: Fix not checked kmalloc() result
>
> Pavel Emelyanov schrieb:
>> 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().
>
> What about doing both allocations in succession to local variables,
> share the failure path if an error occours an kfree them unconditionally
> like this?
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.
> 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
>
> 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 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
From: Pavel Emelyanov <[email protected]>
Date: Wed, 07 May 2008 09:54:53 +0400
> David Miller wrote:
> > 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.
>
> 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?
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 :-)
On Thu, May 08, 2008 at 02:51:23PM -0400, John W. Linville wrote:
> On Tue, May 06, 2008 at 10:59:50PM -0700, David Miller wrote:
>
> > 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.
Ah, sorry -- I guess I should have read the whole series before
commenting... :-)
John
--
John W. Linville
[email protected]
Hi Pavel,
regarding:
[PATCH 1/4][MAC80211]: Fix GFP_KERNEL allocation under read lock
[PATCH 2/4][MAC80211]: Fix not checked kmalloc() result
Pavel Emelyanov schrieb:
> 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().
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