Re: [PATCH v3] ip6_tunnel: Correct tos value in collect_md mode

2017-06-18 Thread Peter Dawson 
On Sat, 17 Jun 2017 11:38:05 +0800
Haishuang Yan <yanhaishu...@cmss.chinamobile.com> wrote:

> Same as ip_gre, geneve and vxlan, use key->tos as traffic class value.
> 
> CC: Peter Dawson <peted...@gmail.com>
> Fixes: 0e9a709560db ("ip6_tunnel, ip6_gre: fix setting of DSCP on
> encapsulated packets”)
> Signed-off-by: Haishuang Yan <yanhaishu...@cmss.chinamobile.com>
> 
> ---
> Changes since v3:
>   * Add fixes information
>   * Remove obsoleted RT_TOS mask
> ---
>  net/ipv6/ip6_tunnel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index ef99d59..9d65918 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1249,7 +1249,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
> *dev, __u8 dsfield,
>   fl6.flowi6_proto = IPPROTO_IPIP;
>   fl6.daddr = key->u.ipv6.dst;
>   fl6.flowlabel = key->label;
> - dsfield = ip6_tclass(key->label);
> + dsfield =  key->tos;
>   } else {
>   if (!(t->parms.flags & IP6_TNL_F_IGN_ENCAP_LIMIT))
>   encap_limit = t->parms.encap_limit;
> @@ -1320,7 +1320,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
> *dev, __u8 dsfield,
>   fl6.flowi6_proto = IPPROTO_IPV6;
>   fl6.daddr = key->u.ipv6.dst;
>   fl6.flowlabel = key->label;
> - dsfield = ip6_tclass(key->label);
> + dsfield = key->tos;
>   } else {
>       offset = ip6_tnl_parse_tlv_enc_lim(skb, 
> skb_network_header(skb));
>   /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head 
> */

Acked-by: Peter Dawson <peter.a.daw...@boeing.com>

Re: [PATCH v3] ip6_tunnel: Correct tos value in collect_md mode

2017-06-18 Thread Peter Dawson 
On Sat, 17 Jun 2017 11:38:05 +0800
Haishuang Yan  wrote:

> Same as ip_gre, geneve and vxlan, use key->tos as traffic class value.
> 
> CC: Peter Dawson 
> Fixes: 0e9a709560db ("ip6_tunnel, ip6_gre: fix setting of DSCP on
> encapsulated packets”)
> Signed-off-by: Haishuang Yan 
> 
> ---
> Changes since v3:
>   * Add fixes information
>   * Remove obsoleted RT_TOS mask
> ---
>  net/ipv6/ip6_tunnel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index ef99d59..9d65918 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1249,7 +1249,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
> *dev, __u8 dsfield,
>   fl6.flowi6_proto = IPPROTO_IPIP;
>   fl6.daddr = key->u.ipv6.dst;
>   fl6.flowlabel = key->label;
> - dsfield = ip6_tclass(key->label);
> + dsfield =  key->tos;
>   } else {
>   if (!(t->parms.flags & IP6_TNL_F_IGN_ENCAP_LIMIT))
>   encap_limit = t->parms.encap_limit;
> @@ -1320,7 +1320,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
> *dev, __u8 dsfield,
>   fl6.flowi6_proto = IPPROTO_IPV6;
>   fl6.daddr = key->u.ipv6.dst;
>   fl6.flowlabel = key->label;
> - dsfield = ip6_tclass(key->label);
> + dsfield = key->tos;
>   } else {
>   offset = ip6_tnl_parse_tlv_enc_lim(skb, 
> skb_network_header(skb));
>   /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head 
> */

Acked-by: Peter Dawson

Re: [PATCH v2] ip6_tunnel: Correct tos value in collect_md mode

2017-06-14 Thread Peter Dawson 
On Thu, 15 Jun 2017 10:30:29 +0800
Haishuang Yan <yanhaishu...@cmss.chinamobile.com> wrote:

> Same as ip_gre, geneve and vxlan, use key->tos as tos value.
> 
> CC: Peter Dawson <peted...@gmail.com>
> Fixes: 0e9a709560db ("ip6_tunnel, ip6_gre: fix setting of DSCP on
> encapsulated packets”)
> Suggested-by: Daniel Borkmann <dan...@iogearbox.net>
> Signed-off-by: Haishuang Yan <yanhaishu...@cmss.chinamobile.com>
> 
> ---
> Changes since v2:
>   * Add fixes information
>   * mask key->tos with RT_TOS() suggested by Daniel
> ---
>  net/ipv6/ip6_tunnel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index ef99d59..6400726 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1249,7 +1249,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
> *dev, __u8 dsfield,
>   fl6.flowi6_proto = IPPROTO_IPIP;
>   fl6.daddr = key->u.ipv6.dst;
>   fl6.flowlabel = key->label;
> - dsfield = ip6_tclass(key->label);
> + dsfield =  RT_TOS(key->tos);
>   } else {
>   if (!(t->parms.flags & IP6_TNL_F_IGN_ENCAP_LIMIT))
>   encap_limit = t->parms.encap_limit;
> @@ -1320,7 +1320,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
> *dev, __u8 dsfield,
>   fl6.flowi6_proto = IPPROTO_IPV6;
>   fl6.daddr = key->u.ipv6.dst;
>   fl6.flowlabel = key->label;
> - dsfield = ip6_tclass(key->label);
> + dsfield = RT_TOS(key->tos);
>   } else {
>   offset = ip6_tnl_parse_tlv_enc_lim(skb, 
> skb_network_header(skb));
>   /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head 
> */

I don't think it is correct to apply RT_TOS

Here is my understanding based on the RFCs.

IPv4/6 Header:0 |0 1 2 3 |0 1 2 3 |0 1 2 3 |0 1 2 3 |
RFC2460(IPv6)   |Version | Traffic Class   ||
RFC2474(IPv6)   |Version | DSCP|ECN||
RFC2474(IPv4)   |Version |  IHL   |DSCP |ECN|
RFC1349(IPv4)   |Version |  IHL   | PREC |  TOS   |X| 
RFC791 (IPv4)   |Version |  IHL   |  TOS|

u8 key->tos stores the full 8bits of Traffic class from an IPv6 header and;
u8 key->tos stores the full 8bits of TOS(RFC791) from an IPv4 header
u8 ip6_tclass will return the full 8bits of Traffic Class from an IPv6 flowlabel

RT_TOS will return the RFC1349 4bit TOS field.

Applying RT_TOS to a key->tos will result in lost information and the inclusion 
of 1 bit of ECN if the original field was a DSCP+ECN.

Based on this understanding of the RFCs (but not years of experience) and since 
RFC1349 has been obsoleted by RFC2474 I think the use of RT_TOS should be 
deprecated.

This being said, dsfield = ip6_tclass(key->label) = key->tos isn't fully 
correct either because the result will contain the ECN bits as well as the DSCP.

I agree that code should be consistent, but not where there is a potential 
issue.

Re: [PATCH v2] ip6_tunnel: Correct tos value in collect_md mode

2017-06-14 Thread Peter Dawson 
On Thu, 15 Jun 2017 10:30:29 +0800
Haishuang Yan  wrote:

> Same as ip_gre, geneve and vxlan, use key->tos as tos value.
> 
> CC: Peter Dawson 
> Fixes: 0e9a709560db ("ip6_tunnel, ip6_gre: fix setting of DSCP on
> encapsulated packets”)
> Suggested-by: Daniel Borkmann 
> Signed-off-by: Haishuang Yan 
> 
> ---
> Changes since v2:
>   * Add fixes information
>   * mask key->tos with RT_TOS() suggested by Daniel
> ---
>  net/ipv6/ip6_tunnel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index ef99d59..6400726 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1249,7 +1249,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
> *dev, __u8 dsfield,
>   fl6.flowi6_proto = IPPROTO_IPIP;
>   fl6.daddr = key->u.ipv6.dst;
>   fl6.flowlabel = key->label;
> - dsfield = ip6_tclass(key->label);
> + dsfield =  RT_TOS(key->tos);
>   } else {
>   if (!(t->parms.flags & IP6_TNL_F_IGN_ENCAP_LIMIT))
>   encap_limit = t->parms.encap_limit;
> @@ -1320,7 +1320,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
> *dev, __u8 dsfield,
>   fl6.flowi6_proto = IPPROTO_IPV6;
>   fl6.daddr = key->u.ipv6.dst;
>   fl6.flowlabel = key->label;
> - dsfield = ip6_tclass(key->label);
> + dsfield = RT_TOS(key->tos);
>   } else {
>   offset = ip6_tnl_parse_tlv_enc_lim(skb, 
> skb_network_header(skb));
>   /* ip6_tnl_parse_tlv_enc_lim() might have reallocated skb->head 
> */

I don't think it is correct to apply RT_TOS

Here is my understanding based on the RFCs.

IPv4/6 Header:0 |0 1 2 3 |0 1 2 3 |0 1 2 3 |0 1 2 3 |
RFC2460(IPv6)   |Version | Traffic Class   ||
RFC2474(IPv6)   |Version | DSCP|ECN||
RFC2474(IPv4)   |Version |  IHL   |DSCP |ECN|
RFC1349(IPv4)   |Version |  IHL   | PREC |  TOS   |X| 
RFC791 (IPv4)   |Version |  IHL   |  TOS|

u8 key->tos stores the full 8bits of Traffic class from an IPv6 header and;
u8 key->tos stores the full 8bits of TOS(RFC791) from an IPv4 header
u8 ip6_tclass will return the full 8bits of Traffic Class from an IPv6 flowlabel

RT_TOS will return the RFC1349 4bit TOS field.

Applying RT_TOS to a key->tos will result in lost information and the inclusion 
of 1 bit of ECN if the original field was a DSCP+ECN.

Based on this understanding of the RFCs (but not years of experience) and since 
RFC1349 has been obsoleted by RFC2474 I think the use of RT_TOS should be 
deprecated.

This being said, dsfield = ip6_tclass(key->label) = key->tos isn't fully 
correct either because the result will contain the ECN bits as well as the DSCP.

I agree that code should be consistent, but not where there is a potential 
issue.

Re: [PATCH] ip6_tunnel: Correct tos value in collect_md mode

2017-06-13 Thread Peter Dawson 
On Wed, 14 Jun 2017 10:54:31 +0800
严海双  wrote:


> > Changes since v2:
> >  * mask key->tos with RT_TOS() suggested by Daniel

Can you help me understand the rationale for this change? Is there are bug 
introduced by dsfield = ip6_tclass(key->label); ?

The RT_TOS masks out 4bits of the 8bit tos field in accordance with RFC1349 
(obsoleted by RFC2474). IPv6 does not have a TOS field. So it dosen't make 
sense to apply a TOS value to the outer header of an IPv6 tunnel.

Re: [PATCH] ip6_tunnel: Correct tos value in collect_md mode

2017-06-13 Thread Peter Dawson 
On Wed, 14 Jun 2017 10:54:31 +0800
严海双  wrote:


> > Changes since v2:
> >  * mask key->tos with RT_TOS() suggested by Daniel

Can you help me understand the rationale for this change? Is there are bug 
introduced by dsfield = ip6_tclass(key->label); ?

The RT_TOS masks out 4bits of the 8bit tos field in accordance with RFC1349 
(obsoleted by RFC2474). IPv6 does not have a TOS field. So it dosen't make 
sense to apply a TOS value to the outer header of an IPv6 tunnel.

[PATCH net,v2] ip6_tunnel, ip6_gre: fix setting of DSCP on encapsulated packets

2017-05-25 Thread Peter Dawson 
This fix addresses two problems in the way the DSCP field is formulated
 on the encapsulating header of IPv6 tunnels.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=195661

1) The IPv6 tunneling code was manipulating the DSCP field of the
 encapsulating packet using the 32b flowlabel. Since the flowlabel is
 only the lower 20b it was incorrect to assume that the upper 12b
 containing the DSCP and ECN fields would remain intact when formulating
 the encapsulating header. This fix handles the 'inherit' and
 'fixed-value' DSCP cases explicitly using the extant dsfield u8 variable.

