[PATCH] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[PATCH] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[John W. Linville]

This seems to have been a "thinko".  IP_ECN_decapsulate needs info
from both internal and external headers.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/geneve.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index da3259ce7c8d..a917ae1cfbf3 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -121,10 +121,10 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 	struct metadata_dst *tun_dst = NULL;
 	struct geneve_dev *geneve = NULL;
 	struct pcpu_sw_netstats *stats;
-	struct iphdr *iph;
+	struct iphdr *iph = NULL;
 	u8 *vni;
 	__be32 addr;
-	int err;
+	int err = 0;
 
 	if (gs->collect_md) {
 		static u8 zero_vni[3];
@@ -178,13 +178,15 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 
 	skb_reset_network_header(skb);
 
-	iph = ip_hdr(skb); /* Now inner IP header... */
-	err = IP_ECN_decapsulate(iph, skb);
+	if (iph)
+		err = IP_ECN_decapsulate(iph, skb);
 
 	if (unlikely(err)) {
 		if (log_ecn_error)
-			net_info_ratelimited("non-ECT from %pI4 with TOS=%#x\n",
-					     &iph->saddr, iph->tos);
+			if (iph)
+				net_info_ratelimited("non-ECT from %pI4 "
+						     "with TOS=%#x\n",
+						     &iph->saddr, iph->tos);
 		if (err > 1) {
 			++geneve->dev->stats.rx_frame_errors;
 			++geneve->dev->stats.rx_errors;
-- 
2.4.3

Re: [PATCH] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[Jesse Gross]

On Thu, Sep 17, 2015 at 10:17 AM, John W. Linville
<linville@tuxdriver.com> wrote:
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index da3259ce7c8d..a917ae1cfbf3 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -178,13 +178,15 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
>
>         skb_reset_network_header(skb);
>
> -       iph = ip_hdr(skb); /* Now inner IP header... */
> -       err = IP_ECN_decapsulate(iph, skb);
> +       if (iph)
> +               err = IP_ECN_decapsulate(iph, skb);

It looks like this is now conditional based on !collect_md. I'm not
sure that we want to have a difference in behavior between the two.

Re: [PATCH] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[John W. Linville]

On Thu, Sep 17, 2015 at 12:46:48PM -0700, Jesse Gross wrote:
> On Thu, Sep 17, 2015 at 10:17 AM, John W. Linville
> <linville@tuxdriver.com> wrote:
> > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> > index da3259ce7c8d..a917ae1cfbf3 100644
> > --- a/drivers/net/geneve.c
> > +++ b/drivers/net/geneve.c
> > @@ -178,13 +178,15 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
> >
> >         skb_reset_network_header(skb);
> >
> > -       iph = ip_hdr(skb); /* Now inner IP header... */
> > -       err = IP_ECN_decapsulate(iph, skb);
> > +       if (iph)
> > +               err = IP_ECN_decapsulate(iph, skb);
> 
> It looks like this is now conditional based on !collect_md. I'm not
> sure that we want to have a difference in behavior between the two.

Sure, I can move the iph assignment higher-up and keep the other bits unconditional.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH v2] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[John W. Linville]

This seems to have been a "thinko".  IP_ECN_decapsulate needs info
from both internal and external headers.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
v2 -- ensure the collect_md path still calls IP_ECN_decapsulate

 drivers/net/geneve.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index da3259ce7c8d..549febac0579 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -126,6 +126,8 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 	__be32 addr;
 	int err;
 
+	iph = ip_hdr(skb); /* outer IP header... */
+
 	if (gs->collect_md) {
 		static u8 zero_vni[3];
 
@@ -133,7 +135,6 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 		addr = 0;
 	} else {
 		vni = gnvh->vni;
-		iph = ip_hdr(skb); /* Still outer IP header... */
 		addr = iph->saddr;
 	}
 
@@ -178,7 +179,6 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 
 	skb_reset_network_header(skb);
 
-	iph = ip_hdr(skb); /* Now inner IP header... */
 	err = IP_ECN_decapsulate(iph, skb);
 
 	if (unlikely(err)) {
-- 
2.4.3

Re: [PATCH v2] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[Jesse Gross]

On Thu, Sep 17, 2015 at 1:34 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> This seems to have been a "thinko".  IP_ECN_decapsulate needs info
> from both internal and external headers.
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

This looks good to me although I realized that the transmit path is
also conditional based on !collect_md metadata. I suppose that means
that this difference is intentional and I guess I could see arguments
either way but I still think it is better to be consistent across
different code paths in this respect.

Re: [PATCH v2] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[John W. Linville]

On Fri, Sep 18, 2015 at 01:30:36PM -0700, Jesse Gross wrote:
> On Thu, Sep 17, 2015 at 1:34 PM, John W. Linville
> <linville@tuxdriver.com> wrote:
> > This seems to have been a "thinko".  IP_ECN_decapsulate needs info
> > from both internal and external headers.
> >
> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> 
> This looks good to me although I realized that the transmit path is
> also conditional based on !collect_md metadata. I suppose that means
> that this difference is intentional and I guess I could see arguments
> either way but I still think it is better to be consistent across
> different code paths in this respect.

I'm happy with the v2 patch if you are. :-)

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH v2] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[Jesse Gross]

On Fri, Sep 18, 2015 at 1:40 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Fri, Sep 18, 2015 at 01:30:36PM -0700, Jesse Gross wrote:
>> On Thu, Sep 17, 2015 at 1:34 PM, John W. Linville
>> <linville@tuxdriver.com> wrote:
>> > This seems to have been a "thinko".  IP_ECN_decapsulate needs info
>> > from both internal and external headers.
>> >
>> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
>>
>> This looks good to me although I realized that the transmit path is
>> also conditional based on !collect_md metadata. I suppose that means
>> that this difference is intentional and I guess I could see arguments
>> either way but I still think it is better to be consistent across
>> different code paths in this respect.
>
> I'm happy with the v2 patch if you are. :-)

I'm happy with this part of the code path after this patch. However,
my concern was that this patch, while correct, will make receive
inconsistent with transmit. Presumably we should update the transmit
side as well.

Re: [PATCH v2] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[John W. Linville]

On Fri, Sep 18, 2015 at 02:49:30PM -0700, Jesse Gross wrote:
> On Fri, Sep 18, 2015 at 1:40 PM, John W. Linville
> <linville@tuxdriver.com> wrote:
> > On Fri, Sep 18, 2015 at 01:30:36PM -0700, Jesse Gross wrote:
> >> On Thu, Sep 17, 2015 at 1:34 PM, John W. Linville
> >> <linville@tuxdriver.com> wrote:
> >> > This seems to have been a "thinko".  IP_ECN_decapsulate needs info
> >> > from both internal and external headers.
> >> >
> >> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> >>
> >> This looks good to me although I realized that the transmit path is
> >> also conditional based on !collect_md metadata. I suppose that means
> >> that this difference is intentional and I guess I could see arguments
> >> either way but I still think it is better to be consistent across
> >> different code paths in this respect.
> >
> > I'm happy with the v2 patch if you are. :-)
> 
> I'm happy with this part of the code path after this patch. However,
> my concern was that this patch, while correct, will make receive
> inconsistent with transmit. Presumably we should update the transmit
> side as well.

Oh, sorry -- I missed your point.  You mean something like this?

@@ -812,19 +815,16 @@ static netdev_tx_t geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev)
		if (unlikely(err))
			goto err;
 
-		tos = key->tos;
+		tos = ip_tunnel_ecn_encap(key->tos, iip, skb);
		ttl = key->ttl;
		df = key->tun_flags & TUNNEL_DONT_FRAGMENT ? htons(IP_DF) : 0;

Am I understanding now?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH v2] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[Jesse Gross]

