| Message ID | 20130815192046.GA11788@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 08/15/2013 03:20 PM, Michael S. Tsirkin wrote: > 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? > No, this is really no different then what I had to start with. > --> > > 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; This wouldn't work when tap_features = 0, since the skb features in forward would evaluate to 0. This means that netif_needs_gso() will return false and a GSO packet will be queued to the socket without going though segmentation. -vlad > vlan->set_features = features; > netdev_update_features(vlan->dev); > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 15, 2013 at 03:26:24PM -0400, Vlad Yasevich wrote: > On 08/15/2013 03:20 PM, Michael S. Tsirkin wrote: > >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? > > > > No, this is really no different then what I had to start with. > > >--> > > > >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; > > This wouldn't work when tap_features = 0, since the skb features in > forward would evaluate to 0. This means that netif_needs_gso() will > return false and a GSO packet will be queued to the socket without > going though segmentation. > > -vlad > Hmm won't same thing happen if features == 0 on lowerdev? > > vlan->set_features = features; > > netdev_update_features(vlan->dev); > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);