2) The use of INET_ECN_encapsulate(0, dsfield) in ip6_tnl_xmit was
 incorrect and resulted in the DSCP value always being set to 0.

Commit 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class 
 is non-0") caused the regression by masking out the flowlabel
 which exposed the incorrect handling of the DSCP portion of the 
 flowlabel in ip6_tunnel and ip6_gre.

Fixes: 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class is non-0")
Signed-off-by: Peter Dawson <peter.a.daw...@boeing.com>
---
 net/ipv6/ip6_gre.c| 13 +++--
 net/ipv6/ip6_tunnel.c | 21 +
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 8d128ba..0c5b4caa 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -537,11 +537,10 @@ static inline int ip6gre_xmit_ipv4(struct sk_buff *skb, 
struct net_device *dev)
 
memcpy(, >fl.u.ip6, sizeof(fl6));
 
-   dsfield = ipv4_get_dsfield(iph);
-
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= htonl((__u32)iph->tos << IPV6_TCLASS_SHIFT)
- & IPV6_TCLASS_MASK;
+   dsfield = ipv4_get_dsfield(iph);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
fl6.flowi6_mark = skb->mark;
else
@@ -598,9 +597,11 @@ static inline int ip6gre_xmit_ipv6(struct sk_buff *skb, 
struct net_device *dev)
 
