netdev - Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130815182430.GB10265@redhat.com>
Date:	Thu, 15 Aug 2013 21:24:31 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Vlad Yasevich <vyasevic@...hat.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH v3 1/2] macvtap: include all checksum offloads in
 TUN_OFFLOAD mask

On Thu, Aug 15, 2013 at 01:02:52PM -0400, Vlad Yasevich wrote:
> The features of the macvlan are based on the features of lower
> device and thus can have checksum featurs other then IFF_F_HW_CSUM

s/featurs/features/

:set spell spelllang=en_us

or whatever's the equivalent in your editor of choice.

> set.  However, TUN_OFFLOAD mask only includes IFF_F_HW_CSUM.  Thus
> when performing gso segmentation during macvtap_forward(),
> it is possbile to end up with skbs that have ip_summed set
> to CHECKSUM_PARTIAL.  This is incorrect when the user
> turns off checksum offloading.
> 
> Include all possible checksum offload values so that
> we'll properly mask them off when performing GSO.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@...hat.com>
> ---
>  drivers/net/macvtap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index b51db2a..8121358 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
>  
>  static const struct proto_ops macvtap_socket_ops;
>  
> -#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> +#define TUN_OFFLOADS (NETIF_F_ALL_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
>  		      NETIF_F_TSO6 | NETIF_F_UFO)
>  #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
>  /*

Okay so you are talking about hardware that sets some other
checksum bit besides HW_CSUM, e.g. IP_CSUM, so

        vlan->tap_features = vlan->dev->features &
                            (feature_mask | ~TUN_OFFLOADS);

will not clear IP_CSUM even if feature_mask is 0.

Maybe mention this in the changelog, in case user
will wonder whether his hardware is affected.

So I agree, that's a bug, but if you make this change the reverse will hold
(on this hardware):
if user sets TUN_F_CSUM, we won't set NETIF_F_IP_CSUM
so checksum offloading won't work.

So I think you want s/NETIF_F_HW_CSUM/NETIF_F_ALL_CSUM/ everywhere
and not just in this place.

Also - Cc stable?

> -- 
> 1.8.1.4
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html