* [PATCH vhost] virtio_net: update the hdr_len calculation for xmit
@ 2024-03-20 10:20 Xuan Zhuo
2024-03-21 4:17 ` Jason Wang
0 siblings, 1 reply; 9+ messages in thread
From: Xuan Zhuo @ 2024-03-20 10:20 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Spike Du
The virtio spec says:
If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have
been negotiated:
If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated,
and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver
MUST set hdr_len to a value equal to the length of the headers,
including the transport header.
If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated,
or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set
hdr_len to a value not less than the length of the headers,
including the transport header.
So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the
hdr->hdr_len should be eth header + ip header + tcp/udp header.
But now:
hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb));
The skb_headlen() returns the linear space of the skb, not the header
size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has
not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE.
We do not check the feature of the device. This function is a common
function used by many places. So we do more stricter work whatever
the features is negotiated.
For the case skb_is_gso(skb) is false, if none of the
VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated,
the spec not define the action of setting hdr_len. Here I set it to
skb_headlen(). If one of the above features have been negotiated, we
should set a value not less than the length of "eth header + ip header +
tcp/udp header". So the skb_headlen() also is a valid value.
For the case skb_is_gso(skb) is true, it implies that one of
VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been
negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set
it to the length of "eth header + ip header + tcp/udp header".
If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a
valid value.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reported-by: Spike Du <spiked@nvidia.com>
---
include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++----------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 4dfa9b69ca8d..51d93f9762d7 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
if (skb_is_gso(skb)) {
struct skb_shared_info *sinfo = skb_shinfo(skb);
+ u32 hdrlen;
- /* This is a hint as to how much should be linear. */
- hdr->hdr_len = __cpu_to_virtio16(little_endian,
- skb_headlen(skb));
- hdr->gso_size = __cpu_to_virtio16(little_endian,
- sinfo->gso_size);
- if (sinfo->gso_type & SKB_GSO_TCPV4)
+ if (sinfo->gso_type & SKB_GSO_TCPV4) {
hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
- else if (sinfo->gso_type & SKB_GSO_TCPV6)
+ hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb);
+
+ } else if (sinfo->gso_type & SKB_GSO_TCPV6) {
hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
- else if (sinfo->gso_type & SKB_GSO_UDP_L4)
+ hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb);
+
+ } else if (sinfo->gso_type & SKB_GSO_UDP_L4) {
hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4;
- else
+ hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb);
+
+ } else {
return -EINVAL;
+ }
+
+ /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST
+ * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is
+ * negotiated, we MUST set it to the length of "eth header + ip
+ * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN
+ * is not negotiated, that still be a valid value.
+ */
+ hdr->hdr_len = __cpu_to_virtio16(little_endian, hdrlen);
+ hdr->gso_size = __cpu_to_virtio16(little_endian,
+ sinfo->gso_size);
+
if (sinfo->gso_type & SKB_GSO_TCP_ECN)
hdr->gso_type |= VIRTIO_NET_HDR_GSO_ECN;
- } else
+ } else {
+ /* If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO
+ * options have been negotiated, we should set hdr_len to a
+ * value not less than the length of "eth header + ip header +
+ * tcp/udp header".
+ */
+ hdr->hdr_len = __cpu_to_virtio16(little_endian, skb_headlen(skb));
hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
+ }
if (skb->ip_summed == CHECKSUM_PARTIAL) {
hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
--
2.32.0.3.g01195cf9f
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit 2024-03-20 10:20 [PATCH vhost] virtio_net: update the hdr_len calculation for xmit Xuan Zhuo @ 2024-03-21 4:17 ` Jason Wang 2024-03-22 2:27 ` Xuan Zhuo 0 siblings, 1 reply; 9+ messages in thread From: Jason Wang @ 2024-03-21 4:17 UTC (permalink / raw) To: Xuan Zhuo; +Cc: virtualization, Michael S. Tsirkin, Spike Du On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > The virtio spec says: > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have > been negotiated: > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver > MUST set hdr_len to a value equal to the length of the headers, > including the transport header. > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set > hdr_len to a value not less than the length of the headers, > including the transport header. > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the > hdr->hdr_len should be eth header + ip header + tcp/udp header. > > But now: > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb)); > > The skb_headlen() returns the linear space of the skb, not the header > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE. > > We do not check the feature of the device. This function is a common > function used by many places. So we do more stricter work whatever > the features is negotiated. > > For the case skb_is_gso(skb) is false, if none of the > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated, > the spec not define the action of setting hdr_len. Here I set it to > skb_headlen(). If one of the above features have been negotiated, we > should set a value not less than the length of "eth header + ip header + > tcp/udp header". So the skb_headlen() also is a valid value. > > For the case skb_is_gso(skb) is true, it implies that one of > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set > it to the length of "eth header + ip header + tcp/udp header". > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a > valid value. > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > Reported-by: Spike Du <spiked@nvidia.com> > --- > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++---------- > 1 file changed, 31 insertions(+), 10 deletions(-) > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index 4dfa9b69ca8d..51d93f9762d7 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > if (skb_is_gso(skb)) { > struct skb_shared_info *sinfo = skb_shinfo(skb); > + u32 hdrlen; > > - /* This is a hint as to how much should be linear. */ > - hdr->hdr_len = __cpu_to_virtio16(little_endian, > - skb_headlen(skb)); > - hdr->gso_size = __cpu_to_virtio16(little_endian, > - sinfo->gso_size); > - if (sinfo->gso_type & SKB_GSO_TCPV4) > + if (sinfo->gso_type & SKB_GSO_TCPV4) { > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > - else if (sinfo->gso_type & SKB_GSO_TCPV6) > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); So could evil guests give us a 0 hdrlen through this. If yes, it seems much more dangerous than headlen or we need harden the value as 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did. > + > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) { > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > - else if (sinfo->gso_type & SKB_GSO_UDP_L4) > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > + > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) { > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > - else > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb); > + > + } else { > return -EINVAL; > + } > + > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is > + * negotiated, we MUST set it to the length of "eth header + ip > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN > + * is not negotiated, that still be a valid value. > + */ I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much more safe as we don't want to break legacy devices. Thanks ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit 2024-03-21 4:17 ` Jason Wang @ 2024-03-22 2:27 ` Xuan Zhuo 2024-03-22 5:04 ` Jason Wang 0 siblings, 1 reply; 9+ messages in thread From: Xuan Zhuo @ 2024-03-22 2:27 UTC (permalink / raw) To: Jason Wang; +Cc: virtualization, Michael S. Tsirkin, Spike Du On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > The virtio spec says: > > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have > > been negotiated: > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, > > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver > > MUST set hdr_len to a value equal to the length of the headers, > > including the transport header. > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, > > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set > > hdr_len to a value not less than the length of the headers, > > including the transport header. > > > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the > > hdr->hdr_len should be eth header + ip header + tcp/udp header. > > > > But now: > > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb)); > > > > The skb_headlen() returns the linear space of the skb, not the header > > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has > > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE. > > > > We do not check the feature of the device. This function is a common > > function used by many places. So we do more stricter work whatever > > the features is negotiated. > > > > For the case skb_is_gso(skb) is false, if none of the > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated, > > the spec not define the action of setting hdr_len. Here I set it to > > skb_headlen(). If one of the above features have been negotiated, we > > should set a value not less than the length of "eth header + ip header + > > tcp/udp header". So the skb_headlen() also is a valid value. > > > > For the case skb_is_gso(skb) is true, it implies that one of > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been > > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set > > it to the length of "eth header + ip header + tcp/udp header". > > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a > > valid value. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > Reported-by: Spike Du <spiked@nvidia.com> > > --- > > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++---------- > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > index 4dfa9b69ca8d..51d93f9762d7 100644 > > --- a/include/linux/virtio_net.h > > +++ b/include/linux/virtio_net.h > > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > if (skb_is_gso(skb)) { > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > + u32 hdrlen; > > > > - /* This is a hint as to how much should be linear. */ > > - hdr->hdr_len = __cpu_to_virtio16(little_endian, > > - skb_headlen(skb)); > > - hdr->gso_size = __cpu_to_virtio16(little_endian, > > - sinfo->gso_size); > > - if (sinfo->gso_type & SKB_GSO_TCPV4) > > + if (sinfo->gso_type & SKB_GSO_TCPV4) { > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > - else if (sinfo->gso_type & SKB_GSO_TCPV6) > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > So could evil guests give us a 0 hdrlen through this. If yes, it seems > much more dangerous than headlen or we need harden the value as > 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did. Spec says: If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len as the transport header size. Note: Caution should be taken by the implementation so as to prevent a malicious driver from attacking the device by setting an incorrect hdr_len. If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len only as a hint about the transport header size. The device MUST NOT rely on hdr_len to be correct. Note: This is due to various bugs in implementations. The device nerver trust the hdr_len completely. So I think that is safe. > > > + > > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) { > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > - else if (sinfo->gso_type & SKB_GSO_UDP_L4) > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > + > > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) { > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > > - else > > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb); > > + > > + } else { > > return -EINVAL; > > + } > > + > > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST > > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is > > + * negotiated, we MUST set it to the length of "eth header + ip > > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN > > + * is not negotiated, that still be a valid value. > > + */ > > I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much > more safe as we don't want to break legacy devices. I do not get it. Thanks. > > Thanks > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit 2024-03-22 2:27 ` Xuan Zhuo @ 2024-03-22 5:04 ` Jason Wang 2024-03-22 5:45 ` Xuan Zhuo 0 siblings, 1 reply; 9+ messages in thread From: Jason Wang @ 2024-03-22 5:04 UTC (permalink / raw) To: Xuan Zhuo; +Cc: virtualization, Michael S. Tsirkin, Spike Du On Fri, Mar 22, 2024 at 10:29 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > The virtio spec says: > > > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have > > > been negotiated: > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, > > > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver > > > MUST set hdr_len to a value equal to the length of the headers, > > > including the transport header. > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, > > > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set > > > hdr_len to a value not less than the length of the headers, > > > including the transport header. > > > > > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the > > > hdr->hdr_len should be eth header + ip header + tcp/udp header. > > > > > > But now: > > > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb)); > > > > > > The skb_headlen() returns the linear space of the skb, not the header > > > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has > > > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE. > > > > > > We do not check the feature of the device. This function is a common > > > function used by many places. So we do more stricter work whatever > > > the features is negotiated. > > > > > > For the case skb_is_gso(skb) is false, if none of the > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated, > > > the spec not define the action of setting hdr_len. Here I set it to > > > skb_headlen(). If one of the above features have been negotiated, we > > > should set a value not less than the length of "eth header + ip header + > > > tcp/udp header". So the skb_headlen() also is a valid value. > > > > > > For the case skb_is_gso(skb) is true, it implies that one of > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been > > > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set > > > it to the length of "eth header + ip header + tcp/udp header". > > > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a > > > valid value. > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > Reported-by: Spike Du <spiked@nvidia.com> > > > --- > > > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++---------- > > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > index 4dfa9b69ca8d..51d93f9762d7 100644 > > > --- a/include/linux/virtio_net.h > > > +++ b/include/linux/virtio_net.h > > > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > > > if (skb_is_gso(skb)) { > > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > > + u32 hdrlen; > > > > > > - /* This is a hint as to how much should be linear. */ > > > - hdr->hdr_len = __cpu_to_virtio16(little_endian, > > > - skb_headlen(skb)); > > > - hdr->gso_size = __cpu_to_virtio16(little_endian, > > > - sinfo->gso_size); > > > - if (sinfo->gso_type & SKB_GSO_TCPV4) > > > + if (sinfo->gso_type & SKB_GSO_TCPV4) { > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > > - else if (sinfo->gso_type & SKB_GSO_TCPV6) > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > So could evil guests give us a 0 hdrlen through this. If yes, it seems > > much more dangerous than headlen or we need harden the value as > > 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did. > > Spec says: > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, and > gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the device MAY use > hdr_len as the transport header size. Note: Caution should be taken by > the implementation so as to prevent a malicious driver from attacking > the device by setting an incorrect hdr_len. > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, or > gso_type is VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len only as > a hint about the transport header size. The device MUST NOT rely on > hdr_len to be correct. Note: This is due to various bugs in > implementations. > > The device nerver trust the hdr_len completely. > So I think that is safe. It doesn't, there's no way to audit all the existing implementations of virtio. For example, have you seen the commit I've pointed to you? > > > > > > > + > > > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) { > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > > - else if (sinfo->gso_type & SKB_GSO_UDP_L4) > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > + > > > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) { > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > > > - else > > > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb); > > > + > > > + } else { > > > return -EINVAL; > > > + } > > > + > > > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST > > > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is > > > + * negotiated, we MUST set it to the length of "eth header + ip > > > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN > > > + * is not negotiated, that still be a valid value. > > > + */ > > > > I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much > > more safe as we don't want to break legacy devices. > > I do not get it. if (GUEST_HDRLEN) tell the offset of the payload else tell the headlen (stick to the old behaviour) Thanks > > Thanks. > > > > > > Thanks > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit 2024-03-22 5:04 ` Jason Wang @ 2024-03-22 5:45 ` Xuan Zhuo 2024-03-22 5:57 ` Jason Wang 0 siblings, 1 reply; 9+ messages in thread From: Xuan Zhuo @ 2024-03-22 5:45 UTC (permalink / raw) To: Jason Wang; +Cc: virtualization, Michael S. Tsirkin, Spike Du On Fri, 22 Mar 2024 13:04:48 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Fri, Mar 22, 2024 at 10:29 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > The virtio spec says: > > > > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have > > > > been negotiated: > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, > > > > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver > > > > MUST set hdr_len to a value equal to the length of the headers, > > > > including the transport header. > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, > > > > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set > > > > hdr_len to a value not less than the length of the headers, > > > > including the transport header. > > > > > > > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the > > > > hdr->hdr_len should be eth header + ip header + tcp/udp header. > > > > > > > > But now: > > > > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb)); > > > > > > > > The skb_headlen() returns the linear space of the skb, not the header > > > > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has > > > > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE. > > > > > > > > We do not check the feature of the device. This function is a common > > > > function used by many places. So we do more stricter work whatever > > > > the features is negotiated. > > > > > > > > For the case skb_is_gso(skb) is false, if none of the > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated, > > > > the spec not define the action of setting hdr_len. Here I set it to > > > > skb_headlen(). If one of the above features have been negotiated, we > > > > should set a value not less than the length of "eth header + ip header + > > > > tcp/udp header". So the skb_headlen() also is a valid value. > > > > > > > > For the case skb_is_gso(skb) is true, it implies that one of > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been > > > > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set > > > > it to the length of "eth header + ip header + tcp/udp header". > > > > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a > > > > valid value. > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > Reported-by: Spike Du <spiked@nvidia.com> > > > > --- > > > > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++---------- > > > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > > index 4dfa9b69ca8d..51d93f9762d7 100644 > > > > --- a/include/linux/virtio_net.h > > > > +++ b/include/linux/virtio_net.h > > > > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > > > > > if (skb_is_gso(skb)) { > > > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > > > + u32 hdrlen; > > > > > > > > - /* This is a hint as to how much should be linear. */ > > > > - hdr->hdr_len = __cpu_to_virtio16(little_endian, > > > > - skb_headlen(skb)); > > > > - hdr->gso_size = __cpu_to_virtio16(little_endian, > > > > - sinfo->gso_size); > > > > - if (sinfo->gso_type & SKB_GSO_TCPV4) > > > > + if (sinfo->gso_type & SKB_GSO_TCPV4) { > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > > > - else if (sinfo->gso_type & SKB_GSO_TCPV6) > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > > So could evil guests give us a 0 hdrlen through this. If yes, it seems > > > much more dangerous than headlen or we need harden the value as > > > 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did. > > > > Spec says: > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, and > > gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the device MAY use > > hdr_len as the transport header size. Note: Caution should be taken by > > the implementation so as to prevent a malicious driver from attacking > > the device by setting an incorrect hdr_len. > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, or > > gso_type is VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len only as > > a hint about the transport header size. The device MUST NOT rely on > > hdr_len to be correct. Note: This is due to various bugs in > > implementations. > > > > The device nerver trust the hdr_len completely. > > So I think that is safe. > > It doesn't, there's no way to audit all the existing implementations of virtio. > > For example, have you seen the commit I've pointed to you? I haven seen it. But here, this is driver, the device should handen the check. For the driver, we should do as the spec. Do you mean that some evil driver pass 0 to device? Now we fix that. > > > > > > > > > > > > + > > > > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) { > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > > > - else if (sinfo->gso_type & SKB_GSO_UDP_L4) > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > + > > > > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) { > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > > > > - else > > > > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb); > > > > + > > > > + } else { > > > > return -EINVAL; > > > > + } > > > > + > > > > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST > > > > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is > > > > + * negotiated, we MUST set it to the length of "eth header + ip > > > > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN > > > > + * is not negotiated, that still be a valid value. > > > > + */ > > > > > > I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much > > > more safe as we don't want to break legacy devices. > > > > I do not get it. > > if (GUEST_HDRLEN) > tell the offset of the payload > else > tell the headlen (stick to the old behaviour) I am ok. Then we need to pass the features of the device to this function. Thanks > > Thanks > > > > > Thanks. > > > > > > > > > > Thanks > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit 2024-03-22 5:45 ` Xuan Zhuo @ 2024-03-22 5:57 ` Jason Wang 2024-03-22 6:08 ` Xuan Zhuo 0 siblings, 1 reply; 9+ messages in thread From: Jason Wang @ 2024-03-22 5:57 UTC (permalink / raw) To: Xuan Zhuo; +Cc: virtualization, Michael S. Tsirkin, Spike Du On Fri, Mar 22, 2024 at 1:51 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Fri, 22 Mar 2024 13:04:48 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Mar 22, 2024 at 10:29 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > > > The virtio spec says: > > > > > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have > > > > > been negotiated: > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, > > > > > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver > > > > > MUST set hdr_len to a value equal to the length of the headers, > > > > > including the transport header. > > > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, > > > > > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set > > > > > hdr_len to a value not less than the length of the headers, > > > > > including the transport header. > > > > > > > > > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the > > > > > hdr->hdr_len should be eth header + ip header + tcp/udp header. > > > > > > > > > > But now: > > > > > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb)); > > > > > > > > > > The skb_headlen() returns the linear space of the skb, not the header > > > > > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has > > > > > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE. > > > > > > > > > > We do not check the feature of the device. This function is a common > > > > > function used by many places. So we do more stricter work whatever > > > > > the features is negotiated. > > > > > > > > > > For the case skb_is_gso(skb) is false, if none of the > > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated, > > > > > the spec not define the action of setting hdr_len. Here I set it to > > > > > skb_headlen(). If one of the above features have been negotiated, we > > > > > should set a value not less than the length of "eth header + ip header + > > > > > tcp/udp header". So the skb_headlen() also is a valid value. > > > > > > > > > > For the case skb_is_gso(skb) is true, it implies that one of > > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been > > > > > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set > > > > > it to the length of "eth header + ip header + tcp/udp header". > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a > > > > > valid value. > > > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > Reported-by: Spike Du <spiked@nvidia.com> > > > > > --- > > > > > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++---------- > > > > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > > > index 4dfa9b69ca8d..51d93f9762d7 100644 > > > > > --- a/include/linux/virtio_net.h > > > > > +++ b/include/linux/virtio_net.h > > > > > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > > > > > > > if (skb_is_gso(skb)) { > > > > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > > > > + u32 hdrlen; > > > > > > > > > > - /* This is a hint as to how much should be linear. */ > > > > > - hdr->hdr_len = __cpu_to_virtio16(little_endian, > > > > > - skb_headlen(skb)); > > > > > - hdr->gso_size = __cpu_to_virtio16(little_endian, > > > > > - sinfo->gso_size); > > > > > - if (sinfo->gso_type & SKB_GSO_TCPV4) > > > > > + if (sinfo->gso_type & SKB_GSO_TCPV4) { > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > > > > - else if (sinfo->gso_type & SKB_GSO_TCPV6) > > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > > > > So could evil guests give us a 0 hdrlen through this. If yes, it seems > > > > much more dangerous than headlen or we need harden the value as > > > > 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did. > > > > > > Spec says: > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, and > > > gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the device MAY use > > > hdr_len as the transport header size. Note: Caution should be taken by > > > the implementation so as to prevent a malicious driver from attacking > > > the device by setting an incorrect hdr_len. > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, or > > > gso_type is VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len only as > > > a hint about the transport header size. The device MUST NOT rely on > > > hdr_len to be correct. Note: This is due to various bugs in > > > implementations. > > > > > > The device nerver trust the hdr_len completely. > > > So I think that is safe. > > > > It doesn't, there's no way to audit all the existing implementations of virtio. > > > > For example, have you seen the commit I've pointed to you? > > I haven seen it. > > But here, this is driver, the device should handen the check. Unfortunately not. We can't audit all device implementations. > For the driver, we should do as the spec. > Do you mean that some evil driver pass 0 to device? > Now we fix that. hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb) Did you mean hdrlen can't be zero here? I guess it is done by the DODGY checking in the GSO path? Then, let's explain it here. > > > > > > > > > > > > > > > > > > > + > > > > > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) { > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > > > > - else if (sinfo->gso_type & SKB_GSO_UDP_L4) > > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > + > > > > > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) { > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > > > > > - else > > > > > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb); > > > > > + > > > > > + } else { > > > > > return -EINVAL; > > > > > + } > > > > > + > > > > > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST > > > > > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is > > > > > + * negotiated, we MUST set it to the length of "eth header + ip > > > > > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN > > > > > + * is not negotiated, that still be a valid value. > > > > > + */ > > > > > > > > I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much > > > > more safe as we don't want to break legacy devices. > > > > > > I do not get it. > > > > if (GUEST_HDRLEN) > > tell the offset of the payload > > else > > tell the headlen (stick to the old behaviour) > > > I am ok. Then we need to pass the features of the device to this function. Yes, a boolean probably. Thanks > > Thanks > > > > > > > Thanks > > > > > > > > Thanks. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit 2024-03-22 5:57 ` Jason Wang @ 2024-03-22 6:08 ` Xuan Zhuo 2024-04-22 20:31 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Xuan Zhuo @ 2024-03-22 6:08 UTC (permalink / raw) To: Jason Wang; +Cc: virtualization, Michael S. Tsirkin, Spike Du On Fri, 22 Mar 2024 13:57:36 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Fri, Mar 22, 2024 at 1:51 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > On Fri, 22 Mar 2024 13:04:48 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > On Fri, Mar 22, 2024 at 10:29 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > > > > > The virtio spec says: > > > > > > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have > > > > > > been negotiated: > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, > > > > > > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver > > > > > > MUST set hdr_len to a value equal to the length of the headers, > > > > > > including the transport header. > > > > > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, > > > > > > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set > > > > > > hdr_len to a value not less than the length of the headers, > > > > > > including the transport header. > > > > > > > > > > > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the > > > > > > hdr->hdr_len should be eth header + ip header + tcp/udp header. > > > > > > > > > > > > But now: > > > > > > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb)); > > > > > > > > > > > > The skb_headlen() returns the linear space of the skb, not the header > > > > > > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has > > > > > > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE. > > > > > > > > > > > > We do not check the feature of the device. This function is a common > > > > > > function used by many places. So we do more stricter work whatever > > > > > > the features is negotiated. > > > > > > > > > > > > For the case skb_is_gso(skb) is false, if none of the > > > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated, > > > > > > the spec not define the action of setting hdr_len. Here I set it to > > > > > > skb_headlen(). If one of the above features have been negotiated, we > > > > > > should set a value not less than the length of "eth header + ip header + > > > > > > tcp/udp header". So the skb_headlen() also is a valid value. > > > > > > > > > > > > For the case skb_is_gso(skb) is true, it implies that one of > > > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been > > > > > > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set > > > > > > it to the length of "eth header + ip header + tcp/udp header". > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a > > > > > > valid value. > > > > > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > > Reported-by: Spike Du <spiked@nvidia.com> > > > > > > --- > > > > > > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++---------- > > > > > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > > > > index 4dfa9b69ca8d..51d93f9762d7 100644 > > > > > > --- a/include/linux/virtio_net.h > > > > > > +++ b/include/linux/virtio_net.h > > > > > > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > > > > > > > > > if (skb_is_gso(skb)) { > > > > > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > > > > > + u32 hdrlen; > > > > > > > > > > > > - /* This is a hint as to how much should be linear. */ > > > > > > - hdr->hdr_len = __cpu_to_virtio16(little_endian, > > > > > > - skb_headlen(skb)); > > > > > > - hdr->gso_size = __cpu_to_virtio16(little_endian, > > > > > > - sinfo->gso_size); > > > > > > - if (sinfo->gso_type & SKB_GSO_TCPV4) > > > > > > + if (sinfo->gso_type & SKB_GSO_TCPV4) { > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > > > > > - else if (sinfo->gso_type & SKB_GSO_TCPV6) > > > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > > > > > > So could evil guests give us a 0 hdrlen through this. If yes, it seems > > > > > much more dangerous than headlen or we need harden the value as > > > > > 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did. > > > > > > > > Spec says: > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, and > > > > gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the device MAY use > > > > hdr_len as the transport header size. Note: Caution should be taken by > > > > the implementation so as to prevent a malicious driver from attacking > > > > the device by setting an incorrect hdr_len. > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, or > > > > gso_type is VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len only as > > > > a hint about the transport header size. The device MUST NOT rely on > > > > hdr_len to be correct. Note: This is due to various bugs in > > > > implementations. > > > > > > > > The device nerver trust the hdr_len completely. > > > > So I think that is safe. > > > > > > It doesn't, there's no way to audit all the existing implementations of virtio. > > > > > > For example, have you seen the commit I've pointed to you? > > > > I haven seen it. > > > > But here, this is driver, the device should handen the check. > > Unfortunately not. We can't audit all device implementations. > > > For the driver, we should do as the spec. > > Do you mean that some evil driver pass 0 to device? > > Now we fix that. > > hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb) > > Did you mean hdrlen can't be zero here? I guess it is done by the > DODGY checking in the GSO path? Do you mean tcp_hdrlen(skb) + skb_transport_offset(skb) maybe zero? I think so indeed. I will to check the logic of DODGY. Thanks. > > Then, let's explain it here. > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) { > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > > > > > - else if (sinfo->gso_type & SKB_GSO_UDP_L4) > > > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > > + > > > > > > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) { > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > > > > > > - else > > > > > > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb); > > > > > > + > > > > > > + } else { > > > > > > return -EINVAL; > > > > > > + } > > > > > > + > > > > > > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST > > > > > > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is > > > > > > + * negotiated, we MUST set it to the length of "eth header + ip > > > > > > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN > > > > > > + * is not negotiated, that still be a valid value. > > > > > > + */ > > > > > > > > > > I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much > > > > > more safe as we don't want to break legacy devices. > > > > > > > > I do not get it. > > > > > > if (GUEST_HDRLEN) > > > tell the offset of the payload > > > else > > > tell the headlen (stick to the old behaviour) > > > > > > I am ok. Then we need to pass the features of the device to this function. > > Yes, a boolean probably. > > Thanks > > > > > Thanks > > > > > > > > > > > > Thanks > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit 2024-03-22 6:08 ` Xuan Zhuo @ 2024-04-22 20:31 ` Michael S. Tsirkin 2024-04-23 5:53 ` Xuan Zhuo 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2024-04-22 20:31 UTC (permalink / raw) To: Xuan Zhuo; +Cc: Jason Wang, virtualization, Spike Du On Fri, Mar 22, 2024 at 02:08:25PM +0800, Xuan Zhuo wrote: > On Fri, 22 Mar 2024 13:57:36 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Fri, Mar 22, 2024 at 1:51 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > On Fri, 22 Mar 2024 13:04:48 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > On Fri, Mar 22, 2024 at 10:29 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > > > On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > > > > > > > The virtio spec says: > > > > > > > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have > > > > > > > been negotiated: > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, > > > > > > > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver > > > > > > > MUST set hdr_len to a value equal to the length of the headers, > > > > > > > including the transport header. > > > > > > > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, > > > > > > > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set > > > > > > > hdr_len to a value not less than the length of the headers, > > > > > > > including the transport header. > > > > > > > > > > > > > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the > > > > > > > hdr->hdr_len should be eth header + ip header + tcp/udp header. > > > > > > > > > > > > > > But now: > > > > > > > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb)); > > > > > > > > > > > > > > The skb_headlen() returns the linear space of the skb, not the header > > > > > > > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has > > > > > > > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE. > > > > > > > > > > > > > > We do not check the feature of the device. This function is a common > > > > > > > function used by many places. So we do more stricter work whatever > > > > > > > the features is negotiated. > > > > > > > > > > > > > > For the case skb_is_gso(skb) is false, if none of the > > > > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated, > > > > > > > the spec not define the action of setting hdr_len. Here I set it to > > > > > > > skb_headlen(). If one of the above features have been negotiated, we > > > > > > > should set a value not less than the length of "eth header + ip header + > > > > > > > tcp/udp header". So the skb_headlen() also is a valid value. > > > > > > > > > > > > > > For the case skb_is_gso(skb) is true, it implies that one of > > > > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been > > > > > > > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set > > > > > > > it to the length of "eth header + ip header + tcp/udp header". > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a > > > > > > > valid value. > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > > > Reported-by: Spike Du <spiked@nvidia.com> > > > > > > > --- > > > > > > > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++---------- > > > > > > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > > > > > index 4dfa9b69ca8d..51d93f9762d7 100644 > > > > > > > --- a/include/linux/virtio_net.h > > > > > > > +++ b/include/linux/virtio_net.h > > > > > > > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > > > > > > > > > > > if (skb_is_gso(skb)) { > > > > > > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > > > > > > + u32 hdrlen; > > > > > > > > > > > > > > - /* This is a hint as to how much should be linear. */ > > > > > > > - hdr->hdr_len = __cpu_to_virtio16(little_endian, > > > > > > > - skb_headlen(skb)); > > > > > > > - hdr->gso_size = __cpu_to_virtio16(little_endian, > > > > > > > - sinfo->gso_size); > > > > > > > - if (sinfo->gso_type & SKB_GSO_TCPV4) > > > > > > > + if (sinfo->gso_type & SKB_GSO_TCPV4) { > > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > > > > > > - else if (sinfo->gso_type & SKB_GSO_TCPV6) > > > > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > > > > > > > > So could evil guests give us a 0 hdrlen through this. If yes, it seems > > > > > > much more dangerous than headlen or we need harden the value as > > > > > > 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did. > > > > > > > > > > Spec says: > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, and > > > > > gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the device MAY use > > > > > hdr_len as the transport header size. Note: Caution should be taken by > > > > > the implementation so as to prevent a malicious driver from attacking > > > > > the device by setting an incorrect hdr_len. > > > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, or > > > > > gso_type is VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len only as > > > > > a hint about the transport header size. The device MUST NOT rely on > > > > > hdr_len to be correct. Note: This is due to various bugs in > > > > > implementations. > > > > > > > > > > The device nerver trust the hdr_len completely. > > > > > So I think that is safe. > > > > > > > > It doesn't, there's no way to audit all the existing implementations of virtio. > > > > > > > > For example, have you seen the commit I've pointed to you? > > > > > > I haven seen it. > > > > > > But here, this is driver, the device should handen the check. > > > > Unfortunately not. We can't audit all device implementations. > > > > > For the driver, we should do as the spec. > > > Do you mean that some evil driver pass 0 to device? > > > Now we fix that. > > > > hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb) > > > > Did you mean hdrlen can't be zero here? I guess it is done by the > > DODGY checking in the GSO path? > > Do you mean tcp_hdrlen(skb) + skb_transport_offset(skb) maybe zero? > I think so indeed. I will to check the logic of DODGY. > > Thanks. Was any conclusion reached? > > > > Then, let's explain it here. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) { > > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > > > > > > - else if (sinfo->gso_type & SKB_GSO_UDP_L4) > > > > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > > > + > > > > > > > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) { > > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > > > > > > > - else > > > > > > > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb); > > > > > > > + > > > > > > > + } else { > > > > > > > return -EINVAL; > > > > > > > + } > > > > > > > + > > > > > > > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST > > > > > > > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is > > > > > > > + * negotiated, we MUST set it to the length of "eth header + ip > > > > > > > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN > > > > > > > + * is not negotiated, that still be a valid value. > > > > > > > + */ > > > > > > > > > > > > I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much > > > > > > more safe as we don't want to break legacy devices. > > > > > > > > > > I do not get it. > > > > > > > > if (GUEST_HDRLEN) > > > > tell the offset of the payload > > > > else > > > > tell the headlen (stick to the old behaviour) > > > > > > > > > I am ok. Then we need to pass the features of the device to this function. > > > > Yes, a boolean probably. > > > > Thanks > > > > > > > > Thanks > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH vhost] virtio_net: update the hdr_len calculation for xmit 2024-04-22 20:31 ` Michael S. Tsirkin @ 2024-04-23 5:53 ` Xuan Zhuo 0 siblings, 0 replies; 9+ messages in thread From: Xuan Zhuo @ 2024-04-23 5:53 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, Spike Du On Mon, 22 Apr 2024 16:31:03 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Mar 22, 2024 at 02:08:25PM +0800, Xuan Zhuo wrote: > > On Fri, 22 Mar 2024 13:57:36 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > On Fri, Mar 22, 2024 at 1:51 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > On Fri, 22 Mar 2024 13:04:48 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > > On Fri, Mar 22, 2024 at 10:29 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > > > > > On Thu, 21 Mar 2024 12:17:54 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > > > > On Wed, Mar 20, 2024 at 6:20 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > > > > > > > > > > > The virtio spec says: > > > > > > > > If one of the VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have > > > > > > > > been negotiated: > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, > > > > > > > > and gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the driver > > > > > > > > MUST set hdr_len to a value equal to the length of the headers, > > > > > > > > including the transport header. > > > > > > > > > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, > > > > > > > > or gso_type is VIRTIO_NET_HDR_GSO_NONE, the driver SHOULD set > > > > > > > > hdr_len to a value not less than the length of the headers, > > > > > > > > including the transport header. > > > > > > > > > > > > > > > > So if the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, the > > > > > > > > hdr->hdr_len should be eth header + ip header + tcp/udp header. > > > > > > > > > > > > > > > > But now: > > > > > > > > hdr->hdr_len = __cpu_to_virtio15(little_endian, skb_headlen(skb)); > > > > > > > > > > > > > > > > The skb_headlen() returns the linear space of the skb, not the header > > > > > > > > size that only match the case the VIRTIO_NET_F_GUEST_HDRLEN feature has > > > > > > > > not been negotiated, or gso_type is VIRTIO_NET_HDR_GSO_NONE. > > > > > > > > > > > > > > > > We do not check the feature of the device. This function is a common > > > > > > > > function used by many places. So we do more stricter work whatever > > > > > > > > the features is negotiated. > > > > > > > > > > > > > > > > For the case skb_is_gso(skb) is false, if none of the > > > > > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options have been negotiated, > > > > > > > > the spec not define the action of setting hdr_len. Here I set it to > > > > > > > > skb_headlen(). If one of the above features have been negotiated, we > > > > > > > > should set a value not less than the length of "eth header + ip header + > > > > > > > > tcp/udp header". So the skb_headlen() also is a valid value. > > > > > > > > > > > > > > > > For the case skb_is_gso(skb) is true, it implies that one of > > > > > > > > VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST have been > > > > > > > > negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is negotiated, we MUST set > > > > > > > > it to the length of "eth header + ip header + tcp/udp header". > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN is not negotiated, that still be a > > > > > > > > valid value. > > > > > > > > > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > > > > > > > > Reported-by: Spike Du <spiked@nvidia.com> > > > > > > > > --- > > > > > > > > include/linux/virtio_net.h | 41 ++++++++++++++++++++++++++++---------- > > > > > > > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > > > > > > > index 4dfa9b69ca8d..51d93f9762d7 100644 > > > > > > > > --- a/include/linux/virtio_net.h > > > > > > > > +++ b/include/linux/virtio_net.h > > > > > > > > @@ -201,24 +201,45 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb, > > > > > > > > > > > > > > > > if (skb_is_gso(skb)) { > > > > > > > > struct skb_shared_info *sinfo = skb_shinfo(skb); > > > > > > > > + u32 hdrlen; > > > > > > > > > > > > > > > > - /* This is a hint as to how much should be linear. */ > > > > > > > > - hdr->hdr_len = __cpu_to_virtio16(little_endian, > > > > > > > > - skb_headlen(skb)); > > > > > > > > - hdr->gso_size = __cpu_to_virtio16(little_endian, > > > > > > > > - sinfo->gso_size); > > > > > > > > - if (sinfo->gso_type & SKB_GSO_TCPV4) > > > > > > > > + if (sinfo->gso_type & SKB_GSO_TCPV4) { > > > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4; > > > > > > > > - else if (sinfo->gso_type & SKB_GSO_TCPV6) > > > > > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > > > > > > > > > > So could evil guests give us a 0 hdrlen through this. If yes, it seems > > > > > > > much more dangerous than headlen or we need harden the value as > > > > > > > 9181d6f8a2bb32d158de66a84164fac05e3ddd18 did. > > > > > > > > > > > > Spec says: > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has been negotiated, and > > > > > > gso_type differs from VIRTIO_NET_HDR_GSO_NONE, the device MAY use > > > > > > hdr_len as the transport header size. Note: Caution should be taken by > > > > > > the implementation so as to prevent a malicious driver from attacking > > > > > > the device by setting an incorrect hdr_len. > > > > > > > > > > > > If the VIRTIO_NET_F_GUEST_HDRLEN feature has not been negotiated, or > > > > > > gso_type is VIRTIO_NET_HDR_GSO_NONE, the device MAY use hdr_len only as > > > > > > a hint about the transport header size. The device MUST NOT rely on > > > > > > hdr_len to be correct. Note: This is due to various bugs in > > > > > > implementations. > > > > > > > > > > > > The device nerver trust the hdr_len completely. > > > > > > So I think that is safe. > > > > > > > > > > It doesn't, there's no way to audit all the existing implementations of virtio. > > > > > > > > > > For example, have you seen the commit I've pointed to you? > > > > > > > > I haven seen it. > > > > > > > > But here, this is driver, the device should handen the check. > > > > > > Unfortunately not. We can't audit all device implementations. > > > > > > > For the driver, we should do as the spec. > > > > Do you mean that some evil driver pass 0 to device? > > > > Now we fix that. > > > > > > hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb) > > > > > > Did you mean hdrlen can't be zero here? I guess it is done by the > > > DODGY checking in the GSO path? > > > > Do you mean tcp_hdrlen(skb) + skb_transport_offset(skb) maybe zero? > > I think so indeed. I will to check the logic of DODGY. > > > > Thanks. > > Was any conclusion reached? No, I am currently more focused on other work. Thanks. > > > > > > > Then, let's explain it here. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > + } else if (sinfo->gso_type & SKB_GSO_TCPV6) { > > > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6; > > > > > > > > - else if (sinfo->gso_type & SKB_GSO_UDP_L4) > > > > > > > > + hdrlen = tcp_hdrlen(skb) + skb_transport_offset(skb); > > > > > > > > + > > > > > > > > + } else if (sinfo->gso_type & SKB_GSO_UDP_L4) { > > > > > > > > hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP_L4; > > > > > > > > - else > > > > > > > > + hdrlen = sizeof(struct udphdr) + skb_transport_offset(skb); > > > > > > > > + > > > > > > > > + } else { > > > > > > > > return -EINVAL; > > > > > > > > + } > > > > > > > > + > > > > > > > > + /* One of VIRTIO_NET_F_HOST_TSO4, TSO6, USO or UFO options MUST > > > > > > > > + * have been negotiated. If the VIRTIO_NET_F_GUEST_HDRLEN is > > > > > > > > + * negotiated, we MUST set it to the length of "eth header + ip > > > > > > > > + * header + tcp/udp header". If the VIRTIO_NET_F_GUEST_HDRLEN > > > > > > > > + * is not negotiated, that still be a valid value. > > > > > > > > + */ > > > > > > > > > > > > > > I'd stick the headlen for deivce without GUEST_HDRLEN. It seems much > > > > > > > more safe as we don't want to break legacy devices. > > > > > > > > > > > > I do not get it. > > > > > > > > > > if (GUEST_HDRLEN) > > > > > tell the offset of the payload > > > > > else > > > > > tell the headlen (stick to the old behaviour) > > > > > > > > > > > > I am ok. Then we need to pass the features of the device to this function. > > > > > > Yes, a boolean probably. > > > > > > Thanks > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-23 5:54 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-20 10:20 [PATCH vhost] virtio_net: update the hdr_len calculation for xmit Xuan Zhuo 2024-03-21 4:17 ` Jason Wang 2024-03-22 2:27 ` Xuan Zhuo 2024-03-22 5:04 ` Jason Wang 2024-03-22 5:45 ` Xuan Zhuo 2024-03-22 5:57 ` Jason Wang 2024-03-22 6:08 ` Xuan Zhuo 2024-04-22 20:31 ` Michael S. Tsirkin 2024-04-23 5:53 ` Xuan Zhuo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).