memcpy(, >fl.u.ip6, sizeof(fl6));
 
-   dsfield = ipv6_get_dsfield(ipv6h);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= (*(__be32 *) ipv6h & IPV6_TCLASS_MASK);
+   dsfield = ipv6_get_dsfield(ipv6h);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
+
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FLOWLABEL)
fl6.flowlabel |= ip6_flowlabel(ipv6h);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 6eb2ae5..7ae6c50 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1196,7 +1196,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev, __u8 dsfield,
skb_push(skb, sizeof(struct ipv6hdr));
skb_reset_network_header(skb);
ipv6h = ipv6_hdr(skb);
-   ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield),
+   ip6_flow_hdr(ipv6h, dsfield,
 ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6));
ipv6h->hop_limit = hop_limit;
ipv6h->nexthdr = proto;
@@ -1231,8 +1231,6 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
if (tproto != IPPROTO_IPIP && tproto != 0)
return -1;
 
-   dsfield = ipv4_get_dsfield(iph);
-
if (t->parms.collect_md) {
struct ip_tunnel_info *tun_info;
const struct ip_tunnel_key *key;
@@ -1246,6 +1244,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
fl6.flowi6_proto = IPPROTO_IPIP;
fl6.daddr = key->u.ipv6.dst;
fl6.flowlabel = key->label;
+   dsfield = ip6_tclass(key->label);
} else {
if (!(t->parms.flags & IP6_TNL_F_IGN_ENCAP_LIMIT))
encap_limit = t->parms.encap_limit;
@@ -1254,8 +1253,9 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
fl6.flowi6_proto = IPPROTO_IPIP;
 
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= htonl((__u32)iph->tos << 
IPV6_TCLASS_SHIFT)
-& IPV6_TCLASS_MASK;
+   dsfield = ipv4_get_dsfield(iph);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
fl6.flowi6_mark = skb->mark;
else
@@ -1267,6 +1267,8 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP6))
return -1;
 
