| Message ID | 1507162445-18540-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] ip_gre: check packet length and mtu correctly in erspan_fb_xmit | expand |
From: William Tu > Sent: 05 October 2017 01:14 > Similarly to early patch for erspan_xmit(), the ARPHDR_ETHER device > is the length of the whole ether packet. So skb->len should subtract > the dev->hard_header_len. > > Fixes: 1a66a836da63 ("gre: add collect_md mode to ERSPAN tunnel") > Signed-off-by: William Tu <u9012063@gmail.com> > Cc: Xin Long <lucien.xin@gmail.com> > --- > net/ipv4/ip_gre.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > index b279c325c7f6..10b21fe5b3a6 100644 > --- a/net/ipv4/ip_gre.c > +++ b/net/ipv4/ip_gre.c > @@ -579,7 +579,7 @@ static void erspan_fb_xmit(struct sk_buff *skb, struct net_device *dev, > if (gre_handle_offloads(skb, false)) > goto err_free_rt; > > - if (skb->len > dev->mtu) { > + if (skb->len - dev->hard_header_len > dev->mtu) { Can you guarantee that skb->len > dev_hard_header_len? It is probably safer to check skb->len > dev->hard_header_len + dev->mtu since that addition isn't going to overflow. > pskb_trim(skb, dev->mtu); > truncate = true; Is that pskb_trim() now truncating to the correct size? David
On Thu, Oct 5, 2017 at 6:59 AM, David Laight <David.Laight@aculab.com> wrote: > From: William Tu >> Sent: 05 October 2017 01:14 >> Similarly to early patch for erspan_xmit(), the ARPHDR_ETHER device >> is the length of the whole ether packet. So skb->len should subtract >> the dev->hard_header_len. >> >> Fixes: 1a66a836da63 ("gre: add collect_md mode to ERSPAN tunnel") >> Signed-off-by: William Tu <u9012063@gmail.com> >> Cc: Xin Long <lucien.xin@gmail.com> >> --- >> net/ipv4/ip_gre.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >> index b279c325c7f6..10b21fe5b3a6 100644 >> --- a/net/ipv4/ip_gre.c >> +++ b/net/ipv4/ip_gre.c >> @@ -579,7 +579,7 @@ static void erspan_fb_xmit(struct sk_buff *skb, struct net_device *dev, >> if (gre_handle_offloads(skb, false)) >> goto err_free_rt; >> >> - if (skb->len > dev->mtu) { >> + if (skb->len - dev->hard_header_len > dev->mtu) { > > Can you guarantee that skb->len > dev_hard_header_len? > It is probably safer to check skb->len > dev->hard_header_len + dev->mtu > since that addition isn't going to overflow. Sure, I will fix it. > >> pskb_trim(skb, dev->mtu); >> truncate = true; > > Is that pskb_trim() now truncating to the correct size? You're right, now I should truncate to (dev->mtu + dev_hard_header_len) Thanks William
From: William Tu > Sent: 05 October 2017 22:21 ... > >> - if (skb->len > dev->mtu) { > >> + if (skb->len - dev->hard_header_len > dev->mtu) { > > > > Can you guarantee that skb->len > dev_hard_header_len? > > It is probably safer to check skb->len > dev->hard_header_len + dev->mtu > > since that addition isn't going to overflow. > Sure, I will fix it. > > > > >> pskb_trim(skb, dev->mtu); > >> truncate = true; > > > > Is that pskb_trim() now truncating to the correct size? > > You're right, now I should truncate to (dev->mtu + dev_hard_header_len) It might be worth caching that length in the dev structure to avoid the arithmetic on every packet. David
On Fri, Oct 6, 2017 at 2:38 AM, David Laight <David.Laight@aculab.com> wrote: > From: William Tu >> Sent: 05 October 2017 22:21 > ... >> >> - if (skb->len > dev->mtu) { >> >> + if (skb->len - dev->hard_header_len > dev->mtu) { >> > >> > Can you guarantee that skb->len > dev_hard_header_len? >> > It is probably safer to check skb->len > dev->hard_header_len + dev->mtu >> > since that addition isn't going to overflow. >> Sure, I will fix it. >> >> > >> >> pskb_trim(skb, dev->mtu); >> >> truncate = true; >> > >> > Is that pskb_trim() now truncating to the correct size? >> >> You're right, now I should truncate to (dev->mtu + dev_hard_header_len) > > It might be worth caching that length in the dev structure > to avoid the arithmetic on every packet. > > David > Thanks for the advise. Yes, adding another field in the struct net_device can avoid the arithmetic operation. I'm not sure it's a good idea to add new field since there are already a lot in net_device. Let's wait to see how others think. William
From: William Tu <u9012063@gmail.com> Date: Fri, 6 Oct 2017 15:09:29 -0700 > Yes, adding another field in the struct net_device can avoid the > arithmetic operation. I'm not sure it's a good idea to add new field > since there are already a lot in net_device. Let's wait to see how > others think. Why would you add it to net_device when you can add it to ip_tunnel?
On Fri, Oct 6, 2017 at 3:22 PM, David Miller <davem@davemloft.net> wrote: > From: William Tu <u9012063@gmail.com> > Date: Fri, 6 Oct 2017 15:09:29 -0700 > >> Yes, adding another field in the struct net_device can avoid the >> arithmetic operation. I'm not sure it's a good idea to add new field >> since there are already a lot in net_device. Let's wait to see how >> others think. > > Why would you add it to net_device when you can add it to ip_tunnel? > I see. I will add it to ip_tunnel and resubmit the patch. Thanks!
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index b279c325c7f6..10b21fe5b3a6 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -579,7 +579,7 @@ static void erspan_fb_xmit(struct sk_buff *skb, struct net_device *dev, if (gre_handle_offloads(skb, false)) goto err_free_rt; - if (skb->len > dev->mtu) { + if (skb->len - dev->hard_header_len > dev->mtu) { pskb_trim(skb, dev->mtu); truncate = true; }
Similarly to early patch for erspan_xmit(), the ARPHDR_ETHER device is the length of the whole ether packet. So skb->len should subtract the dev->hard_header_len. Fixes: 1a66a836da63 ("gre: add collect_md mode to ERSPAN tunnel") Signed-off-by: William Tu <u9012063@gmail.com> Cc: Xin Long <lucien.xin@gmail.com> --- net/ipv4/ip_gre.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)