| Message ID | 20160512195003.26694.17950.stgit@localhost.localdomain |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2016-05-12 at 12:51 -0700, Alexander Duyck wrote: > While testing an OpenStack configuration using VXLANs I saw the following > call trace: > The following trace is seen when receiving a DHCP request over a flow-based > VXLAN tunnel. I believe this is caused by the metadata dst having a NULL > dev value and as a result dev_net(dev) is causing a NULL pointer dereference. > > To resolve this I am replacing the check for skb_dst() with skb_valid_dst() > so that we do not attempt to use the metadata dst to retrieve a device in > order to determine the network namespace. > > Fixes: 63058308cd55 ("udp: Add udp6_lib_lookup_skb and udp4_lib_lookup_skb") > Signed-off-by: Alexander Duyck <aduyck@mirantis.com> > --- > net/ipv4/udp.c | 3 ++- > net/ipv6/udp.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index f67f52ba4809..69aa7ab81933 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -114,6 +114,7 @@ > #include <net/busy_poll.h> > #include "udp_impl.h" > #include <net/sock_reuseport.h> > +#include <net/dst_metadata.h> > > struct udp_table udp_table __read_mostly; > EXPORT_SYMBOL(udp_table); > @@ -614,7 +615,7 @@ struct sock *udp4_lib_lookup_skb(struct sk_buff *skb, > { > const struct iphdr *iph = ip_hdr(skb); > const struct net_device *dev = > - skb_dst(skb) ? skb_dst(skb)->dev : skb->dev; > + skb_valid_dst(skb) ? skb_dst(skb)->dev : skb->dev; Looks overly complicated to me. If this is called from GRO, why don't we simply use skb->dev ?
On Thu, May 12, 2016 at 12:51 PM, Alexander Duyck <aduyck@mirantis.com> wrote: > The following trace is seen when receiving a DHCP request over a flow-based > VXLAN tunnel. I believe this is caused by the metadata dst having a NULL > dev value and as a result dev_net(dev) is causing a NULL pointer dereference. > > To resolve this I am replacing the check for skb_dst() with skb_valid_dst() > so that we do not attempt to use the metadata dst to retrieve a device in > order to determine the network namespace. Why does UDP layer need to care about tunnel layer things? If the problem is really what you describe, then the tunnel layer should reset that dst after finish.
On Thu, May 12, 2016 at 1:02 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Thu, May 12, 2016 at 12:51 PM, Alexander Duyck <aduyck@mirantis.com> wrote: >> The following trace is seen when receiving a DHCP request over a flow-based >> VXLAN tunnel. I believe this is caused by the metadata dst having a NULL >> dev value and as a result dev_net(dev) is causing a NULL pointer dereference. >> >> To resolve this I am replacing the check for skb_dst() with skb_valid_dst() >> so that we do not attempt to use the metadata dst to retrieve a device in >> order to determine the network namespace. > > Why does UDP layer need to care about tunnel layer things? > > If the problem is really what you describe, then the tunnel layer > should reset that dst after finish. A metadata dst is inserted by the tunnel layer after it has released the regular dst that routed the packet to the tunnel. My understanding is it is used by OVS to handle the routing of the packet after it has come out of the tunnel but before it has gone up through the IP stack. All of this was introduced back around the time of the lightweight tunnels. - Alex
On Thu, May 12, 2016 at 12:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Thu, 2016-05-12 at 12:51 -0700, Alexander Duyck wrote: >> While testing an OpenStack configuration using VXLANs I saw the following >> call trace: > >> The following trace is seen when receiving a DHCP request over a flow-based >> VXLAN tunnel. I believe this is caused by the metadata dst having a NULL >> dev value and as a result dev_net(dev) is causing a NULL pointer dereference. >> >> To resolve this I am replacing the check for skb_dst() with skb_valid_dst() >> so that we do not attempt to use the metadata dst to retrieve a device in >> order to determine the network namespace. >> >> Fixes: 63058308cd55 ("udp: Add udp6_lib_lookup_skb and udp4_lib_lookup_skb") >> Signed-off-by: Alexander Duyck <aduyck@mirantis.com> >> --- >> net/ipv4/udp.c | 3 ++- >> net/ipv6/udp.c | 3 ++- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c >> index f67f52ba4809..69aa7ab81933 100644 >> --- a/net/ipv4/udp.c >> +++ b/net/ipv4/udp.c >> @@ -114,6 +114,7 @@ >> #include <net/busy_poll.h> >> #include "udp_impl.h" >> #include <net/sock_reuseport.h> >> +#include <net/dst_metadata.h> >> >> struct udp_table udp_table __read_mostly; >> EXPORT_SYMBOL(udp_table); >> @@ -614,7 +615,7 @@ struct sock *udp4_lib_lookup_skb(struct sk_buff *skb, >> { >> const struct iphdr *iph = ip_hdr(skb); >> const struct net_device *dev = >> - skb_dst(skb) ? skb_dst(skb)->dev : skb->dev; >> + skb_valid_dst(skb) ? skb_dst(skb)->dev : skb->dev; > > Looks overly complicated to me. > > If this is called from GRO, why don't we simply use skb->dev ? I'm assuming this was using skb_dst(skb)->dev in order to allow for use of this function by other callers since the original function __udp4_lib_lookup_skb was using that. If we change this then it reduces the likelihood of the code being reusable by other callers. In such a case I would probably want to go through and also rename the functions to make sure they are tagged as being GRO specific. - Alex
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index f67f52ba4809..69aa7ab81933 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -114,6 +114,7 @@ #include <net/busy_poll.h> #include "udp_impl.h" #include <net/sock_reuseport.h> +#include <net/dst_metadata.h> struct udp_table udp_table __read_mostly; EXPORT_SYMBOL(udp_table); @@ -614,7 +615,7 @@ struct sock *udp4_lib_lookup_skb(struct sk_buff *skb, { const struct iphdr *iph = ip_hdr(skb); const struct net_device *dev = - skb_dst(skb) ? skb_dst(skb)->dev : skb->dev; + skb_valid_dst(skb) ? skb_dst(skb)->dev : skb->dev; return __udp4_lib_lookup(dev_net(dev), iph->saddr, sport, iph->daddr, dport, inet_iif(skb), diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index aca06094110f..12c06deda5ef 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -49,6 +49,7 @@ #include <net/inet6_hashtables.h> #include <net/busy_poll.h> #include <net/sock_reuseport.h> +#include <net/dst_metadata.h> #include <linux/proc_fs.h> #include <linux/seq_file.h> @@ -331,7 +332,7 @@ struct sock *udp6_lib_lookup_skb(struct sk_buff *skb, { const struct ipv6hdr *iph = ipv6_hdr(skb); const struct net_device *dev = - skb_dst(skb) ? skb_dst(skb)->dev : skb->dev; + skb_valid_dst(skb) ? skb_dst(skb)->dev : skb->dev; return __udp6_lib_lookup(dev_net(dev), &iph->saddr, sport, &iph->daddr, dport, inet6_iif(skb),
While testing an OpenStack configuration using VXLANs I saw the following call trace: RIP: 0010:[<ffffffff815fad49>] udp4_lib_lookup_skb+0x49/0x80 RSP: 0018:ffff88103867bc50 EFLAGS: 00010286 RAX: ffff88103269bf00 RBX: ffff88103269bf00 RCX: 00000000ffffffff RDX: 0000000000004300 RSI: 0000000000000000 RDI: ffff880f2932e780 RBP: ffff88103867bc60 R08: 0000000000000000 R09: 000000009001a8c0 R10: 0000000000004400 R11: ffffffff81333a58 R12: ffff880f2932e794 R13: 0000000000000014 R14: 0000000000000014 R15: ffffe8efbfd89ca0 FS: 0000000000000000(0000) GS:ffff88103fd80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000488 CR3: 0000000001c06000 CR4: 00000000001426e0 Stack: ffffffff81576515 ffffffff815733c0 ffff88103867bc98 ffffffff815fcc17 ffff88103269bf00 ffffe8efbfd89ca0 0000000000000014 0000000000000080 ffffe8efbfd89ca0 ffff88103867bcc8 ffffffff815fcf8b ffff880f2932e794 Call Trace: [<ffffffff81576515>] ? skb_checksum+0x35/0x50 [<ffffffff815733c0>] ? skb_push+0x40/0x40 [<ffffffff815fcc17>] udp_gro_receive+0x57/0x130 [<ffffffff815fcf8b>] udp4_gro_receive+0x10b/0x2c0 [<ffffffff81605863>] inet_gro_receive+0x1d3/0x270 [<ffffffff81589e59>] dev_gro_receive+0x269/0x3b0 [<ffffffff8158a1b8>] napi_gro_receive+0x38/0x120 [<ffffffffa0871297>] gro_cell_poll+0x57/0x80 [vxlan] [<ffffffff815899d0>] net_rx_action+0x160/0x380 [<ffffffff816965c7>] __do_softirq+0xd7/0x2c5 [<ffffffff8107d969>] run_ksoftirqd+0x29/0x50 [<ffffffff8109a50f>] smpboot_thread_fn+0x10f/0x160 [<ffffffff8109a400>] ? sort_range+0x30/0x30 [<ffffffff81096da8>] kthread+0xd8/0xf0 [<ffffffff81693c82>] ret_from_fork+0x22/0x40 [<ffffffff81096cd0>] ? kthread_park+0x60/0x60 The following trace is seen when receiving a DHCP request over a flow-based VXLAN tunnel. I believe this is caused by the metadata dst having a NULL dev value and as a result dev_net(dev) is causing a NULL pointer dereference. To resolve this I am replacing the check for skb_dst() with skb_valid_dst() so that we do not attempt to use the metadata dst to retrieve a device in order to determine the network namespace. Fixes: 63058308cd55 ("udp: Add udp6_lib_lookup_skb and udp4_lib_lookup_skb") Signed-off-by: Alexander Duyck <aduyck@mirantis.com> --- net/ipv4/udp.c | 3 ++- net/ipv6/udp.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)