+   dsfield = INET_ECN_encapsulate(dsf

[PATCH net,v2] ip6_tunnel, ip6_gre: fix setting of DSCP on encapsulated packets

2017-05-25 Thread Peter Dawson 
This fix addresses two problems in the way the DSCP field is formulated
 on the encapsulating header of IPv6 tunnels.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=195661

1) The IPv6 tunneling code was manipulating the DSCP field of the
 encapsulating packet using the 32b flowlabel. Since the flowlabel is
 only the lower 20b it was incorrect to assume that the upper 12b
 containing the DSCP and ECN fields would remain intact when formulating
 the encapsulating header. This fix handles the 'inherit' and
 'fixed-value' DSCP cases explicitly using the extant dsfield u8 variable.

2) The use of INET_ECN_encapsulate(0, dsfield) in ip6_tnl_xmit was
 incorrect and resulted in the DSCP value always being set to 0.

Commit 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class 
 is non-0") caused the regression by masking out the flowlabel
 which exposed the incorrect handling of the DSCP portion of the 
 flowlabel in ip6_tunnel and ip6_gre.

Fixes: 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class is non-0")
Signed-off-by: Peter Dawson 
---
 net/ipv6/ip6_gre.c| 13 +++--
 net/ipv6/ip6_tunnel.c | 21 +
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 8d128ba..0c5b4caa 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -537,11 +537,10 @@ static inline int ip6gre_xmit_ipv4(struct sk_buff *skb, 
struct net_device *dev)
 
