Welcome to Soft32 Linux Forums!
FAQFAQ    SearchSearch      ProfileProfile    Private MessagesPrivate Messages   Log inLog in

[Patch] net: fix incorrect counting in __scm_destroy()

  
   Soft32 Home -> Linux -> Kernel RSS
Next:  [gentoo-user] emerge --depclean does not remove d..  
Author Message
Amerigo Wang

External


Since: May 11, 2009
Posts: 44



(Msg. 1) Posted: Wed Nov 04, 2009 5:20 am
Post subject: [Patch] net: fix incorrect counting in __scm_destroy()
Archived from groups: linux>kernel (more info?)
It seems that in __scm_destroy() we forgot to decrease
the ->count after fput(->fp[i]), this may cause some
problem when we recursively call fput() again.

Signed-off-by: WANG Cong <amwang RemoveThis @redhat.com>
Cc: David S. Miller <davem RemoveThis @davemloft.net>

---
diff --git a/net/core/scm.c b/net/core/scm.c
index b7ba91b..fa53219 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -120,8 +120,10 @@ void __scm_destroy(struct scm_cookie *scm)
fpl = list_first_entry(&work_list, struct scm_fp_list, list);

list_del(&fpl->list);
- for (i=fpl->count-1; i>=0; i--)
+ for (i = fpl->count-1; i >= 0; i--) {
fput(fpl->fp[i]);
+ fpl->count--;
+ }
kfree(fpl);
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to top
Login to vote
Eric Dumazet

External


Since: Jul 03, 2009
Posts: 39



(Msg. 2) Posted: Wed Nov 04, 2009 5:20 am
Post subject: Re: [Patch] net: fix incorrect counting in __scm_destroy() [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)
Amerigo Wang a écrit :
> It seems that in __scm_destroy() we forgot to decrease
> the ->count after fput(->fp[i]), this may cause some
> problem when we recursively call fput() again.
>
> Signed-off-by: WANG Cong <amwang DeleteThis @redhat.com>
> Cc: David S. Miller <davem DeleteThis @davemloft.net>
>
> ---
> diff --git a/net/core/scm.c b/net/core/scm.c
> index b7ba91b..fa53219 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -120,8 +120,10 @@ void __scm_destroy(struct scm_cookie *scm)
> fpl = list_first_entry(&work_list, struct scm_fp_list, list);
>
> list_del(&fpl->list);
> - for (i=fpl->count-1; i>=0; i--)
> + for (i = fpl->count-1; i >= 0; i--) {
> fput(fpl->fp[i]);
> + fpl->count--;
> + }
> kfree(fpl);
> }
>

Hmm, your patch seems suspicious.

Are you fixing a real crash/bug, or is it something you discovered in a code review ?

Given we kfree(fpl) at the end of loop, we cannot recursively call __scm_destroy()
on same fpl, it would be a bug anyway ?

So you probably need something better, like testing fpl->list being not re-included
in current->scm_work_list before kfree() it

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to top
Login to vote
David Miller

External


Since: May 26, 2007
Posts: 1109



(Msg. 3) Posted: Wed Nov 04, 2009 7:20 am
Post subject: Re: [Patch] net: fix incorrect counting in __scm_destroy() [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)
From: Eric Dumazet <eric.dumazet.TakeThisOut@gmail.com>
Date: Wed, 04 Nov 2009 11:29:05 +0100

> Given we kfree(fpl) at the end of loop, we cannot recursively call
> __scm_destroy() on same fpl, it would be a bug anyway ?
>
> So you probably need something better, like testing fpl->list being
> not re-included in current->scm_work_list before kfree() it

I can't even see what the problem is.

The code is designed such that the ->count only matters for
the top level.

If we recursively fput() and get back here, we'll see that
there is someone higher in the call chain already running
the fput() loop and we'll just list_add_tail().

The inner while() loop will make sure we process such
entries once we get back to the top level and exit the
for() loop.

Amerigo, please show us the problematic code path where the counts go
wrong and this causes problems.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to top
Login to vote
Cong Wang

External


Since: Nov 10, 2009
Posts: 1



(Msg. 4) Posted: Tue Nov 10, 2009 3:20 am
Post subject: Re: [Patch] net: fix incorrect counting in __scm_destroy() [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)
David Miller wrote:
> From: Eric Dumazet <eric.dumazet.DeleteThis@gmail.com>
> Date: Wed, 04 Nov 2009 11:29:05 +0100
>
>> Given we kfree(fpl) at the end of loop, we cannot recursively call
>> __scm_destroy() on same fpl, it would be a bug anyway ?
>>
>> So you probably need something better, like testing fpl->list being
>> not re-included in current->scm_work_list before kfree() it
>
> I can't even see what the problem is.
>
> The code is designed such that the ->count only matters for
> the top level.
>
> If we recursively fput() and get back here, we'll see that
> there is someone higher in the call chain already running
> the fput() loop and we'll just list_add_tail().
>
> The inner while() loop will make sure we process such
> entries once we get back to the top level and exit the
> for() loop.
>
> Amerigo, please show us the problematic code path where the counts go
> wrong and this causes problems.

