* [PATCH net] gre: fix error handler @ 2016-06-15 5:15 Eric Dumazet 2016-06-15 5:45 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2016-06-15 5:15 UTC (permalink / raw) To: David Miller; +Cc: netdev, Tom Herbert, Maciej Żenczykowski From: Eric Dumazet <edumazet@google.com> 1) gre_parse_header() can be called from gre_err() At this point transport header points to ICMP header, not the inner header. 2) We can not really change transport header as ipgre_err() will later assume transport header still points to ICMP header. 3) pskb_may_pull() logic in gre_parse_header() really works if we are interested at zone pointed by skb->data So this fix : A) changes gre_parse_header() to use skb->data instead of skb_transport_header() B) changes gre_err() to pull the IPv4 header immediately following the ICMP header that was already pulled earlier. C) remove obsolete IPV6 includes Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Tom Herbert <tom@herbertland.com> Cc: Maciej Żenczykowski <maze@google.com> --- net/ipv4/gre_demux.c | 4 ++-- net/ipv4/ip_gre.c | 9 +++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c index 4c39f4fd332a..0ba26ad9809d 100644 --- a/net/ipv4/gre_demux.c +++ b/net/ipv4/gre_demux.c @@ -71,7 +71,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, if (unlikely(!pskb_may_pull(skb, sizeof(struct gre_base_hdr)))) return -EINVAL; - greh = (struct gre_base_hdr *)skb_transport_header(skb); + greh = (struct gre_base_hdr *)skb->data; if (unlikely(greh->flags & (GRE_VERSION | GRE_ROUTING))) return -EINVAL; @@ -81,7 +81,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, if (!pskb_may_pull(skb, hdr_len)) return -EINVAL; - greh = (struct gre_base_hdr *)skb_transport_header(skb); + greh = (struct gre_base_hdr *)skb->data; tpi->proto = greh->protocol; options = (__be32 *)(greh + 1); diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 4d2025f7ec57..454832bc2897 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -49,12 +49,6 @@ #include <net/gre.h> #include <net/dst_metadata.h> -#if IS_ENABLED(CONFIG_IPV6) -#include <net/ipv6.h> -#include <net/ip6_fib.h> -#include <net/ip6_route.h> -#endif - /* Problems & solutions -------------------- @@ -217,11 +211,14 @@ static void gre_err(struct sk_buff *skb, u32 info) * by themselves??? */ + const struct iphdr *iph = (struct iphdr *)skb->data; const int type = icmp_hdr(skb)->type; const int code = icmp_hdr(skb)->code; struct tnl_ptk_info tpi; bool csum_err = false; + pskb_pull(skb, iph->ihl * 4); + if (gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IP)) < 0) { if (!csum_err) /* ignore csum errors. */ return; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] gre: fix error handler 2016-06-15 5:15 [PATCH net] gre: fix error handler Eric Dumazet @ 2016-06-15 5:45 ` Eric Dumazet 2016-06-15 13:24 ` [PATCH v2 " Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2016-06-15 5:45 UTC (permalink / raw) To: David Miller, Jiri Benc; +Cc: netdev, Tom Herbert, Maciej Żenczykowski On Tue, 2016-06-14 at 22:15 -0700, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > 1) gre_parse_header() can be called from gre_err() > > At this point transport header points to ICMP header, not the inner > header. > > 2) We can not really change transport header as ipgre_err() will later > assume transport header still points to ICMP header. > > 3) pskb_may_pull() logic in gre_parse_header() really works > if we are interested at zone pointed by skb->data > > So this fix : > > A) changes gre_parse_header() to use skb->data instead of > skb_transport_header() > > B) changes gre_err() to pull the IPv4 header immediately following > the ICMP header that was already pulled earlier. > > C) remove obsolete IPV6 includes > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Tom Herbert <tom@herbertland.com> > Cc: Maciej Żenczykowski <maze@google.com> > --- > net/ipv4/gre_demux.c | 4 ++-- > net/ipv4/ip_gre.c | 9 +++------ > 2 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c > index 4c39f4fd332a..0ba26ad9809d 100644 > --- a/net/ipv4/gre_demux.c > +++ b/net/ipv4/gre_demux.c > @@ -71,7 +71,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, > if (unlikely(!pskb_may_pull(skb, sizeof(struct gre_base_hdr)))) > return -EINVAL; > > - greh = (struct gre_base_hdr *)skb_transport_header(skb); > + greh = (struct gre_base_hdr *)skb->data; > if (unlikely(greh->flags & (GRE_VERSION | GRE_ROUTING))) > return -EINVAL; > > @@ -81,7 +81,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, > if (!pskb_may_pull(skb, hdr_len)) > return -EINVAL; > > - greh = (struct gre_base_hdr *)skb_transport_header(skb); > + greh = (struct gre_base_hdr *)skb->data; > tpi->proto = greh->protocol; > > options = (__be32 *)(greh + 1); > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > index 4d2025f7ec57..454832bc2897 100644 > --- a/net/ipv4/ip_gre.c > +++ b/net/ipv4/ip_gre.c > @@ -49,12 +49,6 @@ > #include <net/gre.h> > #include <net/dst_metadata.h> > > -#if IS_ENABLED(CONFIG_IPV6) > -#include <net/ipv6.h> > -#include <net/ip6_fib.h> > -#include <net/ip6_route.h> > -#endif > - > /* > Problems & solutions > -------------------- > @@ -217,11 +211,14 @@ static void gre_err(struct sk_buff *skb, u32 info) > * by themselves??? > */ > > + const struct iphdr *iph = (struct iphdr *)skb->data; > const int type = icmp_hdr(skb)->type; > const int code = icmp_hdr(skb)->code; > struct tnl_ptk_info tpi; > bool csum_err = false; > > + pskb_pull(skb, iph->ihl * 4); > + > if (gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IP)) < 0) { > if (!csum_err) /* ignore csum errors. */ > return; > Hmm... I read this prior commit. It looks like we need something else. commit b7f8fe251e4609e2a437bd2c2dea01e61db6849c Author: Jiri Benc <jbenc@redhat.com> Date: Fri Apr 29 23:31:32 2016 +0200 gre: do not pull header in ICMP error processing iptunnel_pull_header expects that IP header was already pulled; with this expectation, it pulls the tunnel header. This is not true in gre_err. Furthermore, ipv4_update_pmtu and ipv4_redirect expect that skb->data points to the IP header. We cannot pull the tunnel header in this path. It's just a matter of not calling iptunnel_pull_header - we don't need any of its effects. Fixes: bda7bb463436 ("gre: Allow multiple protocol listener for gre protocol.") Signed-off-by: Jiri Benc <jbenc@redhat.com> Signed-off-by: David S. Miller <davem@davemloft.net> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 net] gre: fix error handler 2016-06-15 5:45 ` Eric Dumazet @ 2016-06-15 13:24 ` Eric Dumazet 2016-06-16 5:15 ` David Miller 2016-06-16 7:40 ` Jiri Benc 0 siblings, 2 replies; 6+ messages in thread From: Eric Dumazet @ 2016-06-15 13:24 UTC (permalink / raw) To: David Miller; +Cc: Jiri Benc, netdev, Tom Herbert, Maciej Żenczykowski From: Eric Dumazet <edumazet@google.com> 1) gre_parse_header() can be called from gre_err() At this point transport header points to ICMP header, not the inner header. 2) We can not really change transport header as ipgre_err() will later assume transport header still points to ICMP header (using icmp_hdr()) 3) pskb_may_pull() logic in gre_parse_header() really works if we are interested at zone pointed by skb->data 4) As Jiri explained in commit b7f8fe251e46 ("gre: do not pull header in ICMP error processing") we should not pull headers in error handler. So this fix : A) changes gre_parse_header() to use skb->data instead of skb_transport_header() B) Adds a nhs parameter to gre_parse_header() so that we can skip the not pulled IP header from error path. This offset is 0 for normal receive path. C) remove obsolete IPV6 includes Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Tom Herbert <tom@herbertland.com> Cc: Maciej Żenczykowski <maze@google.com> Cc: Jiri Benc <jbenc@redhat.com> --- include/net/gre.h | 2 +- net/ipv4/gre_demux.c | 10 +++++----- net/ipv4/ip_gre.c | 12 ++++-------- net/ipv6/ip6_gre.c | 2 +- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/include/net/gre.h b/include/net/gre.h index 5dce30a6abe3..7a54a31d1d4c 100644 --- a/include/net/gre.h +++ b/include/net/gre.h @@ -26,7 +26,7 @@ int gre_del_protocol(const struct gre_protocol *proto, u8 version); struct net_device *gretap_fb_dev_create(struct net *net, const char *name, u8 name_assign_type); int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, - bool *csum_err, __be16 proto); + bool *csum_err, __be16 proto, int nhs); static inline int gre_calc_hlen(__be16 o_flags) { diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c index 4c39f4fd332a..de1d119a4497 100644 --- a/net/ipv4/gre_demux.c +++ b/net/ipv4/gre_demux.c @@ -62,26 +62,26 @@ EXPORT_SYMBOL_GPL(gre_del_protocol); /* Fills in tpi and returns header length to be pulled. */ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, - bool *csum_err, __be16 proto) + bool *csum_err, __be16 proto, int nhs) { const struct gre_base_hdr *greh; __be32 *options; int hdr_len; - if (unlikely(!pskb_may_pull(skb, sizeof(struct gre_base_hdr)))) + if (unlikely(!pskb_may_pull(skb, nhs + sizeof(struct gre_base_hdr)))) return -EINVAL; - greh = (struct gre_base_hdr *)skb_transport_header(skb); + greh = (struct gre_base_hdr *)(skb->data + nhs); if (unlikely(greh->flags & (GRE_VERSION | GRE_ROUTING))) return -EINVAL; tpi->flags = gre_flags_to_tnl_flags(greh->flags); hdr_len = gre_calc_hlen(tpi->flags); - if (!pskb_may_pull(skb, hdr_len)) + if (!pskb_may_pull(skb, nhs + hdr_len)) return -EINVAL; - greh = (struct gre_base_hdr *)skb_transport_header(skb); + greh = (struct gre_base_hdr *)(skb->data + nhs); tpi->proto = greh->protocol; options = (__be32 *)(greh + 1); diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 07c5cf1838d8..1d000af7f561 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -49,12 +49,6 @@ #include <net/gre.h> #include <net/dst_metadata.h> -#if IS_ENABLED(CONFIG_IPV6) -#include <net/ipv6.h> -#include <net/ip6_fib.h> -#include <net/ip6_route.h> -#endif - /* Problems & solutions -------------------- @@ -217,12 +211,14 @@ static void gre_err(struct sk_buff *skb, u32 info) * by themselves??? */ + const struct iphdr *iph = (struct iphdr *)skb->data; const int type = icmp_hdr(skb)->type; const int code = icmp_hdr(skb)->code; struct tnl_ptk_info tpi; bool csum_err = false; - if (gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IP)) < 0) { + if (gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IP), + iph->ihl * 4) < 0) { if (!csum_err) /* ignore csum errors. */ return; } @@ -338,7 +334,7 @@ static int gre_rcv(struct sk_buff *skb) } #endif - hdr_len = gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IP)); + hdr_len = gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IP), 0); if (hdr_len < 0) goto drop; diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index fdc9de276ab1..776d145113e1 100644 --- a/net/ipv6/ip6_gre.c +++ b/net/ipv6/ip6_gre.c @@ -468,7 +468,7 @@ static int gre_rcv(struct sk_buff *skb) bool csum_err = false; int hdr_len; - hdr_len = gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IPV6)); + hdr_len = gre_parse_header(skb, &tpi, &csum_err, htons(ETH_P_IPV6), 0); if (hdr_len < 0) goto drop; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] gre: fix error handler 2016-06-15 13:24 ` [PATCH v2 " Eric Dumazet @ 2016-06-16 5:15 ` David Miller 2016-06-16 7:40 ` Jiri Benc 1 sibling, 0 replies; 6+ messages in thread From: David Miller @ 2016-06-16 5:15 UTC (permalink / raw) To: eric.dumazet; +Cc: jbenc, netdev, tom, maze From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 15 Jun 2016 06:24:00 -0700 > From: Eric Dumazet <edumazet@google.com> > > 1) gre_parse_header() can be called from gre_err() > > At this point transport header points to ICMP header, not the inner > header. > > 2) We can not really change transport header as ipgre_err() will later > assume transport header still points to ICMP header (using icmp_hdr()) > > 3) pskb_may_pull() logic in gre_parse_header() really works > if we are interested at zone pointed by skb->data > > 4) As Jiri explained in commit b7f8fe251e46 ("gre: do not pull header in > ICMP error processing") we should not pull headers in error handler. > > So this fix : > > A) changes gre_parse_header() to use skb->data instead of > skb_transport_header() > > B) Adds a nhs parameter to gre_parse_header() so that we can skip the > not pulled IP header from error path. > This offset is 0 for normal receive path. > > C) remove obsolete IPV6 includes > > Signed-off-by: Eric Dumazet <edumazet@google.com> Applied, thanks Eric. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] gre: fix error handler 2016-06-15 13:24 ` [PATCH v2 " Eric Dumazet 2016-06-16 5:15 ` David Miller @ 2016-06-16 7:40 ` Jiri Benc 2016-06-16 11:40 ` Eric Dumazet 1 sibling, 1 reply; 6+ messages in thread From: Jiri Benc @ 2016-06-16 7:40 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, netdev, Tom Herbert, Maciej Żenczykowski On Wed, 15 Jun 2016 06:24:00 -0700, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > 1) gre_parse_header() can be called from gre_err() > > At this point transport header points to ICMP header, not the inner > header. > > 2) We can not really change transport header as ipgre_err() will later > assume transport header still points to ICMP header (using icmp_hdr()) > > 3) pskb_may_pull() logic in gre_parse_header() really works > if we are interested at zone pointed by skb->data > > 4) As Jiri explained in commit b7f8fe251e46 ("gre: do not pull header in > ICMP error processing") we should not pull headers in error handler. > > So this fix : > > A) changes gre_parse_header() to use skb->data instead of > skb_transport_header() > > B) Adds a nhs parameter to gre_parse_header() so that we can skip the > not pulled IP header from error path. > This offset is 0 for normal receive path. > > C) remove obsolete IPV6 includes > > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Tom Herbert <tom@herbertland.com> > Cc: Maciej Żenczykowski <maze@google.com> > Cc: Jiri Benc <jbenc@redhat.com> Reviewed-by: Jiri Benc <jbenc@redhat.com> I know this has already been applied. Didn't get to it earlier, sorry. The patch looks good. Thanks! Jiri ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] gre: fix error handler 2016-06-16 7:40 ` Jiri Benc @ 2016-06-16 11:40 ` Eric Dumazet 0 siblings, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2016-06-16 11:40 UTC (permalink / raw) To: Jiri Benc; +Cc: David Miller, netdev, Tom Herbert, Maciej Żenczykowski On Thu, 2016-06-16 at 09:40 +0200, Jiri Benc wrote: > Reviewed-by: Jiri Benc <jbenc@redhat.com> > > I know this has already been applied. Didn't get to it earlier, sorry. > The patch looks good. Thanks! Thanks Jiri for reviewing it ! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-16 11:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-15 5:15 [PATCH net] gre: fix error handler Eric Dumazet 2016-06-15 5:45 ` Eric Dumazet 2016-06-15 13:24 ` [PATCH v2 " Eric Dumazet 2016-06-16 5:15 ` David Miller 2016-06-16 7:40 ` Jiri Benc 2016-06-16 11:40 ` Eric Dumazet
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).