On Fri, Sep 18, 2015 at 6:36 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Fri, Sep 18, 2015 at 02:49:30PM -0700, Jesse Gross wrote:
>> On Fri, Sep 18, 2015 at 1:40 PM, John W. Linville
>> <linville@tuxdriver.com> wrote:
>> > On Fri, Sep 18, 2015 at 01:30:36PM -0700, Jesse Gross wrote:
>> >> On Thu, Sep 17, 2015 at 1:34 PM, John W. Linville
>> >> <linville@tuxdriver.com> wrote:
>> >> > This seems to have been a "thinko".  IP_ECN_decapsulate needs info
>> >> > from both internal and external headers.
>> >> >
>> >> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
>> >>
>> >> This looks good to me although I realized that the transmit path is
>> >> also conditional based on !collect_md metadata. I suppose that means
>> >> that this difference is intentional and I guess I could see arguments
>> >> either way but I still think it is better to be consistent across
>> >> different code paths in this respect.
>> >
>> > I'm happy with the v2 patch if you are. :-)
>>
>> I'm happy with this part of the code path after this patch. However,
>> my concern was that this patch, while correct, will make receive
>> inconsistent with transmit. Presumably we should update the transmit
>> side as well.
>
> Oh, sorry -- I missed your point.  You mean something like this?
>
> @@ -812,19 +815,16 @@ static netdev_tx_t geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev)
>                 if (unlikely(err))
>                         goto err;
>
> -               tos = key->tos;
> +               tos = ip_tunnel_ecn_encap(key->tos, iip, skb);
>                 ttl = key->ttl;
>                 df = key->tun_flags & TUNNEL_DONT_FRAGMENT ? htons(IP_DF) : 0;
>
> Am I understanding now?

Yes, essentially though I think that 'iip' is only defined in the
latter half of the if statement and it might be nice to have the ECN
encapsulation called only once before udp_tunnel_xmit_skb() rather
than independently in each code path.

Re: [PATCH v2] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[David Miller]

From: "John W. Linville" <linville@tuxdriver.com>
Date: Thu, 17 Sep 2015 16:34:54 -0400

> This seems to have been a "thinko".  IP_ECN_decapsulate needs info
> from both internal and external headers.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
> v2 -- ensure the collect_md path still calls IP_ECN_decapsulate

Judging by the discussion on this patch, I expect that this will
be respun to make the TX path match what we'll be doing in the RX
path here.

