Re: [PATCH v3 1/2] macvtap: include all checksum offloads in TUN_OFFLOAD mask

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Aug 15, 2013 at 10:09:45PM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 15, 2013 at 02:45:30PM -0400, Vlad Yasevich wrote:
> > On 08/15/2013 02:24 PM, Michael S. Tsirkin wrote:
> > >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@redhat.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.
> > 
> > Yes.  I thought about that, but forgot to do in this patch set.
> 
> In fact I wonder why do we do it like this at all.
> tap_features have nothing to do with the device,
> they only have to do with tap.
> For example, you might have a dumb device with no
> checksum support but userspace said it does not need a checksum,
> we can have NETIF_F_HW_CSUM there anyway.
> 
> So why don't we just
>         vlan->tap_features = feature_mask;
> ?
> In fact, this is exactly set_features, so why don't
> we just use set_features everywhere?
> Then this patch won't be needed at all.
> 
> Kind of like this - compile-tested only - do you have a setup with
> IP_CSUM which you can use to test?

Ugh this does not work even on a simple setup, sorry about the noise.
Maybe like this? Does this work for you?

-->

macvtap: fix up tap features

There's no apparent need to have tap features
masked according to the lowerdev config:
we only use them in software so hardware configuration
does not matter.

This patch fixes bugs for some unusual hardware configurations,
for example, we only masked HW_CSUM so for hardware with
e.g. IP_CSUM the checksum bit remained set, which would cause
packets with CHECKSUM_PARTIAL to be sent to userspace
even when offloads are disabled.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..3c18f12 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -1058,8 +1058,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
 	/* tap_features are the same as features on tun/tap and
 	 * reflect user expectations.
 	 */
-	vlan->tap_features = vlan->dev->features &
-			    (feature_mask | ~TUN_OFFLOADS);
+	vlan->tap_features = feature_mask;
 	vlan->set_features = features;
 	netdev_update_features(vlan->dev);