Hi, all.

Thanks for your replies.

I met a soft lockup around this code on ia64, something like:

[<a0000001006394e0>] unix_gc+0x240/0x760
sp=e0000260f002fd70 bsp=e0000260f0029560
[<a000000100634500>] unix_release_sock+0x440/0x460
sp=e0000260f002fdb0 bsp=e0000260f0029508
[<a000000100634560>] unix_release+0x40/0x60
sp=e0000260f002fdb0 bsp=e0000260f00294e8
[<a00000010051fba0>] sock_release+0x80/0x1c0
sp=e0000260f002fdb0 bsp=e0000260f00294c0
[<a00000010051fd60>] sock_close+0x80/0xa0
sp=e0000260f002fdc0 bsp=e0000260f0029498
[<a000000100172280>] __fput+0x1a0/0x420
sp=e0000260f002fdc0 bsp=e0000260f0029458
[<a000000100172540>] fput+0x40/0x60
sp=e0000260f002fdc0 bsp=e0000260f0029438
[<a000000100534a30>] __scm_destroy+0x130/0x1e0
sp=e0000260f002fdc0 bsp=e0000260f0029410
[<a000000100636370>] unix_destruct_fds+0x70/0xa0
sp=e0000260f002fdd0 bsp=e0000260f00293e8
[<a00000010052da30>] __kfree_skb+0x1f0/0x320
sp=e0000260f002fe00 bsp=e0000260f00293c0
[<a00000010052dbf0>] kfree_skb+0x90/0xc0
sp=e0000260f002fe00 bsp=e0000260f00293a0
[<a000000100634420>] unix_release_sock+0x360/0x460
sp=e0000260f002fe00 bsp=e0000260f0029348
[<a000000100634560>] unix_release+0x40/0x60
sp=e0000260f002fe00 bsp=e0000260f0029328
[<a00000010051fba0>] sock_release+0x80/0x1c0
sp=e0000260f002fe00 bsp=e0000260f0029300
[<a00000010051fd60>] sock_close+0x80/0xa0
sp=e0000260f002fe10 bsp=e0000260f00292d8
[<a000000100172280>] __fput+0x1a0/0x420
sp=e0000260f002fe10 bsp=e0000260f0029298
[<a000000100172540>] fput+0x40/0x60
sp=e0000260f002fe10 bsp=e0000260f0029278


Yes, this even happens after commit f8d570a47.

But after doing a bisect, we found another hrtimer patch fixes this
problem, so it's not a bug of __scm_destroy().

Sorry for the noise.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to top
Login to vote
Eric Dumazet

External


Since: Jul 03, 2009
Posts: 39



(Msg. 5) Posted: Tue Nov 10, 2009 3:20 am
Post subject: Re: [Patch] net: fix incorrect counting in __scm_destroy() [Login to view extended thread Info.]
Archived from groups: per prev. post (more info?)
Cong Wang a écrit :
>
> Yes, this even happens after commit f8d570a47.
>
> But after doing a bisect, we found another hrtimer patch fixes this
> problem, so it's not a bug of __scm_destroy().
>
> Sorry for the noise.
>

Thanks for the explanation !
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Back to top
Login to vote
Display posts from previous:   
Related Topics:
[PATCH] sl82c105: Switch to ref counting API - Not sure how this one got missed in the great purge some time ago but it did. Signed-off-by: Alan Cox..

[PATCH] openprom: Switch to ref counting PCI API - Signed-off-by: Alan Cox <alan@redhat.com> diff -u --new-file --recursive --exclude-from /usr/src/exclude..

[PATCH] via-pmu: Switch to ref counting PCI API - Signed-off-by: Alan Cox <alan@redhat.com> diff -u --new-file --recursive --exclude-from /usr/src/exclude..

[PATCH] sworks-agp: Switch to PCI ref counting APIs - Signed-off-by: Alan Cox <alan@redhat.com> diff -u --new-file --recursive --exclude-from /usr/src/exclude..

[PATCH 2.6.20 07/10] nfnetlink_log: fix module reference c.. - Count module references correctly: after instance_destroy() there might be timer pending and holding a reference for..

[PATCH-2.4] jfs: incorrect use of "&&" instead of "&" - Hi Dave, I'm about to merge this fix in 2.4. It's already in 2.6 BTW. Do you have any objection ? Thanks in advance,....
       Soft32 Home -> Linux -> Kernel All times are: Pacific Time (US & Canada) (change)
Page 1 of 1

 
 You can post new topics in this forum
You can reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum

Categories:
 Windows
  Linux
 Mac
 PDA

[ Contact us | Terms of Service/Privacy Policy ]