Re: [PATCH] macvtap: Correctly set tap features when IFF_VNET_HDR is disabled.

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

On Wed, Aug 14, 2013 at 12:04:22PM -0400, Vlad Yasevich wrote:
> On 08/14/2013 10:07 AM, Vlad Yasevich wrote:
> >On 08/14/2013 08:10 AM, Michael S. Tsirkin wrote:
> >>On Tue, Aug 13, 2013 at 01:55:58PM -0400, Vlad Yasevich wrote:
> >>>When the user turns off IFF_VNET_HDR flag, attempts to change
> >>>offload features via TUNSETOFFLOAD do not work.  This could cause
> >>>GSO packets to be delivered to the user when the user is
> >>>not prepared to handle them.
> >>>
> >>>To solve, allow processing of TUNSETOFFLOAD when IFF_VNET_HDR is
> >>>disabled and make sure to turn off all offloads in this case.
> >>>Also, when IFF_VNET_HDR is disabled, run throught the offload change
> >>>as well to make sure that the functionality is not dependent on
> >>>the order of the ioclt() calls.
> >>>
> >>>Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >>
> >>Hmm, this seems to make it even messier.
> >>So
> >>    set TUNSETOFFLOAD
> >>    clear IFF_VNET_HDR
> >>    set IFF_VNET_HDR
> >>
> >>offloads are now clear?
> >>
> >>I'm also not sure userspace that sets offloads but clears
> >>vnet hdr knows what it's doing.
> >>
> >>How about we fail attempts to clear vnet hdr
> >>unless offloads are disabled?
> >>
> >
> >OK.  That makes it simpler...
> 
> Spoke too soon.
> 
> What happens is that libvirt is typically the one that turns off
> IFF_VNET_HDR, and qemu controls the offloads.
> 
> So, if we forbid turning off vnet_hdr while offloads are set, that
> will break libvirt (this is because macvtap defaults to tap offloads
> being enabled).
> 
> -vlad

Well what prevents libvirt from disabling offloads?
Anyway, another approach is to say that this is up to user -
if user disables hdr but not offloads, user will get
packets without a checksum.



> >
> >-vlad
> >
> >>
> >>
> >>
> >>>---
> >>>  drivers/net/macvtap.c | 27 +++++++++++++++++++--------
> >>>  1 file changed, 19 insertions(+), 8 deletions(-)
> >>>
> >>>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >>>index a98fb0e..076b9e7 100644
> >>>--- a/drivers/net/macvtap.c
> >>>+++ b/drivers/net/macvtap.c
> >>>@@ -1019,6 +1019,7 @@ static int set_offload(struct macvtap_queue *q,
> >>>unsigned long arg)
> >>>      struct macvlan_dev *vlan;
> >>>      netdev_features_t features;
> >>>      netdev_features_t feature_mask = 0;
> >>>+    netdev_features_t tap_mask = TUN_OFFLOADS;
> >>>
> >>>      vlan = rtnl_dereference(q->vlan);
> >>>      if (!vlan)
> >>>@@ -1026,7 +1027,12 @@ static int set_offload(struct macvtap_queue
> >>>*q, unsigned long arg)
> >>>
> >>>      features = vlan->dev->features;
> >>>
> >>>-    if (arg & TUN_F_CSUM) {
> >>>+    if (!(q->flags & IFF_VNET_HDR)) {
> >>>+        /* Turn off all checsum offloading also if user does
> >>>+         * not user vnet_hdr.
> >>
> >>Does not set?
> >>
> >>>+         */
> >>>+        tap_mask |= NETIF_F_ALL_CSUM;
> >>>+    } else if (arg & TUN_F_CSUM) {
> >>>          feature_mask = NETIF_F_HW_CSUM;
> >>>
> >>>          if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
> >>>@@ -1058,8 +1064,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 = vlan->dev->features & (feature_mask |
> >>>~tap_mask);
> >>>      vlan->set_features = features;
> >>>      netdev_update_features(vlan->dev);
> >>>
> >>>@@ -1092,8 +1097,18 @@ static long macvtap_ioctl(struct file *file,
> >>>unsigned int cmd,
> >>>          if ((u & ~(IFF_VNET_HDR | IFF_MULTI_QUEUE)) !=
> >>>              (IFF_NO_PI | IFF_TAP))
> >>>              ret = -EINVAL;
> >>>-        else
> >>>+        else {
> >>>+            if ((q->flags ^ u) & IFF_VNET_HDR) {
> >>>+                /* vnet_hdr support impacts the offloads,
> >>>+                 * so we need to run throught the offload
> >>
> >>throught ?
> >>
> >>>+                 * change.
> >>>+                 */
> >>>+                rtnl_lock();
> >>>+                ret = set_offload(q, 0);
> >>>+                rtnl_unlock();
> >>>+            }
> >>>              q->flags = u;
> >>>+        }
> >>>
> >>>          return ret;
> >>>
> >>>@@ -1155,10 +1170,6 @@ static long macvtap_ioctl(struct file *file,
> >>>unsigned int cmd,
> >>>                  TUN_F_TSO_ECN | TUN_F_UFO))
> >>>              return -EINVAL;
> >>>
> >>>-        /* TODO: only accept frames with the features that
> >>>-             got enabled for forwarded frames */
> >>>-        if (!(q->flags & IFF_VNET_HDR))
> >>>-            return  -EINVAL;
> >>>          rtnl_lock();
> >>>          ret = set_offload(q, arg);
> >>>          rtnl_unlock();
> >>>--
> >>>1.8.1.4
> >