memcpy(, >fl.u.ip6, sizeof(fl6));
 
-   dsfield = ipv4_get_dsfield(iph);
-
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= htonl((__u32)iph->tos << IPV6_TCLASS_SHIFT)
- & IPV6_TCLASS_MASK;
+   dsfield = ipv4_get_dsfield(iph);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
fl6.flowi6_mark = skb->mark;
else
@@ -598,9 +597,11 @@ static inline int ip6gre_xmit_ipv6(struct sk_buff *skb, 
struct net_device *dev)
 
memcpy(, >fl.u.ip6, sizeof(fl6));
 
-   dsfield = ipv6_get_dsfield(ipv6h);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= (*(__be32 *) ipv6h & IPV6_TCLASS_MASK);
+   dsfield = ipv6_get_dsfield(ipv6h);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
+
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FLOWLABEL)
fl6.flowlabel |= ip6_flowlabel(ipv6h);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 6eb2ae5..7ae6c50 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1196,7 +1196,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev, __u8 dsfield,
skb_push(skb, sizeof(struct ipv6hdr));
skb_reset_network_header(skb);
ipv6h = ipv6_hdr(skb);
-   ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield),
+   ip6_flow_hdr(ipv6h, dsfield,
 ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6));
ipv6h->hop_limit = hop_limit;
ipv6h->nexthdr = proto;
@@ -1231,8 +1231,6 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
if (tproto != IPPROTO_IPIP && tproto != 0)
return -1;
 
-   dsfield = ipv4_get_dsfield(iph);
-
if (t->parms.collect_md) {
struct ip_tunnel_info *tun_info;
const struct ip_tunnel_key *key;
@@ -1246,6 +1244,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
fl6.flowi6_proto = IPPROTO_IPIP;
fl6.daddr = key->u.ipv6.dst;
fl6.flowlabel = key->label;
+   dsfield = ip6_tclass(key->label);
} else {
if (!(t->parms.flags & IP6_TNL_F_IGN_ENCAP_LIMIT))
encap_limit = t->parms.encap_limit;
@@ -1254,8 +1253,9 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
fl6.flowi6_proto = IPPROTO_IPIP;
 
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= htonl((__u32)iph->tos << 
IPV6_TCLASS_SHIFT)
-& IPV6_TCLASS_MASK;
+   dsfield = ipv4_get_dsfield(iph);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
fl6.flowi6_mark = skb->mark;
else
@@ -1267,6 +1267,8 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP6))
return -1;
 
+   dsfield = INET_ECN_encapsulate(dsfield, ipv4_get_dsfield(iph));
+
skb_s

Re: [PATCH net] ip6_tunnel, ip6_gre: fix setting of DSCP on encapsulated packets

2017-05-25 Thread Peter Dawson 
On Thu, 25 May 2017 15:49:14 -0400 (EDT)
David Miller <da...@davemloft.net> wrote:

> Still not correct, you need to use a "Fixes: " tag of the form:
> 
> Fixes: 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class is non-0")
> 
> And it must appear of the first line of tags, before signoffs and acks,
> with no empty lines in between.

Here is my proposed commit message in full. Note that 
the "Fixes" line extends to 78 chars. Is this OK?
Thanks

This fix addresses two problems in the way the DSCP field is formulated
 on the encapsulating header of IPv6 tunnels.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=195661

1) The IPv6 tunneling code was manipulating the DSCP field of the
 encapsulating packet using the 32b flowlabel. Since the flowlabel is
 only the lower 20b it was incorrect to assume that the upper 12b
 containing the DSCP and ECN fields would remain intact when formulating
 the encapsulating header. This fix handles the 'inherit' and
 'fixed-value' DSCP cases explicitly using the extant dsfield u8 variable.

