* [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb
@ 2021-04-21 23:11 Phillip Potter
2021-04-22 0:39 ` Florian Westphal
2021-04-22 6:43 ` Eric Dumazet
0 siblings, 2 replies; 9+ messages in thread
From: Phillip Potter @ 2021-04-21 23:11 UTC (permalink / raw)
To: davem; +Cc: kuba, netdev, linux-kernel, edumazet
Modify the check in geneve6_xmit_skb to use the size of a struct iphdr
rather than struct ipv6hdr. This fixes two kernel selftest failures
introduced by commit 6628ddfec758
("net: geneve: check skb is large enough for IPv4/IPv6 header"), without
diminishing the fix provided by that commit.
Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
drivers/net/geneve.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 42f31c681846..a57a5e6f614f 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -988,7 +988,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
__be16 sport;
int err;
- if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
+ if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
return -EINVAL;
sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb 2021-04-21 23:11 [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb Phillip Potter @ 2021-04-22 0:39 ` Florian Westphal 2021-04-22 0:56 ` Phillip Potter 2021-04-22 16:03 ` Sabrina Dubroca 2021-04-22 6:43 ` Eric Dumazet 1 sibling, 2 replies; 9+ messages in thread From: Florian Westphal @ 2021-04-22 0:39 UTC (permalink / raw) To: Phillip Potter; +Cc: davem, kuba, netdev, linux-kernel, edumazet Phillip Potter <phil@philpotter.co.uk> wrote: > Modify the check in geneve6_xmit_skb to use the size of a struct iphdr > rather than struct ipv6hdr. This fixes two kernel selftest failures > introduced by commit 6628ddfec758 > ("net: geneve: check skb is large enough for IPv4/IPv6 header"), without > diminishing the fix provided by that commit. What errors? > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > --- > drivers/net/geneve.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index 42f31c681846..a57a5e6f614f 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -988,7 +988,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, > __be16 sport; > int err; > > - if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) > return -EINVAL; Seems this is papering over some bug, this change makes no sense to me. Can you please explain this? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb 2021-04-22 0:39 ` Florian Westphal @ 2021-04-22 0:56 ` Phillip Potter 2021-04-22 16:03 ` Sabrina Dubroca 1 sibling, 0 replies; 9+ messages in thread From: Phillip Potter @ 2021-04-22 0:56 UTC (permalink / raw) To: Florian Westphal; +Cc: davem, kuba, netdev, linux-kernel, edumazet On Thu, Apr 22, 2021 at 02:39:42AM +0200, Florian Westphal wrote: > Phillip Potter <phil@philpotter.co.uk> wrote: > > Modify the check in geneve6_xmit_skb to use the size of a struct iphdr > > rather than struct ipv6hdr. This fixes two kernel selftest failures > > introduced by commit 6628ddfec758 > > ("net: geneve: check skb is large enough for IPv4/IPv6 header"), without > > diminishing the fix provided by that commit. > > What errors? > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > --- > > drivers/net/geneve.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > index 42f31c681846..a57a5e6f614f 100644 > > --- a/drivers/net/geneve.c > > +++ b/drivers/net/geneve.c > > @@ -988,7 +988,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, > > __be16 sport; > > int err; > > > > - if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) > > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) > > return -EINVAL; > > Seems this is papering over some bug, this change makes no sense to > me. Can you please explain this? Dear Florian, I made the change to fix failures in the following tests: IPv4 over geneve6: PMTU exceptions IPv4 over geneve6: PMTU exceptions - nexthop objects Error for both tests was: PMTU exception wasn't created after exceeding link layer MTU on geneve interface I was notified by the kernel test reobot due to the failures being caused by my previous patch to this file. Sorry if I've done this the wrong way, just didn't want to hold anyone up. I also tested the patch with syzbot again to make sure it still fixed the original problem from my last commit. Regards, Phil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb 2021-04-22 0:39 ` Florian Westphal 2021-04-22 0:56 ` Phillip Potter @ 2021-04-22 16:03 ` Sabrina Dubroca 2021-04-22 16:52 ` Eric Dumazet 1 sibling, 1 reply; 9+ messages in thread From: Sabrina Dubroca @ 2021-04-22 16:03 UTC (permalink / raw) To: Florian Westphal Cc: Phillip Potter, davem, kuba, netdev, linux-kernel, edumazet 2021-04-22, 02:39:42 +0200, Florian Westphal wrote: > Phillip Potter <phil@philpotter.co.uk> wrote: > > Modify the check in geneve6_xmit_skb to use the size of a struct iphdr > > rather than struct ipv6hdr. This fixes two kernel selftest failures > > introduced by commit 6628ddfec758 > > ("net: geneve: check skb is large enough for IPv4/IPv6 header"), without > > diminishing the fix provided by that commit. > > What errors? > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > --- > > drivers/net/geneve.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > index 42f31c681846..a57a5e6f614f 100644 > > --- a/drivers/net/geneve.c > > +++ b/drivers/net/geneve.c > > @@ -988,7 +988,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, > > __be16 sport; > > int err; > > > > - if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) > > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) > > return -EINVAL; > > Seems this is papering over some bug, this change makes no sense to > me. Can you please explain this? I'm not sure the original commit (6628ddfec758 ("net: geneve: check skb is large enough for IPv4/IPv6 header")) is correct either. GENEVE isn't limited to carrying IP, I think an ethernet header with not much else on top should be valid. -- Sabrina ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb 2021-04-22 16:03 ` Sabrina Dubroca @ 2021-04-22 16:52 ` Eric Dumazet 2021-04-22 17:18 ` Sabrina Dubroca 0 siblings, 1 reply; 9+ messages in thread From: Eric Dumazet @ 2021-04-22 16:52 UTC (permalink / raw) To: Sabrina Dubroca Cc: Florian Westphal, Phillip Potter, David Miller, Jakub Kicinski, netdev, LKML On Thu, Apr 22, 2021 at 6:04 PM Sabrina Dubroca <sd@queasysnail.net> wrote: > > 2021-04-22, 02:39:42 +0200, Florian Westphal wrote: > > Phillip Potter <phil@philpotter.co.uk> wrote: > > > Modify the check in geneve6_xmit_skb to use the size of a struct iphdr > > > rather than struct ipv6hdr. This fixes two kernel selftest failures > > > introduced by commit 6628ddfec758 > > > ("net: geneve: check skb is large enough for IPv4/IPv6 header"), without > > > diminishing the fix provided by that commit. > > > > What errors? > > > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > > --- > > > drivers/net/geneve.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > > index 42f31c681846..a57a5e6f614f 100644 > > > --- a/drivers/net/geneve.c > > > +++ b/drivers/net/geneve.c > > > @@ -988,7 +988,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, > > > __be16 sport; > > > int err; > > > > > > - if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) > > > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) > > > return -EINVAL; > > > > Seems this is papering over some bug, this change makes no sense to > > me. Can you please explain this? > > I'm not sure the original commit (6628ddfec758 ("net: geneve: check > skb is large enough for IPv4/IPv6 header")) is correct either. GENEVE > isn't limited to carrying IP, I think an ethernet header with not much > else on top should be valid. Maybe, but we still attempt to use ip_hdr() in this case, from geneve_get_v6_dst() So there is something fishy. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb 2021-04-22 16:52 ` Eric Dumazet @ 2021-04-22 17:18 ` Sabrina Dubroca 2021-04-22 21:03 ` Phillip Potter 0 siblings, 1 reply; 9+ messages in thread From: Sabrina Dubroca @ 2021-04-22 17:18 UTC (permalink / raw) To: Eric Dumazet Cc: Florian Westphal, Phillip Potter, David Miller, Jakub Kicinski, netdev, LKML 2021-04-22, 18:52:10 +0200, Eric Dumazet wrote: > On Thu, Apr 22, 2021 at 6:04 PM Sabrina Dubroca <sd@queasysnail.net> wrote: > > > > 2021-04-22, 02:39:42 +0200, Florian Westphal wrote: > > > Phillip Potter <phil@philpotter.co.uk> wrote: > > > > Modify the check in geneve6_xmit_skb to use the size of a struct iphdr > > > > rather than struct ipv6hdr. This fixes two kernel selftest failures > > > > introduced by commit 6628ddfec758 > > > > ("net: geneve: check skb is large enough for IPv4/IPv6 header"), without > > > > diminishing the fix provided by that commit. > > > > > > What errors? > > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > > > --- > > > > drivers/net/geneve.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > > > index 42f31c681846..a57a5e6f614f 100644 > > > > --- a/drivers/net/geneve.c > > > > +++ b/drivers/net/geneve.c > > > > @@ -988,7 +988,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, > > > > __be16 sport; > > > > int err; > > > > > > > > - if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) > > > > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) > > > > return -EINVAL; > > > > > > Seems this is papering over some bug, this change makes no sense to > > > me. Can you please explain this? > > > > I'm not sure the original commit (6628ddfec758 ("net: geneve: check > > skb is large enough for IPv4/IPv6 header")) is correct either. GENEVE > > isn't limited to carrying IP, I think an ethernet header with not much > > else on top should be valid. > > Maybe, but we still attempt to use ip_hdr() in this case, from > geneve_get_v6_dst() > > So there is something fishy. In ip_tunnel_get_dsfield()? Only if there's IP in the packet. Other tunnel types (except vxlan, which probably has the same problem as geneve) ues pskb_inet_may_pull, that looks like what we need here as well. -- Sabrina ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb 2021-04-22 17:18 ` Sabrina Dubroca @ 2021-04-22 21:03 ` Phillip Potter 0 siblings, 0 replies; 9+ messages in thread From: Phillip Potter @ 2021-04-22 21:03 UTC (permalink / raw) To: Sabrina Dubroca Cc: Florian Westphal, Eric Dumazet, David Miller, Jakub Kicinski, netdev, LKML On Thu, Apr 22, 2021 at 07:18:49PM +0200, Sabrina Dubroca wrote: > 2021-04-22, 18:52:10 +0200, Eric Dumazet wrote: > > On Thu, Apr 22, 2021 at 6:04 PM Sabrina Dubroca <sd@queasysnail.net> wrote: > > > > > > 2021-04-22, 02:39:42 +0200, Florian Westphal wrote: > > > > Phillip Potter <phil@philpotter.co.uk> wrote: > > > > > Modify the check in geneve6_xmit_skb to use the size of a struct iphdr > > > > > rather than struct ipv6hdr. This fixes two kernel selftest failures > > > > > introduced by commit 6628ddfec758 > > > > > ("net: geneve: check skb is large enough for IPv4/IPv6 header"), without > > > > > diminishing the fix provided by that commit. > > > > > > > > What errors? > > > > > > > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > > > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > > > > --- > > > > > drivers/net/geneve.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > > > > > index 42f31c681846..a57a5e6f614f 100644 > > > > > --- a/drivers/net/geneve.c > > > > > +++ b/drivers/net/geneve.c > > > > > @@ -988,7 +988,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, > > > > > __be16 sport; > > > > > int err; > > > > > > > > > > - if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) > > > > > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) > > > > > return -EINVAL; > > > > > > > > Seems this is papering over some bug, this change makes no sense to > > > > me. Can you please explain this? > > > > > > I'm not sure the original commit (6628ddfec758 ("net: geneve: check > > > skb is large enough for IPv4/IPv6 header")) is correct either. GENEVE > > > isn't limited to carrying IP, I think an ethernet header with not much > > > else on top should be valid. > > > > Maybe, but we still attempt to use ip_hdr() in this case, from > > geneve_get_v6_dst() > > > > So there is something fishy. > > In ip_tunnel_get_dsfield()? Only if there's IP in the packet. Other > tunnel types (except vxlan, which probably has the same problem as > geneve) ues pskb_inet_may_pull, that looks like what we need here as > well. > > -- > Sabrina > Dear Sabrina, Thank you for your feedback. I will try and rework the patch to fix my original commit using the technique you suggest. Regards, Phil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb 2021-04-21 23:11 [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb Phillip Potter 2021-04-22 0:39 ` Florian Westphal @ 2021-04-22 6:43 ` Eric Dumazet 2021-04-22 20:58 ` Phillip Potter 1 sibling, 1 reply; 9+ messages in thread From: Eric Dumazet @ 2021-04-22 6:43 UTC (permalink / raw) To: Phillip Potter; +Cc: David Miller, Jakub Kicinski, netdev, LKML On Thu, Apr 22, 2021 at 1:11 AM Phillip Potter <phil@philpotter.co.uk> wrote: > > Modify the check in geneve6_xmit_skb to use the size of a struct iphdr > rather than struct ipv6hdr. This fixes two kernel selftest failures > introduced by commit 6628ddfec758 > ("net: geneve: check skb is large enough for IPv4/IPv6 header"), without > diminishing the fix provided by that commit. > > Reported-by: kernel test robot <oliver.sang@intel.com> > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> SGTM, but _please_ add a Fixes: tag, since stable teams now depend on this, more than human parsing the changelog. Fixes: 6628ddfec758 ("net: geneve: check skb is large enough for IPv4/IPv6 header") > --- > drivers/net/geneve.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index 42f31c681846..a57a5e6f614f 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -988,7 +988,7 @@ static int geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev, > __be16 sport; > int err; > > - if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr))) > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr))) > return -EINVAL; > > sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true); > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb 2021-04-22 6:43 ` Eric Dumazet @ 2021-04-22 20:58 ` Phillip Potter 0 siblings, 0 replies; 9+ messages in thread From: Phillip Potter @ 2021-04-22 20:58 UTC (permalink / raw) To: Eric Dumazet Cc: Florian Westphal, Sabrina Dubroca, David Miller, Jakub Kicinski, netdev, LKML On Thu, Apr 22, 2021 at 08:43:57AM +0200, Eric Dumazet wrote: > On Thu, Apr 22, 2021 at 1:11 AM Phillip Potter <phil@philpotter.co.uk> wrote: > > > > Modify the check in geneve6_xmit_skb to use the size of a struct iphdr > > rather than struct ipv6hdr. This fixes two kernel selftest failures > > introduced by commit 6628ddfec758 > > ("net: geneve: check skb is large enough for IPv4/IPv6 header"), without > > diminishing the fix provided by that commit. > > > > Reported-by: kernel test robot <oliver.sang@intel.com> > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > SGTM, but _please_ add a Fixes: tag, since stable teams now depend on this, > more than human parsing the changelog. > > Fixes: 6628ddfec758 ("net: geneve: check skb is large enough for > IPv4/IPv6 header") > Dear Eric, Thank you for your feedback, I will make sure the new patch has the Fixes: tag in it. Regards, Phil ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-04-22 21:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-21 23:11 [PATCH] net: geneve: modify IP header check in geneve6_xmit_skb Phillip Potter 2021-04-22 0:39 ` Florian Westphal 2021-04-22 0:56 ` Phillip Potter 2021-04-22 16:03 ` Sabrina Dubroca 2021-04-22 16:52 ` Eric Dumazet 2021-04-22 17:18 ` Sabrina Dubroca 2021-04-22 21:03 ` Phillip Potter 2021-04-22 6:43 ` Eric Dumazet 2021-04-22 20:58 ` Phillip Potter
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.