| Message ID | 1268753394-17765-1-git-send-email-jslaby@suse.cz |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Acked-by: chavey@google.com On Tue, Mar 16, 2010 at 8:29 AM, Jiri Slaby <jslaby@suse.cz> wrote: > Stanse found that one error path in netpoll_setup dereferences npinfo > even though it is NULL. Avoid that by adding new label and go to that > instead. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Daniel Borkmann <danborkmann@googlemail.com> > Cc: David S. Miller <davem@davemloft.net> > --- > net/core/netpoll.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c > index 7aa6972..d4ec38f 100644 > --- a/net/core/netpoll.c > +++ b/net/core/netpoll.c > @@ -735,7 +735,7 @@ int netpoll_setup(struct netpoll *np) > npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); > if (!npinfo) { > err = -ENOMEM; > - goto release; > + goto put; > } > > npinfo->rx_flags = 0; > @@ -845,7 +845,7 @@ int netpoll_setup(struct netpoll *np) > > kfree(npinfo); > } > - > +put: > dev_put(ndev); > return err; > } > -- > 1.7.0.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2010-03-16 at 16:29 +0100, Jiri Slaby wrote: > Stanse found that one error path in netpoll_setup dereferences npinfo > even though it is NULL. Avoid that by adding new label and go to that > instead. > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > Cc: Daniel Borkmann <danborkmann@googlemail.com> > Cc: David S. Miller <davem@davemloft.net> > --- > net/core/netpoll.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c > index 7aa6972..d4ec38f 100644 > --- a/net/core/netpoll.c > +++ b/net/core/netpoll.c > @@ -735,7 +735,7 @@ int netpoll_setup(struct netpoll *np) > npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); > if (!npinfo) { > err = -ENOMEM; > - goto release; > + goto put; > } > > npinfo->rx_flags = 0; > @@ -845,7 +845,7 @@ int netpoll_setup(struct netpoll *np) > > kfree(npinfo); > } > - > +put: > dev_put(ndev); > return err; > } I don't get it. The source of the branch tests for !ndev->npinfo and the original destination of the branch also tests for !ndev->npinfo. I don't see how it gets dereferenced. This looks like it just patches over a false positive in your tool (which isn't correlating the validity of npinfo with ndev->npinfo) without actually improving the code. However, it seems that we can drop the second check at release if we add your new exit point.
On 03/16/2010 06:12 PM, Matt Mackall wrote: > I don't get it. The source of the branch tests for !ndev->npinfo and the > original destination of the branch also tests for !ndev->npinfo. I don't > see how it gets dereferenced. Let's look at more of the context: if (!ndev->npinfo) { npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); if (!npinfo) { // npinfo is NULL err = -ENOMEM; goto release; } ... release: // npinfo is still NULL if (!ndev->npinfo) { // condition is the same (holds) // dereference below: vvvvvvvvvvvvvvv spin_lock_irqsave(&npinfo->rx_lock, flags); list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) { npe->dev = NULL; } spin_unlock_irqrestore(&npinfo->rx_lock, flags); kfree(npinfo); } thanks,
On Tue, 2010-03-16 at 18:22 +0100, Jiri Slaby wrote: > On 03/16/2010 06:12 PM, Matt Mackall wrote: > > I don't get it. The source of the branch tests for !ndev->npinfo and the > > original destination of the branch also tests for !ndev->npinfo. I don't > > see how it gets dereferenced. > > Let's look at more of the context: > if (!ndev->npinfo) { > npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); > if (!npinfo) { // npinfo is NULL > err = -ENOMEM; > goto release; > } > ... > release: // npinfo is still NULL > if (!ndev->npinfo) { // condition is the same (holds) > // dereference below: vvvvvvvvvvvvvvv > spin_lock_irqsave(&npinfo->rx_lock, flags); > list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) { > npe->dev = NULL; > } > spin_unlock_irqrestore(&npinfo->rx_lock, flags); > > kfree(npinfo); > } Ok, you're correct, I read the second test backwards. Acked-by: Matt Mackall <mpm@selenic.com>
From: Matt Mackall <mpm@selenic.com> Date: Tue, 16 Mar 2010 12:56:00 -0500 > On Tue, 2010-03-16 at 18:22 +0100, Jiri Slaby wrote: >> On 03/16/2010 06:12 PM, Matt Mackall wrote: >> > I don't get it. The source of the branch tests for !ndev->npinfo and the >> > original destination of the branch also tests for !ndev->npinfo. I don't >> > see how it gets dereferenced. >> >> Let's look at more of the context: >> if (!ndev->npinfo) { >> npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); >> if (!npinfo) { // npinfo is NULL >> err = -ENOMEM; >> goto release; >> } >> ... >> release: // npinfo is still NULL >> if (!ndev->npinfo) { // condition is the same (holds) >> // dereference below: vvvvvvvvvvvvvvv >> spin_lock_irqsave(&npinfo->rx_lock, flags); >> list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) { >> npe->dev = NULL; >> } >> spin_unlock_irqrestore(&npinfo->rx_lock, flags); >> >> kfree(npinfo); >> } > > Ok, you're correct, I read the second test backwards. > > Acked-by: Matt Mackall <mpm@selenic.com> Applied, thanks everyone. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Matt Mackall wrote: > On Tue, 2010-03-16 at 18:22 +0100, Jiri Slaby wrote: >> On 03/16/2010 06:12 PM, Matt Mackall wrote: >>> I don't get it. The source of the branch tests for !ndev->npinfo and the >>> original destination of the branch also tests for !ndev->npinfo. I don't >>> see how it gets dereferenced. >> Let's look at more of the context: >> if (!ndev->npinfo) { >> npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); >> if (!npinfo) { // npinfo is NULL >> err = -ENOMEM; >> goto release; >> } >> ... >> release: // npinfo is still NULL >> if (!ndev->npinfo) { // condition is the same (holds) >> // dereference below: vvvvvvvvvvvvvvv >> spin_lock_irqsave(&npinfo->rx_lock, flags); >> list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) { >> npe->dev = NULL; >> } >> spin_unlock_irqrestore(&npinfo->rx_lock, flags); >> >> kfree(npinfo); >> } > > Ok, you're correct, I read the second test backwards. > > Acked-by: Matt Mackall <mpm@selenic.com> > Thanks for fixing this and sorry for not being responsive, obviously it sucks when you have a broken leg and German hospitals do not really have Internet access ... ;) Thanks, Daniel
diff --git a/net/core/netpoll.c b/net/core/netpoll.c index 7aa6972..d4ec38f 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -735,7 +735,7 @@ int netpoll_setup(struct netpoll *np) npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL); if (!npinfo) { err = -ENOMEM; - goto release; + goto put; } npinfo->rx_flags = 0; @@ -845,7 +845,7 @@ int netpoll_setup(struct netpoll *np) kfree(npinfo); } - +put: dev_put(ndev); return err; }
Stanse found that one error path in netpoll_setup dereferences npinfo even though it is NULL. Avoid that by adding new label and go to that instead. Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Daniel Borkmann <danborkmann@googlemail.com> Cc: David S. Miller <davem@davemloft.net> --- net/core/netpoll.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)