Linux Virtualization - Nov 2018 - [PATCH net 2/2] virtio-net: fail XDP set if guest csum is negotiated

We don't disable VIRTIO_NET_F_GUEST_CSUM if XDP was set. This means we
can receive partial csumed packets with metadata kept in the
vnet_hdr. This may have several side effects:

- It could be overridden by header adjustment, thus is might be not
  correct after XDP processing.
- There's no way to pass such metadata information through
  XDP_REDIRECT to another driver.
- XDP does not support checksum offload right now.

So simply disable guest csum if possible in this the case of XDP.

Fixes: 3f93522ffab2d ("virtio-net: switch off offloads on demand if
possible on XDP set")
Reported-by: Jesper Dangaard Brouer <brouer at redhat.com>
Cc: Jesper Dangaard Brouer <brouer at redhat.com>
Cc: Pavel Popa <pashinho1990 at gmail.com>
Cc: David Ahern <dsahern at gmail.com>
Signed-off-by: Jason Wang <jasowang at redhat.com>
---
 drivers/net/virtio_net.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3e2c041d76ac..9b5ace538824 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -70,7 +70,8 @@ static const unsigned long guest_offloads[] = {
 	VIRTIO_NET_F_GUEST_TSO4,
 	VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN,
-	VIRTIO_NET_F_GUEST_UFO
+	VIRTIO_NET_F_GUEST_UFO,
+	VIRTIO_NET_F_GUEST_CSUM
 };
 
 struct virtnet_stat_desc {
@@ -2334,9 +2335,6 @@ static int virtnet_clear_guest_offloads(struct
virtnet_info *vi)
 	if (!vi->guest_offloads)
 		return 0;
 
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
-		offloads = 1ULL << VIRTIO_NET_F_GUEST_CSUM;
-
 	return virtnet_set_guest_offloads(vi, offloads);
 }
 
@@ -2346,8 +2344,6 @@ static int virtnet_restore_guest_offloads(struct
virtnet_info *vi)
 
 	if (!vi->guest_offloads)
 		return 0;
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
-		offloads |= 1ULL << VIRTIO_NET_F_GUEST_CSUM;
 
 	return virtnet_set_guest_offloads(vi, offloads);
 }
-- 
2.17.1

We don't support partial csumed packet since its metadata will be lost
or incorrect during XDP processing. So fail the XDP set if guest_csum
feature is negotiated.

Fixes: f600b6905015 ("virtio_net: Add XDP support")
Reported-by: Jesper Dangaard Brouer <brouer at redhat.com>
Cc: Jesper Dangaard Brouer <brouer at redhat.com>
Cc: Pavel Popa <pashinho1990 at gmail.com>
Cc: David Ahern <dsahern at gmail.com>
Signed-off-by: Jason Wang <jasowang at redhat.com>
---
 drivers/net/virtio_net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9b5ace538824..cecfd77c9f3c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2361,8 +2361,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct
bpf_prog *prog,
 	    && (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
 	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
-		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO))) {
-		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing
LRO, disable LRO first");
+		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO) ||
+		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))) {
+		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing
LRO/CSUM, disable LRO/CSUM first");
 		return -EOPNOTSUPP;
 	}
 
-- 
2.17.1

From: Jason Wang <jasowang at redhat.com>
Date: Thu, 22 Nov 2018 14:36:30 +0800
> We don't disable VIRTIO_NET_F_GUEST_CSUM if XDP was set. This means we
> can receive partial csumed packets with metadata kept in the
> vnet_hdr. This may have several side effects:
> 
> - It could be overridden by header adjustment, thus is might be not
>   correct after XDP processing.
> - There's no way to pass such metadata information through
>   XDP_REDIRECT to another driver.
> - XDP does not support checksum offload right now.
> 
> So simply disable guest csum if possible in this the case of XDP.
> 
> Fixes: 3f93522ffab2d ("virtio-net: switch off offloads on demand if
possible on XDP set")
> Reported-by: Jesper Dangaard Brouer <brouer at redhat.com>
> Cc: Jesper Dangaard Brouer <brouer at redhat.com>
> Cc: Pavel Popa <pashinho1990 at gmail.com>
> Cc: David Ahern <dsahern at gmail.com>
> Signed-off-by: Jason Wang <jasowang at redhat.com>
Applied and queued up for -stable.

We really should have a way to use the checksum provided if the XDP
program returns XDP_PASS and does not modify the packet contents
or size.

From: Jason Wang <jasowang at redhat.com>
Date: Thu, 22 Nov 2018 14:36:31 +0800
> We don't support partial csumed packet since its metadata will be lost
> or incorrect during XDP processing. So fail the XDP set if guest_csum
> feature is negotiated.
> 
> Fixes: f600b6905015 ("virtio_net: Add XDP support")
> Reported-by: Jesper Dangaard Brouer <brouer at redhat.com>
> Cc: Jesper Dangaard Brouer <brouer at redhat.com>
> Cc: Pavel Popa <pashinho1990 at gmail.com>
> Cc: David Ahern <dsahern at gmail.com>
> Signed-off-by: Jason Wang <jasowang at redhat.com>
Applied and queued up for -stable.

Same comments as for patch #1.

On 2018/11/24 ??4:01, David Miller wrote:
> From: Jason Wang <jasowang at redhat.com>
> Date: Thu, 22 Nov 2018 14:36:30 +0800
>
>> We don't disable VIRTIO_NET_F_GUEST_CSUM if XDP was set. This means
we
>> can receive partial csumed packets with metadata kept in the
>> vnet_hdr. This may have several side effects:
>>
>> - It could be overridden by header adjustment, thus is might be not
>>    correct after XDP processing.
>> - There's no way to pass such metadata information through
>>    XDP_REDIRECT to another driver.
>> - XDP does not support checksum offload right now.
>>
>> So simply disable guest csum if possible in this the case of XDP.
>>
>> Fixes: 3f93522ffab2d ("virtio-net: switch off offloads on demand
if possible on XDP set")
>> Reported-by: Jesper Dangaard Brouer <brouer at redhat.com>
>> Cc: Jesper Dangaard Brouer <brouer at redhat.com>
>> Cc: Pavel Popa <pashinho1990 at gmail.com>
>> Cc: David Ahern <dsahern at gmail.com>
>> Signed-off-by: Jason Wang <jasowang at redhat.com>
> Applied and queued up for -stable.
>
> We really should have a way to use the checksum provided if the XDP
> program returns XDP_PASS and does not modify the packet contents
> or size.

Yes, I think this may require the assistance of BPF verifier to set a 
flag or other. Then we can assume the metadata is safe to use.

Thanks