From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v3 2/2] macvtap: Allow tap features change when IFF_VNET_HDR is disabled. Date: Thu, 15 Aug 2013 21:48:44 +0300 Message-ID: <20130815184844.GB11350@redhat.com> References: <1376586173-4242-1-git-send-email-vyasevic@redhat.com> <1376586173-4242-3-git-send-email-vyasevic@redhat.com> <20130815173347.GA10265@redhat.com> <520D206B.3000203@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org To: Vlad Yasevich Return-path: Received: from mx1.redhat.com ([209.132.183.28]:32060 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756785Ab3HOSq6 (ORCPT ); Thu, 15 Aug 2013 14:46:58 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7FIkw1r024528 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 15 Aug 2013 14:46:58 -0400 Content-Disposition: inline In-Reply-To: <520D206B.3000203@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Aug 15, 2013 at 02:39:39PM -0400, Vlad Yasevich wrote: > On 08/15/2013 01:33 PM, Michael S. Tsirkin wrote: > >On Thu, Aug 15, 2013 at 01:02:53PM -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. Also, take the setting of IFF_VNET_HDR into consideration > >>when determining the feature set to apply to incommig traffic. > >>If IFF_VNET_HDR is set, we use user-specfied feature set. if the > >>IFF_VNET_HDR is clear, we mask off all tun-specific offload features, > >>since user can not be notified of the possbile offloads. > >> > >>Signed-off-by: Vlad Yasevich > >>--- > >> drivers/net/macvtap.c | 30 ++++++++++++++++++++++++------ > >> 1 file changed, 24 insertions(+), 6 deletions(-) > >> > >>diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c > >>index 8121358..df45a59 100644 > >>--- a/drivers/net/macvtap.c > >>+++ b/drivers/net/macvtap.c > >>@@ -269,6 +269,28 @@ static void macvtap_del_queues(struct net_device *dev) > >> sock_put(&qlist[j]->sk); > >> } > >> > >>+/* Determine features to be applied to the packet as it > > > >/* > > * Please use comments > > * like this > > */ > > > >>+ * gets sent to the tap. The features depend on the offloads > >>+ * set by user and by whether or not IFF_VNET_HDR is enabled. > >>+ */ > >>+static netdev_features_t macvtap_skb_features(struct sk_buff *skb, > >>+ struct macvtap_queue *q) > >>+{ > >>+ struct macvlan_dev *vlan = netdev_priv(skb->dev); > >>+ netdev_features_t features = netif_skb_features(skb); > >>+ > >>+ /* If VNET_HDR is enabled, user can receive offload info so > >>+ * use user-specified tap_features. > >>+ * Otherwise, mask off all tap offload features. > >>+ */ > >>+ if (q->flags & IFF_VNET_HDR) > >>+ features &= vlan->tap_features; > >>+ else > >>+ features &= ~TUN_OFFLOADS; > >>+ > >>+ return features; > >>+} > >>+ > > > >Okay, but this means we need to do netdev_update_features > >on SETIFF since it changes q->flags. > > Why? > The whole point it not to change features on SETIFF, so that > they are preserved for when the flags change again. It just that > flag values either allow whatever offload user specified (correctly > masked via netdev_update_features) or turn off all offloads at the tap. > > -vlad That's the point. As it is, if flag value changes, we don't call netdev_update_features so offloads aren't masked/unmasked. Example: 1. SETOFFLOADS to enable 2. VNET = 0 offloads are still enabled > >> /* > >> * Forward happens for data that gets sent from one macvlan > >> * endpoint to another one in bridge mode. We just take > >>@@ -276,9 +298,9 @@ static void macvtap_del_queues(struct net_device *dev) > >> */ > >> static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) > >> { > >>- struct macvlan_dev *vlan = netdev_priv(dev); > >> struct macvtap_queue *q = macvtap_get_queue(dev, skb); > >> netdev_features_t features; > >>+ > >> if (!q) > >> goto drop; > >> > >>@@ -289,7 +311,7 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb) > >> /* Apply the forward feature mask so that we perform segmentation > >> * according to users wishes. > >> */ > >>- features = netif_skb_features(skb) & vlan->tap_features; > >>+ features = macvtap_skb_features(skb, q); > >> if (netif_needs_gso(skb, features)) { > >> struct sk_buff *segs = __skb_gso_segment(skb, features, false); > >> > >>@@ -1161,10 +1183,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