2) The use of INET_ECN_encapsulate(0, dsfield) in ip6_tnl_xmit was
 incorrect and resulted in the DSCP value always being set to 0.

Commit 90427ef5d2a4 caused the regression by masking out the flowlabel
 which exposed the incorrect the handling of the DSCP portion of the 
 flowlabel in ip6_tunnel and ip6_gre.

Fixes: 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class is non-0")
Signed-off-by: Peter Dawson <peter.a.daw...@boeing.com>

Re: [PATCH net] ip6_tunnel, ip6_gre: fix setting of DSCP on encapsulated packets

2017-05-25 Thread Peter Dawson 
On Thu, 25 May 2017 15:49:14 -0400 (EDT)
David Miller  wrote:

> Still not correct, you need to use a "Fixes: " tag of the form:
> 
> Fixes: 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class is non-0")
> 
> And it must appear of the first line of tags, before signoffs and acks,
> with no empty lines in between.

Here is my proposed commit message in full. Note that 
the "Fixes" line extends to 78 chars. Is this OK?
Thanks

This fix addresses two problems in the way the DSCP field is formulated
 on the encapsulating header of IPv6 tunnels.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=195661

1) The IPv6 tunneling code was manipulating the DSCP field of the
 encapsulating packet using the 32b flowlabel. Since the flowlabel is
 only the lower 20b it was incorrect to assume that the upper 12b
 containing the DSCP and ECN fields would remain intact when formulating
 the encapsulating header. This fix handles the 'inherit' and
 'fixed-value' DSCP cases explicitly using the extant dsfield u8 variable.

2) The use of INET_ECN_encapsulate(0, dsfield) in ip6_tnl_xmit was
 incorrect and resulted in the DSCP value always being set to 0.

Commit 90427ef5d2a4 caused the regression by masking out the flowlabel
 which exposed the incorrect the handling of the DSCP portion of the 
 flowlabel in ip6_tunnel and ip6_gre.

Fixes: 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class is non-0")
Signed-off-by: Peter Dawson

Re: [PATCH net] ip6_tunnel, ip6_gre: fix setting of DSCP on encapsulated packets

2017-05-25 Thread Peter Dawson 
On Thu, 25 May 2017 12:11:17 -0400 (EDT)
David Miller  wrote:

> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=195661  
> 
> This is not the correct way to use the Fixes: tag.
> 
> You should specify the commit that introduced the regression
> between 4.9.x and 4.10.x, and that you are fixing here.

Thanks for your review Dave. I'll resubmit the patch with the following detail 
and just reference the bugzilla in the main body of the commit comment.

Commit 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class is non-0") 
caused the regression by masking out the flowlabel which exposed the incorrect 
the handling of the DSCP portion of the flowlabel in ip6_tunnel and ip6_gre.

Re: [PATCH net] ip6_tunnel, ip6_gre: fix setting of DSCP on encapsulated packets

2017-05-25 Thread Peter Dawson 
On Thu, 25 May 2017 12:11:17 -0400 (EDT)
David Miller  wrote:

> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=195661  
> 
> This is not the correct way to use the Fixes: tag.
> 
> You should specify the commit that introduced the regression
> between 4.9.x and 4.10.x, and that you are fixing here.

Thanks for your review Dave. I'll resubmit the patch with the following detail 
and just reference the bugzilla in the main body of the commit comment.

Commit 90427ef5d2a4 ("ipv6: fix flow labels when the traffic class is non-0") 
caused the regression by masking out the flowlabel which exposed the incorrect 
the handling of the DSCP portion of the flowlabel in ip6_tunnel and ip6_gre.

[PATCH net] ip6_tunnel, ip6_gre: fix setting of DSCP on encapsulated packets

2017-05-22 Thread Peter Dawson 
This fix addresses two problems in the way the DSCP field is formulated
 on the encapsulating header of IPv6 tunnels.

