* [PATCH net 0/2] fix two kernel panics when disabled IPv6 on boot up
@ 2019-02-06 12:51 Hangbin Liu
2019-02-06 12:51 ` [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Hangbin Liu @ 2019-02-06 12:51 UTC (permalink / raw)
To: netdev; +Cc: Stefano Brivio, David Miller, Hangbin Liu
When disabled IPv6 on boot up, since there is no ipv6 route tables, we should
not call rt6_lookup. Fix them by checking if we have inet6_dev pointer on
netdevice.
Hangbin Liu (2):
geneve: should not call rt6_lookup() when ipv6 was disabled
sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach()
drivers/net/geneve.c | 6 ++++++
net/ipv6/sit.c | 8 +++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
--
2.19.2
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-06 12:51 [PATCH net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu @ 2019-02-06 12:51 ` Hangbin Liu 2019-02-06 14:58 ` Stefano Brivio ` (3 more replies) 2019-02-06 12:51 ` [PATCH net 2/2] sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() Hangbin Liu 2019-02-07 10:36 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu 2 siblings, 4 replies; 16+ messages in thread From: Hangbin Liu @ 2019-02-06 12:51 UTC (permalink / raw) To: netdev; +Cc: Stefano Brivio, David Miller, Hangbin Liu When we add a new GENEVE device with IPv6 remote, checking only for IS_ENABLED(CONFIG_IPV6) is not enough as we may disable IPv6 in kernel cmd(ipv6.disable=1), which will cause a NULL pointer dereference. Reported-by: Jianlin Shi <jishi@redhat.com> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- drivers/net/geneve.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index 58bbba8582b0..0658715581e3 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -1512,6 +1512,10 @@ static void geneve_link_config(struct net_device *dev, } #if IS_ENABLED(CONFIG_IPV6) case AF_INET6: { + struct inet6_dev *idev = in6_dev_get(dev); + if (!idev) + break; + struct rt6_info *rt = rt6_lookup(geneve->net, &info->key.u.ipv6.dst, NULL, 0, NULL, 0); @@ -1519,6 +1523,8 @@ static void geneve_link_config(struct net_device *dev, if (rt && rt->dst.dev) ldev_mtu = rt->dst.dev->mtu - GENEVE_IPV6_HLEN; ip6_rt_put(rt); + + in6_dev_put(idev); break; } #endif -- 2.19.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-06 12:51 ` [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu @ 2019-02-06 14:58 ` Stefano Brivio 2019-02-06 16:43 ` Eric Dumazet ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: Stefano Brivio @ 2019-02-06 14:58 UTC (permalink / raw) To: Hangbin Liu; +Cc: netdev, David Miller Hi Hangbin, On Wed, 6 Feb 2019 20:51:10 +0800 Hangbin Liu <liuhangbin@gmail.com> wrote: > When we add a new GENEVE device with IPv6 remote, checking only for > IS_ENABLED(CONFIG_IPV6) is not enough as we may disable IPv6 in kernel > cmd(ipv6.disable=1), which will cause a NULL pointer dereference. > > Reported-by: Jianlin Shi <jishi@redhat.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > drivers/net/geneve.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index 58bbba8582b0..0658715581e3 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -1512,6 +1512,10 @@ static void geneve_link_config(struct net_device *dev, > } > #if IS_ENABLED(CONFIG_IPV6) > case AF_INET6: { > + struct inet6_dev *idev = in6_dev_get(dev); > + if (!idev) > + break; > + > struct rt6_info *rt = rt6_lookup(geneve->net, > &info->key.u.ipv6.dst, NULL, 0, > NULL, 0); You're mixing declarations and code here, ISO C90 forbids it. You could rather declare: struct inet6_dev *idev = in6_dev_get(dev); struct rt6_info *rt; then check idev, and then call rt6_lookup(). > @@ -1519,6 +1523,8 @@ static void geneve_link_config(struct net_device *dev, > if (rt && rt->dst.dev) > ldev_mtu = rt->dst.dev->mtu - GENEVE_IPV6_HLEN; > ip6_rt_put(rt); > + > + in6_dev_put(idev); I think it would be better to put this right after the check on idev, mostly for readability, but also, marginally, to reduce the scope of the reference count bump. -- Stefano ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-06 12:51 ` [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu 2019-02-06 14:58 ` Stefano Brivio @ 2019-02-06 16:43 ` Eric Dumazet 2019-02-06 18:54 ` David Ahern 2019-02-07 0:55 ` kbuild test robot 3 siblings, 0 replies; 16+ messages in thread From: Eric Dumazet @ 2019-02-06 16:43 UTC (permalink / raw) To: Hangbin Liu, netdev; +Cc: Stefano Brivio, David Miller On 02/06/2019 04:51 AM, Hangbin Liu wrote: > When we add a new GENEVE device with IPv6 remote, checking only for > IS_ENABLED(CONFIG_IPV6) is not enough as we may disable IPv6 in kernel > cmd(ipv6.disable=1), which will cause a NULL pointer dereference. > > Reported-by: Jianlin Shi <jishi@redhat.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > drivers/net/geneve.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index 58bbba8582b0..0658715581e3 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -1512,6 +1512,10 @@ static void geneve_link_config(struct net_device *dev, > } > #if IS_ENABLED(CONFIG_IPV6) > case AF_INET6: { > + struct inet6_dev *idev = in6_dev_get(dev); > + if (!idev) > + break; Do not mix declarations and code. Instead, use : struct inet6_dev *idev = in6_dev_get(dev); struct rt6_info *rt; if (!idev) break; rt = rt6_lookup(geneve->net, ...); > + > struct rt6_info *rt = rt6_lookup(geneve->net, > &info->key.u.ipv6.dst, NULL, 0, > NULL, 0); > @@ -1519,6 +1523,8 @@ static void geneve_link_config(struct net_device *dev, > if (rt && rt->dst.dev) > ldev_mtu = rt->dst.dev->mtu - GENEVE_IPV6_HLEN; > ip6_rt_put(rt); > + > + in6_dev_put(idev); > break; > } > #endif > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-06 12:51 ` [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu 2019-02-06 14:58 ` Stefano Brivio 2019-02-06 16:43 ` Eric Dumazet @ 2019-02-06 18:54 ` David Ahern 2019-02-06 19:04 ` Stefano Brivio 2019-02-06 19:08 ` Eric Dumazet 2019-02-07 0:55 ` kbuild test robot 3 siblings, 2 replies; 16+ messages in thread From: David Ahern @ 2019-02-06 18:54 UTC (permalink / raw) To: Hangbin Liu, netdev; +Cc: Stefano Brivio, David Miller On 2/6/19 4:51 AM, Hangbin Liu wrote: > When we add a new GENEVE device with IPv6 remote, checking only for > IS_ENABLED(CONFIG_IPV6) is not enough as we may disable IPv6 in kernel > cmd(ipv6.disable=1), which will cause a NULL pointer dereference. > > Reported-by: Jianlin Shi <jishi@redhat.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > drivers/net/geneve.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index 58bbba8582b0..0658715581e3 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -1512,6 +1512,10 @@ static void geneve_link_config(struct net_device *dev, > } > #if IS_ENABLED(CONFIG_IPV6) > case AF_INET6: { > + struct inet6_dev *idev = in6_dev_get(dev); > + if (!idev) > + break; Since you don't need an idev reference rcu_access_pointer should be enough to say v6 is enabled on this device. ie., add a helper to check that rcu_access_pointer(dev->ip6_ptr) is non-NULL ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-06 18:54 ` David Ahern @ 2019-02-06 19:04 ` Stefano Brivio 2019-02-06 19:09 ` Eric Dumazet 2019-02-06 19:08 ` Eric Dumazet 1 sibling, 1 reply; 16+ messages in thread From: Stefano Brivio @ 2019-02-06 19:04 UTC (permalink / raw) To: David Ahern; +Cc: Hangbin Liu, netdev, David Miller On Wed, 6 Feb 2019 10:54:01 -0800 David Ahern <dsahern@gmail.com> wrote: > On 2/6/19 4:51 AM, Hangbin Liu wrote: > > When we add a new GENEVE device with IPv6 remote, checking only for > > IS_ENABLED(CONFIG_IPV6) is not enough as we may disable IPv6 in kernel > > cmd(ipv6.disable=1), which will cause a NULL pointer dereference. > > > > Reported-by: Jianlin Shi <jishi@redhat.com> > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > --- > > drivers/net/geneve.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > index 58bbba8582b0..0658715581e3 100644 > > --- a/drivers/net/geneve.c > > +++ b/drivers/net/geneve.c > > @@ -1512,6 +1512,10 @@ static void geneve_link_config(struct net_device *dev, > > } > > #if IS_ENABLED(CONFIG_IPV6) > > case AF_INET6: { > > + struct inet6_dev *idev = in6_dev_get(dev); > > + if (!idev) > > + break; > > Since you don't need an idev reference rcu_access_pointer should be > enough to say v6 is enabled on this device. ie., add a helper to check > that rcu_access_pointer(dev->ip6_ptr) is non-NULL I guess if (__in6_dev_get(dev)) is already good, no? This is called under RTNL. -- Stefano ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-06 19:04 ` Stefano Brivio @ 2019-02-06 19:09 ` Eric Dumazet 0 siblings, 0 replies; 16+ messages in thread From: Eric Dumazet @ 2019-02-06 19:09 UTC (permalink / raw) To: Stefano Brivio, David Ahern; +Cc: Hangbin Liu, netdev, David Miller On 02/06/2019 11:04 AM, Stefano Brivio wrote: > I guess if (__in6_dev_get(dev)) is already good, no? This is called > under RTNL. > Exactly ;) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-06 18:54 ` David Ahern 2019-02-06 19:04 ` Stefano Brivio @ 2019-02-06 19:08 ` Eric Dumazet 1 sibling, 0 replies; 16+ messages in thread From: Eric Dumazet @ 2019-02-06 19:08 UTC (permalink / raw) To: David Ahern, Hangbin Liu, netdev; +Cc: Stefano Brivio, David Miller On 02/06/2019 10:54 AM, David Ahern wrote: > On 2/6/19 4:51 AM, Hangbin Liu wrote: >> When we add a new GENEVE device with IPv6 remote, checking only for >> IS_ENABLED(CONFIG_IPV6) is not enough as we may disable IPv6 in kernel >> cmd(ipv6.disable=1), which will cause a NULL pointer dereference. >> >> Reported-by: Jianlin Shi <jishi@redhat.com> >> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> >> --- >> drivers/net/geneve.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c >> index 58bbba8582b0..0658715581e3 100644 >> --- a/drivers/net/geneve.c >> +++ b/drivers/net/geneve.c >> @@ -1512,6 +1512,10 @@ static void geneve_link_config(struct net_device *dev, >> } >> #if IS_ENABLED(CONFIG_IPV6) >> case AF_INET6: { >> + struct inet6_dev *idev = in6_dev_get(dev); >> + if (!idev) >> + break; > > Since you don't need an idev reference rcu_access_pointer should be > enough to say v6 is enabled on this device. ie., add a helper to check > that rcu_access_pointer(dev->ip6_ptr) is non-NULL > I guess RTNL is held at this point, so we can use a lockdep enabled accessor : __in6_dev_get() ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-06 12:51 ` [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu ` (2 preceding siblings ...) 2019-02-06 18:54 ` David Ahern @ 2019-02-07 0:55 ` kbuild test robot 3 siblings, 0 replies; 16+ messages in thread From: kbuild test robot @ 2019-02-07 0:55 UTC (permalink / raw) To: Hangbin Liu; +Cc: kbuild-all, netdev, Stefano Brivio, David Miller, Hangbin Liu [-- Attachment #1: Type: text/plain, Size: 4504 bytes --] Hi Hangbin, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on net/master] url: https://github.com/0day-ci/linux/commits/Hangbin-Liu/fix-two-kernel-panics-when-disabled-IPv6-on-boot-up/20190207-071954 config: m68k-sun3_defconfig (attached as .config) compiler: m68k-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=8.2.0 make.cross ARCH=m68k All warnings (new ones prefixed by >>): drivers/net/geneve.c: In function 'geneve_link_config': >> drivers/net/geneve.c:1519:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] struct rt6_info *rt = rt6_lookup(geneve->net, ^~~~~~ vim +1519 drivers/net/geneve.c abe492b4f5 Tom Herbert 2015-12-10 1490 c40e89fd35 Alexey Kodanev 2018-04-19 1491 static void geneve_link_config(struct net_device *dev, c40e89fd35 Alexey Kodanev 2018-04-19 1492 struct ip_tunnel_info *info, struct nlattr *tb[]) c40e89fd35 Alexey Kodanev 2018-04-19 1493 { c40e89fd35 Alexey Kodanev 2018-04-19 1494 struct geneve_dev *geneve = netdev_priv(dev); c40e89fd35 Alexey Kodanev 2018-04-19 1495 int ldev_mtu = 0; c40e89fd35 Alexey Kodanev 2018-04-19 1496 c40e89fd35 Alexey Kodanev 2018-04-19 1497 if (tb[IFLA_MTU]) { c40e89fd35 Alexey Kodanev 2018-04-19 1498 geneve_change_mtu(dev, nla_get_u32(tb[IFLA_MTU])); c40e89fd35 Alexey Kodanev 2018-04-19 1499 return; c40e89fd35 Alexey Kodanev 2018-04-19 1500 } c40e89fd35 Alexey Kodanev 2018-04-19 1501 c40e89fd35 Alexey Kodanev 2018-04-19 1502 switch (ip_tunnel_info_af(info)) { c40e89fd35 Alexey Kodanev 2018-04-19 1503 case AF_INET: { c40e89fd35 Alexey Kodanev 2018-04-19 1504 struct flowi4 fl4 = { .daddr = info->key.u.ipv4.dst }; c40e89fd35 Alexey Kodanev 2018-04-19 1505 struct rtable *rt = ip_route_output_key(geneve->net, &fl4); c40e89fd35 Alexey Kodanev 2018-04-19 1506 c40e89fd35 Alexey Kodanev 2018-04-19 1507 if (!IS_ERR(rt) && rt->dst.dev) { c40e89fd35 Alexey Kodanev 2018-04-19 1508 ldev_mtu = rt->dst.dev->mtu - GENEVE_IPV4_HLEN; c40e89fd35 Alexey Kodanev 2018-04-19 1509 ip_rt_put(rt); c40e89fd35 Alexey Kodanev 2018-04-19 1510 } c40e89fd35 Alexey Kodanev 2018-04-19 1511 break; c40e89fd35 Alexey Kodanev 2018-04-19 1512 } c40e89fd35 Alexey Kodanev 2018-04-19 1513 #if IS_ENABLED(CONFIG_IPV6) c40e89fd35 Alexey Kodanev 2018-04-19 1514 case AF_INET6: { 55e942d018 Hangbin Liu 2019-02-06 1515 struct inet6_dev *idev = in6_dev_get(dev); 55e942d018 Hangbin Liu 2019-02-06 1516 if (!idev) 55e942d018 Hangbin Liu 2019-02-06 1517 break; 55e942d018 Hangbin Liu 2019-02-06 1518 c40e89fd35 Alexey Kodanev 2018-04-19 @1519 struct rt6_info *rt = rt6_lookup(geneve->net, c40e89fd35 Alexey Kodanev 2018-04-19 1520 &info->key.u.ipv6.dst, NULL, 0, c40e89fd35 Alexey Kodanev 2018-04-19 1521 NULL, 0); c40e89fd35 Alexey Kodanev 2018-04-19 1522 c40e89fd35 Alexey Kodanev 2018-04-19 1523 if (rt && rt->dst.dev) c40e89fd35 Alexey Kodanev 2018-04-19 1524 ldev_mtu = rt->dst.dev->mtu - GENEVE_IPV6_HLEN; c40e89fd35 Alexey Kodanev 2018-04-19 1525 ip6_rt_put(rt); 55e942d018 Hangbin Liu 2019-02-06 1526 55e942d018 Hangbin Liu 2019-02-06 1527 in6_dev_put(idev); c40e89fd35 Alexey Kodanev 2018-04-19 1528 break; c40e89fd35 Alexey Kodanev 2018-04-19 1529 } c40e89fd35 Alexey Kodanev 2018-04-19 1530 #endif c40e89fd35 Alexey Kodanev 2018-04-19 1531 } c40e89fd35 Alexey Kodanev 2018-04-19 1532 c40e89fd35 Alexey Kodanev 2018-04-19 1533 if (ldev_mtu <= 0) c40e89fd35 Alexey Kodanev 2018-04-19 1534 return; c40e89fd35 Alexey Kodanev 2018-04-19 1535 c40e89fd35 Alexey Kodanev 2018-04-19 1536 geneve_change_mtu(dev, ldev_mtu - info->options_len); c40e89fd35 Alexey Kodanev 2018-04-19 1537 } c40e89fd35 Alexey Kodanev 2018-04-19 1538 :::::: The code at line 1519 was first introduced by commit :::::: c40e89fd358e94a55d6c1475afbea17b5580f601 geneve: configure MTU based on a lower device :::::: TO: Alexey Kodanev <alexey.kodanev@oracle.com> :::::: CC: David S. Miller <davem@davemloft.net> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 12117 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net 2/2] sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() 2019-02-06 12:51 [PATCH net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu 2019-02-06 12:51 ` [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu @ 2019-02-06 12:51 ` Hangbin Liu 2019-02-06 15:00 ` Stefano Brivio 2019-02-06 16:45 ` Eric Dumazet 2019-02-07 10:36 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu 2 siblings, 2 replies; 16+ messages in thread From: Hangbin Liu @ 2019-02-06 12:51 UTC (permalink / raw) To: netdev; +Cc: Stefano Brivio, David Miller, Hangbin Liu If we disabled IPv6 from kernel boot up cmd(ipv6.disable=1), we should not call ip6_err_gen_icmpv6_unreach(). Reproducer: ip link add sit1 type sit local 10.10.0.1 remote 10.10.1.1 ttl 1 ip link set sit1 up ip addr add 192.168.0.1/24 dev sit1 ping 192.168.0.2 Reported-by: Jianlin Shi <jishi@redhat.com> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- net/ipv6/sit.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c index 1e03305c0549..e43fbac0fd1a 100644 --- a/net/ipv6/sit.c +++ b/net/ipv6/sit.c @@ -493,6 +493,7 @@ static int ipip6_err(struct sk_buff *skb, u32 info) const int type = icmp_hdr(skb)->type; const int code = icmp_hdr(skb)->code; unsigned int data_len = 0; + struct inet6_dev *idev; struct ip_tunnel *t; int sifindex; int err; @@ -546,8 +547,13 @@ static int ipip6_err(struct sk_buff *skb, u32 info) } err = 0; - if (!ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) + + idev = in6_dev_get(skb->dev); + if (idev && + !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) { + in6_dev_put(idev); goto out; + } if (t->parms.iph.daddr == 0) goto out; -- 2.19.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net 2/2] sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() 2019-02-06 12:51 ` [PATCH net 2/2] sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() Hangbin Liu @ 2019-02-06 15:00 ` Stefano Brivio 2019-02-06 16:45 ` Eric Dumazet 1 sibling, 0 replies; 16+ messages in thread From: Stefano Brivio @ 2019-02-06 15:00 UTC (permalink / raw) To: Hangbin Liu; +Cc: netdev, David Miller On Wed, 6 Feb 2019 20:51:11 +0800 Hangbin Liu <liuhangbin@gmail.com> wrote: > If we disabled IPv6 from kernel boot up cmd(ipv6.disable=1), we should not > call ip6_err_gen_icmpv6_unreach(). > > Reproducer: > ip link add sit1 type sit local 10.10.0.1 remote 10.10.1.1 ttl 1 > ip link set sit1 up > ip addr add 192.168.0.1/24 dev sit1 > ping 192.168.0.2 > > Reported-by: Jianlin Shi <jishi@redhat.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > net/ipv6/sit.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c > index 1e03305c0549..e43fbac0fd1a 100644 > --- a/net/ipv6/sit.c > +++ b/net/ipv6/sit.c > @@ -493,6 +493,7 @@ static int ipip6_err(struct sk_buff *skb, u32 info) > const int type = icmp_hdr(skb)->type; > const int code = icmp_hdr(skb)->code; > unsigned int data_len = 0; > + struct inet6_dev *idev; > struct ip_tunnel *t; > int sifindex; > int err; > @@ -546,8 +547,13 @@ static int ipip6_err(struct sk_buff *skb, u32 info) > } > > err = 0; > - if (!ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) > + > + idev = in6_dev_get(skb->dev); > + if (idev && > + !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) { > + in6_dev_put(idev); > goto out; > + } You are leaking a reference if idev && ip6_err_gen_icmpv6_unreach(...). You should call in6_dev_put(idev) whenever idev is not NULL instead. -- Stefano ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net 2/2] sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() 2019-02-06 12:51 ` [PATCH net 2/2] sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() Hangbin Liu 2019-02-06 15:00 ` Stefano Brivio @ 2019-02-06 16:45 ` Eric Dumazet 1 sibling, 0 replies; 16+ messages in thread From: Eric Dumazet @ 2019-02-06 16:45 UTC (permalink / raw) To: Hangbin Liu, netdev; +Cc: Stefano Brivio, David Miller On 02/06/2019 04:51 AM, Hangbin Liu wrote: > If we disabled IPv6 from kernel boot up cmd(ipv6.disable=1), we should not > call ip6_err_gen_icmpv6_unreach(). > > Reproducer: > ip link add sit1 type sit local 10.10.0.1 remote 10.10.1.1 ttl 1 > ip link set sit1 up > ip addr add 192.168.0.1/24 dev sit1 > ping 192.168.0.2 > > Reported-by: Jianlin Shi <jishi@redhat.com> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > net/ipv6/sit.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c > index 1e03305c0549..e43fbac0fd1a 100644 > --- a/net/ipv6/sit.c > +++ b/net/ipv6/sit.c > @@ -493,6 +493,7 @@ static int ipip6_err(struct sk_buff *skb, u32 info) > const int type = icmp_hdr(skb)->type; > const int code = icmp_hdr(skb)->code; > unsigned int data_len = 0; > + struct inet6_dev *idev; > struct ip_tunnel *t; > int sifindex; > int err; > @@ -546,8 +547,13 @@ static int ipip6_err(struct sk_buff *skb, u32 info) > } > > err = 0; > - if (!ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) > + > + idev = in6_dev_get(skb->dev); > + if (idev && > + !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) { > + in6_dev_put(idev); > goto out; > + } > It seems there is a missing in6_dev_put(idev) depending on ip6_err_gen_icmpv6_unreach()() return value ? ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up 2019-02-06 12:51 [PATCH net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu 2019-02-06 12:51 ` [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu 2019-02-06 12:51 ` [PATCH net 2/2] sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() Hangbin Liu @ 2019-02-07 10:36 ` Hangbin Liu 2019-02-07 10:36 ` [PATCH v2 net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu ` (2 more replies) 2 siblings, 3 replies; 16+ messages in thread From: Hangbin Liu @ 2019-02-07 10:36 UTC (permalink / raw) To: netdev; +Cc: Stefano Brivio, David Miller, Eric Dumazet, Hangbin Liu When disabled IPv6 on boot up, since there is no ipv6 route tables, we should not call rt6_lookup. Fix them by checking if we have inet6_dev pointer on netdevice. v2: Fix idev reference leak, declarations and code mixing as Stefano, Eric pointed. Since we only want to check if idev exists and not reference it, use __in6_dev_get() insteand of in6_dev_get(). Hangbin Liu (2): geneve: should not call rt6_lookup() when ipv6 was disabled sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() drivers/net/geneve.c | 6 ++++++ net/ipv6/sit.c | 8 +++++++- 2 files changed, 13 insertions(+), 1 deletion(-) -- 2.19.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled 2019-02-07 10:36 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu @ 2019-02-07 10:36 ` Hangbin Liu 2019-02-07 10:36 ` [PATCH v2 net 2/2] sit: check if IPv6 enabled before calling ip6_err_gen_icmpv6_unreach() Hangbin Liu 2019-02-07 18:48 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up David Miller 2 siblings, 0 replies; 16+ messages in thread From: Hangbin Liu @ 2019-02-07 10:36 UTC (permalink / raw) To: netdev Cc: Stefano Brivio, David Miller, Eric Dumazet, Hangbin Liu, Alexey Kodanev When we add a new GENEVE device with IPv6 remote, checking only for IS_ENABLED(CONFIG_IPV6) is not enough as we may disable IPv6 in the kernel command line (ipv6.disable=1), and calling rt6_lookup() would cause a NULL pointer dereference. v2: - don't mix declarations and code (reported by Stefano Brivio, Eric Dumazet) - there's no need to use in6_dev_get() as we only need to check that idev exists (reported by David Ahern). This is under RTNL, so we can simply use __in6_dev_get() instead (Stefano, Eric). Reported-by: Jianlin Shi <jishi@redhat.com> Fixes: c40e89fd358e9 ("geneve: configure MTU based on a lower device") Cc: Alexey Kodanev <alexey.kodanev@oracle.com> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> Reviewed-by: Stefano Brivio <sbrivio@redhat.com> --- drivers/net/geneve.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index 58bbba8582b0..3377ac66a347 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -1512,9 +1512,13 @@ static void geneve_link_config(struct net_device *dev, } #if IS_ENABLED(CONFIG_IPV6) case AF_INET6: { - struct rt6_info *rt = rt6_lookup(geneve->net, - &info->key.u.ipv6.dst, NULL, 0, - NULL, 0); + struct rt6_info *rt; + + if (!__in6_dev_get(dev)) + break; + + rt = rt6_lookup(geneve->net, &info->key.u.ipv6.dst, NULL, 0, + NULL, 0); if (rt && rt->dst.dev) ldev_mtu = rt->dst.dev->mtu - GENEVE_IPV6_HLEN; -- 2.19.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 net 2/2] sit: check if IPv6 enabled before calling ip6_err_gen_icmpv6_unreach() 2019-02-07 10:36 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu 2019-02-07 10:36 ` [PATCH v2 net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu @ 2019-02-07 10:36 ` Hangbin Liu 2019-02-07 18:48 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up David Miller 2 siblings, 0 replies; 16+ messages in thread From: Hangbin Liu @ 2019-02-07 10:36 UTC (permalink / raw) To: netdev Cc: Stefano Brivio, David Miller, Eric Dumazet, Hangbin Liu, Oussama Ghorbel If we disabled IPv6 from the kernel command line (ipv6.disable=1), we should not call ip6_err_gen_icmpv6_unreach(). This: ip link add sit1 type sit local 192.0.2.1 remote 192.0.2.2 ttl 1 ip link set sit1 up ip addr add 198.51.100.1/24 dev sit1 ping 198.51.100.2 if IPv6 is disabled at boot time, will crash the kernel. v2: there's no need to use in6_dev_get(), use __in6_dev_get() instead, as we only need to check that idev exists and we are under rcu_read_lock() (from netif_receive_skb_internal()). Reported-by: Jianlin Shi <jishi@redhat.com> Fixes: ca15a078bd90 ("sit: generate icmpv6 error when receiving icmpv4 error") Cc: Oussama Ghorbel <ghorbel@pivasoftware.com> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> Reviewed-by: Stefano Brivio <sbrivio@redhat.com> --- net/ipv6/sit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c index 1e03305c0549..e8a1dabef803 100644 --- a/net/ipv6/sit.c +++ b/net/ipv6/sit.c @@ -546,7 +546,8 @@ static int ipip6_err(struct sk_buff *skb, u32 info) } err = 0; - if (!ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) + if (__in6_dev_get(skb->dev) && + !ip6_err_gen_icmpv6_unreach(skb, iph->ihl * 4, type, data_len)) goto out; if (t->parms.iph.daddr == 0) -- 2.19.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up 2019-02-07 10:36 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu 2019-02-07 10:36 ` [PATCH v2 net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu 2019-02-07 10:36 ` [PATCH v2 net 2/2] sit: check if IPv6 enabled before calling ip6_err_gen_icmpv6_unreach() Hangbin Liu @ 2019-02-07 18:48 ` David Miller 2 siblings, 0 replies; 16+ messages in thread From: David Miller @ 2019-02-07 18:48 UTC (permalink / raw) To: liuhangbin; +Cc: netdev, sbrivio, eric.dumazet From: Hangbin Liu <liuhangbin@gmail.com> Date: Thu, 7 Feb 2019 18:36:09 +0800 > When disabled IPv6 on boot up, since there is no ipv6 route tables, we should > not call rt6_lookup. Fix them by checking if we have inet6_dev pointer on > netdevice. > > v2: Fix idev reference leak, declarations and code mixing as Stefano, > Eric pointed. Since we only want to check if idev exists and not > reference it, use __in6_dev_get() insteand of in6_dev_get(). Series applied and queued up for -stable, thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-02-07 18:49 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-02-06 12:51 [PATCH net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu 2019-02-06 12:51 ` [PATCH net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu 2019-02-06 14:58 ` Stefano Brivio 2019-02-06 16:43 ` Eric Dumazet 2019-02-06 18:54 ` David Ahern 2019-02-06 19:04 ` Stefano Brivio 2019-02-06 19:09 ` Eric Dumazet 2019-02-06 19:08 ` Eric Dumazet 2019-02-07 0:55 ` kbuild test robot 2019-02-06 12:51 ` [PATCH net 2/2] sit: check if IPv6 enabled before call ip6_err_gen_icmpv6_unreach() Hangbin Liu 2019-02-06 15:00 ` Stefano Brivio 2019-02-06 16:45 ` Eric Dumazet 2019-02-07 10:36 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up Hangbin Liu 2019-02-07 10:36 ` [PATCH v2 net 1/2] geneve: should not call rt6_lookup() when ipv6 was disabled Hangbin Liu 2019-02-07 10:36 ` [PATCH v2 net 2/2] sit: check if IPv6 enabled before calling ip6_err_gen_icmpv6_unreach() Hangbin Liu 2019-02-07 18:48 ` [PATCH v2 net 0/2] fix two kernel panics when disabled IPv6 on boot up David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.