Re: [PATCH v2] geneve: remove use of internal IP header when calling IP_ECN_decapsulate

[John W. Linville]

On Sun, Sep 20, 2015 at 10:24:59PM -0700, David Miller wrote:
> From: "John W. Linville" <linville@tuxdriver.com>
> Date: Thu, 17 Sep 2015 16:34:54 -0400
> 
> > This seems to have been a "thinko".  IP_ECN_decapsulate needs info
> > from both internal and external headers.
> > 
> > Signed-off-by: John W. Linville <linville@tuxdriver.com>
> > ---
> > v2 -- ensure the collect_md path still calls IP_ECN_decapsulate
> 
> Judging by the discussion on this patch, I expect that this will
> be respun to make the TX path match what we'll be doing in the RX
> path here.

Yup -- planning to do that this morning!

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH v3] geneve: ensure ECN info is handled properly in all tx/rx paths

[John W. Linville]

Partially due to a pre-exising "thinko", the new metadata-based tx/rx
paths were handling ECN propagation differently than the traditional
tx/rx paths.  This patch removes the "thinko" (involving multiple
ip_hdr assignments) on the rx path and corrects the ECN handling on
both the rx and tx paths.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
v3 -- replace "geneve: remove use of internal IP header when calling
	IP_ECN_decapsulate", include geneve_xmit bits
v2 -- ensure the collect_md path still calls IP_ECN_decapsulate

 drivers/net/geneve.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index da3259ce7c8d..922101253993 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -126,6 +126,8 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 	__be32 addr;
 	int err;
 
+	iph = ip_hdr(skb); /* outer IP header... */
+
 	if (gs->collect_md) {
 		static u8 zero_vni[3];
 
@@ -133,7 +135,6 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 		addr = 0;
 	} else {
 		vni = gnvh->vni;
-		iph = ip_hdr(skb); /* Still outer IP header... */
 		addr = iph->saddr;
 	}
 
@@ -178,7 +179,6 @@ static void geneve_rx(struct geneve_sock *gs, struct sk_buff *skb)
 
 	skb_reset_network_header(skb);
 
-	iph = ip_hdr(skb); /* Now inner IP header... */
 	err = IP_ECN_decapsulate(iph, skb);
 
 	if (unlikely(err)) {
@@ -626,6 +626,7 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct geneve_sock *gs = geneve->sock;
 	struct ip_tunnel_info *info = NULL;
 	struct rtable *rt = NULL;
+	const struct iphdr *iip; /* interior IP header */
 	struct flowi4 fl4;
 	__u8 tos, ttl;
 	__be16 sport;
@@ -653,6 +654,8 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb, struct net_device *dev)
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
 	skb_reset_mac_header(skb);
 
+	iip = ip_hdr(skb);
+
 	if (info) {
 		const struct ip_tunnel_key *key = &info->key;
 		u8 *opts = NULL;
@@ -668,19 +671,16 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb, struct net_device *dev)
 		if (unlikely(err))
 			goto err;
 
-		tos = key->tos;
+		tos = ip_tunnel_ecn_encap(key->tos, iip, skb);
 		ttl = key->ttl;
 		df = key->tun_flags & TUNNEL_DONT_FRAGMENT ? htons(IP_DF) : 0;
 	} else {
-		const struct iphdr *iip; /* interior IP header */
-
 		udp_csum = false;
 		err = geneve_build_skb(rt, skb, 0, geneve->vni,
 				       0, NULL, udp_csum);
 		if (unlikely(err))
 			goto err;
 
-		iip = ip_hdr(skb);
 		tos = ip_tunnel_ecn_encap(fl4.flowi4_tos, iip, skb);
 		ttl = geneve->ttl;
 		if (!ttl && IN_MULTICAST(ntohl(fl4.daddr)))
-- 
2.4.3

Re: [PATCH v3] geneve: ensure ECN info is handled properly in all tx/rx paths

[Jesse Gross]

On Mon, Sep 21, 2015 at 7:29 AM, John W. Linville
<linville@tuxdriver.com> wrote:
> Partially due to a pre-exising "thinko", the new metadata-based tx/rx
> paths were handling ECN propagation differently than the traditional
> tx/rx paths.  This patch removes the "thinko" (involving multiple
> ip_hdr assignments) on the rx path and corrects the ECN handling on
> both the rx and tx paths.
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

Reviewed-by: Jesse Gross <jesse@nicira.com>

Re: [PATCH v3] geneve: ensure ECN info is handled properly in all tx/rx paths

[David Miller]

From: "John W. Linville" <linville@tuxdriver.com>
Date: Mon, 21 Sep 2015 10:29:09 -0400

> Partially due to a pre-exising "thinko", the new metadata-based tx/rx
> paths were handling ECN propagation differently than the traditional
> tx/rx paths.  This patch removes the "thinko" (involving multiple
> ip_hdr assignments) on the rx path and corrects the ECN handling on
> both the rx and tx paths.
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> ---
> v3 -- replace "geneve: remove use of internal IP header when calling
> 	IP_ECN_decapsulate", include geneve_xmit bits
> v2 -- ensure the collect_md path still calls IP_ECN_decapsulate

Applied, thanks John.