* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).