1) The IPv6 tunneling code was manipulating the DSCP field of the
 encapsulating packet using the 32b flowlabel. Since the flowlabel is
 only the lower 20b it was incorrect to assume that the upper 12b
 containing the DSCP and ECN fields would remain intact when formulating
 the encapsulating header. This fix handles the 'inherit' and
 'fixed-value' DSCP cases explicitly using the extant dsfield u8 variable.

2) The use of INET_ECN_encapsulate(0, dsfield) in ip6_tnl_xmit was
 incorrect and resulted in the DSCP value always being set to 0.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=195661
Signed-off-by: Peter Dawson <peter.a.daw...@boeing.com>
---
 net/ipv6/ip6_gre.c| 13 +++--
 net/ipv6/ip6_tunnel.c | 21 +
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 8d128ba..0c5b4caa 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -537,11 +537,10 @@ static inline int ip6gre_xmit_ipv4(struct sk_buff *skb, 
struct net_device *dev)
 
memcpy(, >fl.u.ip6, sizeof(fl6));
 
-   dsfield = ipv4_get_dsfield(iph);
-
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= htonl((__u32)iph->tos << IPV6_TCLASS_SHIFT)
- & IPV6_TCLASS_MASK;
+   dsfield = ipv4_get_dsfield(iph);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
fl6.flowi6_mark = skb->mark;
else
@@ -598,9 +597,11 @@ static inline int ip6gre_xmit_ipv6(struct sk_buff *skb, 
struct net_device *dev)
 
memcpy(, >fl.u.ip6, sizeof(fl6));
 
-   dsfield = ipv6_get_dsfield(ipv6h);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= (*(__be32 *) ipv6h & IPV6_TCLASS_MASK);
+   dsfield = ipv6_get_dsfield(ipv6h);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
+
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FLOWLABEL)
fl6.flowlabel |= ip6_flowlabel(ipv6h);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 6eb2ae5..7ae6c50 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1196,7 +1196,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev, __u8 dsfield,
skb_push(skb, sizeof(struct ipv6hdr));
skb_reset_network_header(skb);
ipv6h = ipv6_hdr(skb);
-   ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield),
+   ip6_flow_hdr(ipv6h, dsfield,
 ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6));
ipv6h->hop_limit = hop_limit;
ipv6h->nexthdr = proto;
@@ -1231,8 +1231,6 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
if (tproto != IPPROTO_IPIP && tproto != 0)
return -1;
 
-   dsfield = ipv4_get_dsfield(iph);
-
if (t->parms.collect_md) {
struct ip_tunnel_info *tun_info;
const struct ip_tunnel_key *key;
@@ -1246,6 +1244,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
fl6.flowi6_proto = IPPROTO_IPIP;
fl6.daddr = key->u.ipv6.dst;
fl6.flowlabel = key->label;
+   dsfield = ip6_tclass(key->label);
} else {
if (!(t->parms.flags & IP6_TNL_F_IGN_ENCAP_LIMIT))
encap_limit = t->parms.encap_limit;
@@ -1254,8 +1253,9 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
fl6.flowi6_proto = IPPROTO_IPIP;
 
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= htonl((__u32)iph->tos << 
IPV6_TCLASS_SHIFT)
-& IPV6_TCLASS_MASK;
+   dsfield = ipv4_get_dsfield(iph);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
fl6.flowi6_mark = skb->mark;
else
@@ -1267,6 +1267,8 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP6))
return -1;
 
+   dsfield = INET_ECN_encapsulate(dsfield, ipv4_get_dsfield(iph));
+
skb_set_inner_ipproto(skb, IPPROTO_IPIP);
 
err = ip6_tnl_xmit(skb, dev, dsfield, , encap_limit, ,
@@ -1300,8 +1302,6 @@ ip6ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
ip6_tnl_addr_conflict(t, ipv6h))
return -1;
 
-   dsfield = ipv6_get_dsfield(ipv6h);

[PATCH net] ip6_tunnel, ip6_gre: fix setting of DSCP on encapsulated packets

2017-05-22 Thread Peter Dawson 
This fix addresses two problems in the way the DSCP field is formulated
 on the encapsulating header of IPv6 tunnels.

