| 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
| ||
|
Date: Thu, 15 Aug 2013 22:27:04 +0300
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Vlad Yasevich <vyasevic@...hat.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH v3 2/2] macvtap: Allow tap features change when
IFF_VNET_HDR is disabled.
On Thu, Aug 15, 2013 at 02:59:01PM -0400, Vlad Yasevich wrote:
> On 08/15/2013 02:48 PM, Michael S. Tsirkin wrote:
> >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 <vyasevic@...hat.com>
> >>>>---
> >>>> 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
> >
>
> But unused since since VNET=0.
Well, I think that's true but it would still be better
to disable RX offloads, no?
> I am trying to be careful not to
> break things when ioctl calls get all inverted and reversed, but
> may be I am trying too hard and we should just let things break.
What exactly will break by call to netdev_update_features?
> I just feels kinds of silly since the app may now know all the
> details, but the kernel does and can do the "right thing".
>
> -vlad
>
> >
> >>>> /*
> >>>> * 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
--
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
Powered by blists - more mailing lists