[PATCH] ipv6: address issue in __ip6_append_data

[PATCH] ipv6: address issue in __ip6_append_data

[Madalin Bucur]

Hello,

I think I've found a problem that allows generic IPv6 traffic to be
sent by the stack with CHECKSUM_PARTIAL to a netdevice that declares
NETIF_F_IPV6_CSUM. The NETIF_F_IPV6_CSUM flag is based on the
NETIF_F_IPV6_CSUM_BIT that is described as referring only to TCP and
UDP but in my test ICMPv6 frames with CHECKSUM_PARTIAL are seen:

NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */

I've traced the issue to a recent commit that includes this check:

	rt->dst.dev->features & NETIF_F_V6_CSUM

The problem with this is that NETIF_F_V6_CSUM is more than one bit:

#define NETIF_F_V6_CSUM         (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)

Thus the above check should be either:

	(rt->dst.dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM

or probably should use NETIF_F_HW_CSUM only:

	rt->dst.dev->features & NETIF_F_HW_CSUM

The mentioned commit is 32dce968dd987adfb0c00946d78dad9154f64759

> Author: Vlad Yasevich <vyasevich@gmail.com>
> Date:   Sat Jan 31 10:40:18 2015 -0500
>
>	ipv6: Allow for partial checksums on non-ufo packets
>
>	Currntly, if we are not doing UFO on the packet, all UDP
>	packets will start with CHECKSUM_NONE and thus perform full
>	checksum computations in software even if device support
>	IPv6 checksum offloading.

>	Let's start start with CHECKSUM_PARTIAL if the device
>	supports it and we are sending only a single packet at
>	or below mtu size.

>	Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>	Signed-off-by: David S. Miller <davem@davemloft.net>

Reverting this commit solved the issue, then the changes below were
validated as well. The problem was evidentiated by running ping6
but it should be visible with any IPv6 payload that it is not UDP
nor TCP.

Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
---
 net/ipv6/ip6_output.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index d33df4c..7177f63 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1275,7 +1275,7 @@ emsgsize:
 	 */
 	if (!skb &&
 	    length + fragheaderlen < mtu &&
-	    rt->dst.dev->features & NETIF_F_V6_CSUM &&
+	    (rt->dst.dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM &&
 	    !exthdrlen)
 		csummode = CHECKSUM_PARTIAL;
 	/*
-- 
1.7.11.7

Re: [PATCH] ipv6: address issue in __ip6_append_data

[David Miller]

From: Madalin Bucur <madalin.bucur@freescale.com>
Date: Fri, 13 Feb 2015 17:30:59 +0200

> I think I've found a problem that allows generic IPv6 traffic to be
> sent by the stack with CHECKSUM_PARTIAL to a netdevice that declares
> NETIF_F_IPV6_CSUM. The NETIF_F_IPV6_CSUM flag is based on the
> NETIF_F_IPV6_CSUM_BIT that is described as referring only to TCP and
> UDP but in my test ICMPv6 frames with CHECKSUM_PARTIAL are seen:
> 
> NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */
> 
> I've traced the issue to a recent commit that includes this check:
> 
> 	rt->dst.dev->features & NETIF_F_V6_CSUM
> 
> The problem with this is that NETIF_F_V6_CSUM is more than one bit:
> 
> #define NETIF_F_V6_CSUM         (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
> 
> Thus the above check should be either:
> 
> 	(rt->dst.dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM
> 
> or probably should use NETIF_F_HW_CSUM only:
> 
> 	rt->dst.dev->features & NETIF_F_HW_CSUM
> 
> The mentioned commit is 32dce968dd987adfb0c00946d78dad9154f64759

Vlad, please review this fix.

Thanks.

>> Author: Vlad Yasevich <vyasevich@gmail.com>
>> Date:   Sat Jan 31 10:40:18 2015 -0500
>>
>>	ipv6: Allow for partial checksums on non-ufo packets
>>
>>	Currntly, if we are not doing UFO on the packet, all UDP
>>	packets will start with CHECKSUM_NONE and thus perform full
>>	checksum computations in software even if device support
>>	IPv6 checksum offloading.
> 
>>	Let's start start with CHECKSUM_PARTIAL if the device
>>	supports it and we are sending only a single packet at
>>	or below mtu size.
> 
>>	Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>	Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Reverting this commit solved the issue, then the changes below were
> validated as well. The problem was evidentiated by running ping6
> but it should be visible with any IPv6 payload that it is not UDP
> nor TCP.
> 
> Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
> ---
>  net/ipv6/ip6_output.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index d33df4c..7177f63 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1275,7 +1275,7 @@ emsgsize:
>  	 */
>  	if (!skb &&
>  	    length + fragheaderlen < mtu &&
> -	    rt->dst.dev->features & NETIF_F_V6_CSUM &&
> +	    (rt->dst.dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM &&
>  	    !exthdrlen)
>  		csummode = CHECKSUM_PARTIAL;
>  	/*
> -- 
> 1.7.11.7
> 

Re: [PATCH] ipv6: address issue in __ip6_append_data

[Sabrina Dubroca]

2015-02-13, 17:30:59 +0200, Madalin Bucur wrote:
> Hello,
> 
> I think I've found a problem that allows generic IPv6 traffic to be
> sent by the stack with CHECKSUM_PARTIAL to a netdevice that declares
> NETIF_F_IPV6_CSUM. The NETIF_F_IPV6_CSUM flag is based on the
> NETIF_F_IPV6_CSUM_BIT that is described as referring only to TCP and
> UDP but in my test ICMPv6 frames with CHECKSUM_PARTIAL are seen:

This should be fixed by:

    bf250a1fa769 ("ipv6: Partial checksum only UDP packets")

Have you tested it?


> NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */
> 
> I've traced the issue to a recent commit that includes this check:
> 
> 	rt->dst.dev->features & NETIF_F_V6_CSUM
> 
> The problem with this is that NETIF_F_V6_CSUM is more than one bit:
> 
> #define NETIF_F_V6_CSUM         (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
> 
> Thus the above check should be either:
> 
> 	(rt->dst.dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM

I think this disables HW checksumming that Vlad's patch enabled.  As
Vlad pointed out in another similar thread [1], the two bits are
mutually exclusive, so you can't have

    (dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM

[1] http://www.spinics.net/lists/netdev/msg316341.html


> or probably should use NETIF_F_HW_CSUM only:
> 
> 	rt->dst.dev->features & NETIF_F_HW_CSUM

Maybe.  But I just tried this, and it doesn't work with qemu's e1000
emulation (could be a driver/qemu problem, I can't check with other
devices).


Thanks,
-- 
Sabrina

RE: [PATCH] ipv6: address issue in __ip6_append_data

[Madalin-Cristian Bucur]

> From: Sabrina Dubroca [mailto:sd@queasysnail.net]
> Sent: Saturday, February 21, 2015 8:34 PM
> 2015-02-13, 17:30:59 +0200, Madalin Bucur wrote:
> > Hello,
> >
> > I think I've found a problem that allows generic IPv6 traffic to be
> > sent by the stack with CHECKSUM_PARTIAL to a netdevice that declares
> > NETIF_F_IPV6_CSUM. The NETIF_F_IPV6_CSUM flag is based on the
> > NETIF_F_IPV6_CSUM_BIT that is described as referring only to TCP and
> > UDP but in my test ICMPv6 frames with CHECKSUM_PARTIAL are seen:
> 
> This should be fixed by:
> 
>     bf250a1fa769 ("ipv6: Partial checksum only UDP packets")
> 
> Have you tested it?

I've sampled the tree between the problem was introduced and your fix was applied.
Next I've checked for the rt->dst.dev->features & NETIF_F_V6_CSUM that was still
there and sent the fix I had prepared locally. Your patch restricts CHECKSUM_PARTIAL
to UDP and thus solved the problem. 

> > NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */
> >
> > I've traced the issue to a recent commit that includes this check:
> >
> > 	rt->dst.dev->features & NETIF_F_V6_CSUM
> >
> > The problem with this is that NETIF_F_V6_CSUM is more than one bit:
> >
> > #define NETIF_F_V6_CSUM         (NETIF_F_GEN_CSUM |
> NETIF_F_IPV6_CSUM)
> >
> > Thus the above check should be either:
> >
> > 	(rt->dst.dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM
> 
> I think this disables HW checksumming that Vlad's patch enabled.  As
> Vlad pointed out in another similar thread [1], the two bits are
> mutually exclusive, so you can't have
> 
>     (dev->features & NETIF_F_V6_CSUM) == NETIF_F_V6_CSUM
> 
> [1] http://www.spinics.net/lists/netdev/msg316341.html
> 

I was unsure about Vlad's intention, the fact that his patch introduced the problem
made me stop at this quick explanation as the code looked suspicious.

> > or probably should use NETIF_F_HW_CSUM only:
> >
> > 	rt->dst.dev->features & NETIF_F_HW_CSUM
> 
> Maybe.  But I just tried this, and it doesn't work with qemu's e1000
> emulation (could be a driver/qemu problem, I can't check with other
> devices).

My concern was to stop receiving ICMPv6 with CHECKSUM_PARTIAL on a driver that 
declared NETIF_F_IPV6_CSUM only (not NETIF_F_HW_CSUM) so I could not have
validated this second approach on my hw.

Thank you for your fix and reply,
Madalin

PS: sorry for the late reply, I've been away last week