1) The IPv6 tunneling code was manipulating the DSCP field of the
 encapsulating packet using the 32b flowlabel. Since the flowlabel is
 only the lower 20b it was incorrect to assume that the upper 12b
 containing the DSCP and ECN fields would remain intact when formulating
 the encapsulating header. This fix handles the 'inherit' and
 'fixed-value' DSCP cases explicitly using the extant dsfield u8 variable.

2) The use of INET_ECN_encapsulate(0, dsfield) in ip6_tnl_xmit was
 incorrect and resulted in the DSCP value always being set to 0.

Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=195661
Signed-off-by: Peter Dawson 
---
 net/ipv6/ip6_gre.c| 13 +++--
 net/ipv6/ip6_tunnel.c | 21 +
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 8d128ba..0c5b4caa 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -537,11 +537,10 @@ static inline int ip6gre_xmit_ipv4(struct sk_buff *skb, 
struct net_device *dev)
 
memcpy(, >fl.u.ip6, sizeof(fl6));
 
-   dsfield = ipv4_get_dsfield(iph);
-
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= htonl((__u32)iph->tos << IPV6_TCLASS_SHIFT)
- & IPV6_TCLASS_MASK;
+   dsfield = ipv4_get_dsfield(iph);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
fl6.flowi6_mark = skb->mark;
else
@@ -598,9 +597,11 @@ static inline int ip6gre_xmit_ipv6(struct sk_buff *skb, 
struct net_device *dev)
 
memcpy(, >fl.u.ip6, sizeof(fl6));
 
-   dsfield = ipv6_get_dsfield(ipv6h);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= (*(__be32 *) ipv6h & IPV6_TCLASS_MASK);
+   dsfield = ipv6_get_dsfield(ipv6h);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
+
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FLOWLABEL)
fl6.flowlabel |= ip6_flowlabel(ipv6h);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 6eb2ae5..7ae6c50 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1196,7 +1196,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev, __u8 dsfield,
skb_push(skb, sizeof(struct ipv6hdr));
skb_reset_network_header(skb);
ipv6h = ipv6_hdr(skb);
-   ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield),
+   ip6_flow_hdr(ipv6h, dsfield,
 ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6));
ipv6h->hop_limit = hop_limit;
ipv6h->nexthdr = proto;
@@ -1231,8 +1231,6 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
if (tproto != IPPROTO_IPIP && tproto != 0)
return -1;
 
-   dsfield = ipv4_get_dsfield(iph);
-
if (t->parms.collect_md) {
struct ip_tunnel_info *tun_info;
const struct ip_tunnel_key *key;
@@ -1246,6 +1244,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
fl6.flowi6_proto = IPPROTO_IPIP;
fl6.daddr = key->u.ipv6.dst;
fl6.flowlabel = key->label;
+   dsfield = ip6_tclass(key->label);
} else {
if (!(t->parms.flags & IP6_TNL_F_IGN_ENCAP_LIMIT))
encap_limit = t->parms.encap_limit;
@@ -1254,8 +1253,9 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
fl6.flowi6_proto = IPPROTO_IPIP;
 
if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-   fl6.flowlabel |= htonl((__u32)iph->tos << 
IPV6_TCLASS_SHIFT)
-& IPV6_TCLASS_MASK;
+   dsfield = ipv4_get_dsfield(iph);
+   else
+   dsfield = ip6_tclass(t->parms.flowinfo);
if (t->parms.flags & IP6_TNL_F_USE_ORIG_FWMARK)
fl6.flowi6_mark = skb->mark;
else
@@ -1267,6 +1267,8 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
if (iptunnel_handle_offloads(skb, SKB_GSO_IPXIP6))
return -1;
 
+   dsfield = INET_ECN_encapsulate(dsfield, ipv4_get_dsfield(iph));
+
skb_set_inner_ipproto(skb, IPPROTO_IPIP);
 
err = ip6_tnl_xmit(skb, dev, dsfield, , encap_limit, ,
@@ -1300,8 +1302,6 @@ ip6ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev)
ip6_tnl_addr_conflict(t, ipv6h))
return -1;
 
-   dsfield = ipv6_get_dsfield(ipv6h);
-
if (t->parms.collect