* [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).