| 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:09:45 +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 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@...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.
>
> 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?
-->
macvtap: fix up tap features
There's no apparent need to have separate tap features
masked according to the lowerdev config:
we only use them in software so hardware configuration
does not matter.
Drop tap_features and use set_features directly
This 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@...hat.com>
---
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index a98ed9f..26cea91 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -289,7 +289,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 = netif_skb_features(skb) & vlan->set_features;
if (netif_needs_gso(skb, features)) {
struct sk_buff *segs = __skb_gso_segment(skb, features, false);
@@ -382,11 +382,6 @@ static int macvtap_newlink(struct net *src_net,
struct macvlan_dev *vlan = netdev_priv(dev);
INIT_LIST_HEAD(&vlan->queue_list);
- /* Since macvlan supports all offloads by default, make
- * tap support all offloads also.
- */
- vlan->tap_features = TUN_OFFLOADS;
-
/* Don't put anything that may fail after macvlan_common_newlink
* because we can't undo what it does.
*/
@@ -1027,7 +1022,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
features = vlan->dev->features;
if (arg & TUN_F_CSUM) {
- feature_mask = NETIF_F_HW_CSUM;
+ feature_mask = NETIF_F_HW_CSUM;
if (arg & (TUN_F_TSO4 | TUN_F_TSO6)) {
if (arg & TUN_F_TSO_ECN)
@@ -1055,11 +1050,6 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
else
features &= ~RX_OFFLOADS;
- /* 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->set_features = features;
netdev_update_features(vlan->dev);
diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
index ddd33fd..e446e82 100644
--- a/include/linux/if_macvlan.h
+++ b/include/linux/if_macvlan.h
@@ -76,7 +76,6 @@ struct macvlan_dev {
struct list_head queue_list;
int numvtaps;
int numqueues;
- netdev_features_t tap_features;
int minor;
};
--
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