| Message ID | 1332878032.3547.39.camel@edumazet-glaptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 03/27/2012 12:53 PM, Eric Dumazet wrote: > Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() ) > added a regression in rt6_fill_node(), leading to rcu_read_lock() > imbalance. > > Thats because NLA_PUT() can make a jump to nla_put_failure label. Ohhh, how truly awful! That NLA_PUT with hidden jump seems like bugs just waiting to happen! Maybe at least someone clever could make some static analysis tool to catch that.... I'm back-porting and will test shortly. Thanks, Ben
On 03/27/2012 12:53 PM, Eric Dumazet wrote: > Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() ) > added a regression in rt6_fill_node(), leading to rcu_read_lock() > imbalance. > > Thats because NLA_PUT() can make a jump to nla_put_failure label. > > Fix this by using nla_put() > > Many thanks to Ben Greear for his help > > Reported-by: Ben Greear<greearb@candelatech.com> > Reported-by: Dave Jones<davej@redhat.com> > Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com> This does indeed fix the problem for me (tested in the 3.0.25 kernel). So, please feel free to add: Tested-by: Ben Greear <greearb@candelatech.com> Thanks, Ben
On Tue, Mar 27, 2012 at 01:17:09PM -0700, Ben Greear wrote: > On 03/27/2012 12:53 PM, Eric Dumazet wrote: > >Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() ) > >added a regression in rt6_fill_node(), leading to rcu_read_lock() > >imbalance. > > > >Thats because NLA_PUT() can make a jump to nla_put_failure label. > > > >Fix this by using nla_put() > > > >Many thanks to Ben Greear for his help > > > >Reported-by: Ben Greear<greearb@candelatech.com> > >Reported-by: Dave Jones<davej@redhat.com> > >Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com> > > This does indeed fix the problem for me (tested in the 3.0.25 kernel). > So, please feel free to add: > > Tested-by: Ben Greear <greearb@candelatech.com> Very nice job Eric, thanks for tracking this down. greg k-h -- 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
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 27 Mar 2012 21:53:52 +0200 > Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() ) > added a regression in rt6_fill_node(), leading to rcu_read_lock() > imbalance. > > Thats because NLA_PUT() can make a jump to nla_put_failure label. > > Fix this by using nla_put() > > Many thanks to Ben Greear for his help > > Reported-by: Ben Greear <greearb@candelatech.com> > Reported-by: Dave Jones <davej@redhat.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Great work everyone. I'll apply this and queue it up for stable soon. In other news, I think the days of hidden gotos from the NLA macros should be over. I'll work in net-next to redo this so that the gotos must be explicitly coded and therefore be visible when people audit these routines. Thanks! -- 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 3/27/2012 3:22 PM, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 27 Mar 2012 21:53:52 +0200 > >> Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() ) >> added a regression in rt6_fill_node(), leading to rcu_read_lock() >> imbalance. >> >> Thats because NLA_PUT() can make a jump to nla_put_failure label. >> >> Fix this by using nla_put() >> >> Many thanks to Ben Greear for his help >> >> Reported-by: Ben Greear <greearb@candelatech.com> >> Reported-by: Dave Jones <davej@redhat.com> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > Great work everyone. > > I'll apply this and queue it up for stable soon. > > In other news, I think the days of hidden gotos from the NLA macros > should be over. I'll work in net-next to redo this so that the > gotos must be explicitly coded and therefore be visible when people > audit these routines. > > Thanks! > -- I can clean up the ./net/dcb/ code if it will save you some time? .John -- 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
From: John Fastabend <john.r.fastabend@intel.com> Date: Tue, 27 Mar 2012 17:54:02 -0700 > I can clean up the ./net/dcb/ code if it will save you some time? Thanks for offering John, but I can take care of this all myself. -- 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
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index 24c456e..496b627 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -2474,8 +2474,12 @@ static int rt6_fill_node(struct net *net, rcu_read_lock(); n = dst_get_neighbour_noref(&rt->dst); - if (n) - NLA_PUT(skb, RTA_GATEWAY, 16, &n->primary_key); + if (n) { + if (nla_put(skb, RTA_GATEWAY, 16, &n->primary_key) < 0) { + rcu_read_unlock(); + goto nla_put_failure; + } + } rcu_read_unlock(); if (rt->dst.dev)
Commit f2c31e32b378 (net: fix NULL dereferences in check_peer_redir() ) added a regression in rt6_fill_node(), leading to rcu_read_lock() imbalance. Thats because NLA_PUT() can make a jump to nla_put_failure label. Fix this by using nla_put() Many thanks to Ben Greear for his help Reported-by: Ben Greear <greearb@candelatech.com> Reported-by: Dave Jones <davej@redhat.com> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- net/ipv6/route.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) -- 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