diff mbox series

[net-next,v2] net: virtio_net: implement exact header length guest feature

Message ID 20230221144741.316477-1-jiri@resnulli.us (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2] net: virtio_net: implement exact header length guest feature | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 4 this patch: 4
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1 this patch: 1
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch warning WARNING: line length of 85 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko Feb. 21, 2023, 2:47 p.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
set implicates that the driver provides the exact size of the header.

Quoting the original virtio spec:
"hdr_len is a hint to the device as to how much of the header needs to
 be kept to copy into each packet"

"a hint" might not be clear for the reader what does it mean, if it is
"maybe like that" of "exactly like that". This feature just makes it
crystal clear and let the device count on the hdr_len being filled up
by the exact length of header.

Also note the spec already has following note about hdr_len:
"Due to various bugs in implementations, this field is not useful
 as a guarantee of the transport header size."

Without this feature the device needs to parse the header in core
data path handling. Accurate information helps the device to eliminate
such header parsing and directly use the hardware accelerators
for GSO operation.

virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
The driver already complies to fill the correct value. Introduce the
feature and advertise it.

Note that virtio spec also includes following note for device
implementation:
"Caution should be taken by the implementation so as to prevent
 a malicious driver from attacking the device by setting
 an incorrect hdr_len."

There is a plan to support this feature in our emulated device.
A device of SolidRun offers this feature bit. They claim this feature
will save the device a few cycles for every GSO packet.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- extended patch description
---
 drivers/net/virtio_net.c        | 6 ++++--
 include/uapi/linux/virtio_net.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Willem de Bruijn Feb. 21, 2023, 3:11 p.m. UTC | #1 
Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
> set implicates that the driver provides the exact size of the header.
> 
> Quoting the original virtio spec:
> "hdr_len is a hint to the device as to how much of the header needs to
>  be kept to copy into each packet"
> 
> "a hint" might not be clear for the reader what does it mean, if it is
> "maybe like that" of "exactly like that". This feature just makes it
> crystal clear and let the device count on the hdr_len being filled up
> by the exact length of header.
> 
> Also note the spec already has following note about hdr_len:
> "Due to various bugs in implementations, this field is not useful
>  as a guarantee of the transport header size."
> 
> Without this feature the device needs to parse the header in core
> data path handling. Accurate information helps the device to eliminate
> such header parsing and directly use the hardware accelerators
> for GSO operation.
> 
> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
> The driver already complies to fill the correct value. Introduce the
> feature and advertise it.
> 
> Note that virtio spec also includes following note for device
> implementation:
> "Caution should be taken by the implementation so as to prevent
>  a malicious driver from attacking the device by setting
>  an incorrect hdr_len."
> 
> There is a plan to support this feature in our emulated device.
> A device of SolidRun offers this feature bit. They claim this feature
> will save the device a few cycles for every GSO packet.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> v1->v2:
> - extended patch description

Is the expectation that in-kernel devices support this feature, and
if so how would it affect them? If I read the spec correctly, devices
still need to be careful against malicious drivers, so cannot assume
much beyond what they do today (i.e., a hint).

Might be good to point to the definition commit:
https://github.com/oasis-tcs/virtio-spec/commit/4f1981a1ff46b7aeb801c4c524ff76e93d9ce022
Parav Pandit Feb. 21, 2023, 3:20 p.m. UTC | #2 
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Tuesday, February 21, 2023 9:48 AM
> 
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
> set implicates that the driver provides the exact size of the header.
> 
> Quoting the original virtio spec:
> "hdr_len is a hint to the device as to how much of the header needs to  be kept
> to copy into each packet"
> 
> "a hint" might not be clear for the reader what does it mean, if it is "maybe like
> that" of "exactly like that". This feature just makes it crystal clear and let the
> device count on the hdr_len being filled up by the exact length of header.
> 
> Also note the spec already has following note about hdr_len:
> "Due to various bugs in implementations, this field is not useful  as a guarantee
> of the transport header size."
> 
> Without this feature the device needs to parse the header in core data path
> handling. Accurate information helps the device to eliminate such header
> parsing and directly use the hardware accelerators for GSO operation.
> 
> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
> The driver already complies to fill the correct value. Introduce the feature and
> advertise it.
> 
> Note that virtio spec also includes following note for device
> implementation:
> "Caution should be taken by the implementation so as to prevent  a malicious
> driver from attacking the device by setting  an incorrect hdr_len."
> 
> There is a plan to support this feature in our emulated device.
> A device of SolidRun offers this feature bit. They claim this feature will save the
> device a few cycles for every GSO packet.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

This also saves a few cycles of L2 to L4 header parsing on every GSO stream for virtio PCI PF and VF devices that we have.

> ---
> v1->v2:
> - extended patch description
> ---
>  drivers/net/virtio_net.c        | 6 ++++--
>  include/uapi/linux/virtio_net.h | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> fb5e68ed3ec2..e85b03988733 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -62,7 +62,8 @@ static const unsigned long guest_offloads[] = {
>  	VIRTIO_NET_F_GUEST_UFO,
>  	VIRTIO_NET_F_GUEST_CSUM,
>  	VIRTIO_NET_F_GUEST_USO4,
> -	VIRTIO_NET_F_GUEST_USO6
> +	VIRTIO_NET_F_GUEST_USO6,
> +	VIRTIO_NET_F_GUEST_HDRLEN
>  };
> 
>  #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL <<
> VIRTIO_NET_F_GUEST_TSO4) | \ @@ -4213,7 +4214,8 @@ static struct
> virtio_device_id id_table[] = {
>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>  	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>  	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> -	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT,
> VIRTIO_NET_F_NOTF_COAL
> +	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT,
> VIRTIO_NET_F_NOTF_COAL, \
> +	VIRTIO_NET_F_GUEST_HDRLEN
> 
>  static unsigned int features[] = {
>  	VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index b4062bed186a..12c1c9699935 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -61,6 +61,7 @@
>  #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can handle USOv6 in.
> */
>  #define VIRTIO_NET_F_HOST_USO	56	/* Host can handle USO in. */
>  #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
> +#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest provides the exact
> hdr_len value. */
>  #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
>  #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
>  #define VIRTIO_NET_F_STANDBY	  62	/* Act as standby for another
> device
> --
> 2.39.0

Reviewed-by: Parav Pandit <parav@nvidia.com>
Alvaro Karsz Feb. 21, 2023, 3:26 p.m. UTC | #3 
>
> From: Jiri Pirko <jiri@nvidia.com>
>
> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
> set implicates that the driver provides the exact size of the header.
>
> Quoting the original virtio spec:
> "hdr_len is a hint to the device as to how much of the header needs to
>  be kept to copy into each packet"
>
> "a hint" might not be clear for the reader what does it mean, if it is
> "maybe like that" of "exactly like that". This feature just makes it
> crystal clear and let the device count on the hdr_len being filled up
> by the exact length of header.
>
> Also note the spec already has following note about hdr_len:
> "Due to various bugs in implementations, this field is not useful
>  as a guarantee of the transport header size."
>
> Without this feature the device needs to parse the header in core
> data path handling. Accurate information helps the device to eliminate
> such header parsing and directly use the hardware accelerators
> for GSO operation.
>
> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
> The driver already complies to fill the correct value. Introduce the
> feature and advertise it.
>
> Note that virtio spec also includes following note for device
> implementation:
> "Caution should be taken by the implementation so as to prevent
>  a malicious driver from attacking the device by setting
>  an incorrect hdr_len."
>
> There is a plan to support this feature in our emulated device.
> A device of SolidRun offers this feature bit. They claim this feature
> will save the device a few cycles for every GSO packet.
>
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> v1->v2:
> - extended patch description
> ---
>  drivers/net/virtio_net.c        | 6 ++++--
>  include/uapi/linux/virtio_net.h | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fb5e68ed3ec2..e85b03988733 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -62,7 +62,8 @@ static const unsigned long guest_offloads[] = {
>         VIRTIO_NET_F_GUEST_UFO,
>         VIRTIO_NET_F_GUEST_CSUM,
>         VIRTIO_NET_F_GUEST_USO4,
> -       VIRTIO_NET_F_GUEST_USO6
> +       VIRTIO_NET_F_GUEST_USO6,
> +       VIRTIO_NET_F_GUEST_HDRLEN
>  };
>
>  #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> @@ -4213,7 +4214,8 @@ static struct virtio_device_id id_table[] = {
>         VIRTIO_NET_F_CTRL_MAC_ADDR, \
>         VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>         VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> -       VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL
> +       VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL, \
> +       VIRTIO_NET_F_GUEST_HDRLEN
>
>  static unsigned int features[] = {
>         VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index b4062bed186a..12c1c9699935 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -61,6 +61,7 @@
>  #define VIRTIO_NET_F_GUEST_USO6        55      /* Guest can handle USOv6 in. */
>  #define VIRTIO_NET_F_HOST_USO  56      /* Host can handle USO in. */
>  #define VIRTIO_NET_F_HASH_REPORT  57   /* Supports hash report */
> +#define VIRTIO_NET_F_GUEST_HDRLEN  59  /* Guest provides the exact hdr_len value. */
>  #define VIRTIO_NET_F_RSS         60    /* Supports RSS RX steering */
>  #define VIRTIO_NET_F_RSC_EXT     61    /* extended coalescing info */
>  #define VIRTIO_NET_F_STANDBY     62    /* Act as standby for another device
> --
> 2.39.0
>

This indeed saves some cycles in our device.
Thanks!

Reviewed-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
Jiri Pirko Feb. 21, 2023, 3:39 p.m. UTC | #4 
Tue, Feb 21, 2023 at 04:11:53PM CET, willemdebruijn.kernel@gmail.com wrote:
>Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
>> set implicates that the driver provides the exact size of the header.
>> 
>> Quoting the original virtio spec:
>> "hdr_len is a hint to the device as to how much of the header needs to
>>  be kept to copy into each packet"
>> 
>> "a hint" might not be clear for the reader what does it mean, if it is
>> "maybe like that" of "exactly like that". This feature just makes it
>> crystal clear and let the device count on the hdr_len being filled up
>> by the exact length of header.
>> 
>> Also note the spec already has following note about hdr_len:
>> "Due to various bugs in implementations, this field is not useful
>>  as a guarantee of the transport header size."
>> 
>> Without this feature the device needs to parse the header in core
>> data path handling. Accurate information helps the device to eliminate
>> such header parsing and directly use the hardware accelerators
>> for GSO operation.
>> 
>> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
>> The driver already complies to fill the correct value. Introduce the
>> feature and advertise it.
>> 
>> Note that virtio spec also includes following note for device
>> implementation:
>> "Caution should be taken by the implementation so as to prevent
>>  a malicious driver from attacking the device by setting
>>  an incorrect hdr_len."
>> 
>> There is a plan to support this feature in our emulated device.
>> A device of SolidRun offers this feature bit. They claim this feature
>> will save the device a few cycles for every GSO packet.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> v1->v2:
>> - extended patch description
>
>Is the expectation that in-kernel devices support this feature, and
>if so how would it affect them? If I read the spec correctly, devices

Well, the tap driver actually trusts the hdr_len to be of correct header
size nowadays.


>still need to be careful against malicious drivers, so cannot assume
>much beyond what they do today (i.e., a hint).

Malicious how? There is upper limit of size in tap which is checked.
I assume that for hw implementation, that would be the same.

But anyway, this discussion would be rather part of the spec/device
patch, don't you think?


>
>Might be good to point to the definition commit:
>https://github.com/oasis-tcs/virtio-spec/commit/4f1981a1ff46b7aeb801c4c524ff76e93d9ce022

There were couple of fixes to the spec since then, that's why I didn't
include it. It is trivial to look it up in the spec.
Willem de Bruijn Feb. 21, 2023, 4:12 p.m. UTC | #5 
Jiri Pirko wrote:
> Tue, Feb 21, 2023 at 04:11:53PM CET, willemdebruijn.kernel@gmail.com wrote:
> >Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >> 
> >> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
> >> set implicates that the driver provides the exact size of the header.
> >> 
> >> Quoting the original virtio spec:
> >> "hdr_len is a hint to the device as to how much of the header needs to
> >>  be kept to copy into each packet"
> >> 
> >> "a hint" might not be clear for the reader what does it mean, if it is
> >> "maybe like that" of "exactly like that". This feature just makes it
> >> crystal clear and let the device count on the hdr_len being filled up
> >> by the exact length of header.
> >> 
> >> Also note the spec already has following note about hdr_len:
> >> "Due to various bugs in implementations, this field is not useful
> >>  as a guarantee of the transport header size."
> >> 
> >> Without this feature the device needs to parse the header in core
> >> data path handling. Accurate information helps the device to eliminate
> >> such header parsing and directly use the hardware accelerators
> >> for GSO operation.
> >> 
> >> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
> >> The driver already complies to fill the correct value. Introduce the
> >> feature and advertise it.
> >> 
> >> Note that virtio spec also includes following note for device
> >> implementation:
> >> "Caution should be taken by the implementation so as to prevent
> >>  a malicious driver from attacking the device by setting
> >>  an incorrect hdr_len."
> >> 
> >> There is a plan to support this feature in our emulated device.
> >> A device of SolidRun offers this feature bit. They claim this feature
> >> will save the device a few cycles for every GSO packet.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> ---
> >> v1->v2:
> >> - extended patch description
> >
> >Is the expectation that in-kernel devices support this feature, and
> >if so how would it affect them? If I read the spec correctly, devices
> 
> Well, the tap driver actually trusts the hdr_len to be of correct header
> size nowadays.

tap_get_user performs basic bounds checking on the length passed.
 
> 
> >still need to be careful against malicious drivers, so cannot assume
> >much beyond what they do today (i.e., a hint).
> 
> Malicious how? There is upper limit of size in tap which is checked.
> I assume that for hw implementation, that would be the same.

A device cannot blindly trust a hdr_len passed from a driver. We have
had bugs in the kernel with this before, such as the one fixed in
commit 57031eb79490 ("packet: round up linear to header len").

> But anyway, this discussion would be rather part of the spec/device
> patch, don't you think?

I disagree. If it's not much effort to make a commit self-documenting
that is preferable. And if not, then an explicit reference to an
authoratitive external reference is preferable over "it is trivial to
look it up".
 
> 
> >
> >Might be good to point to the definition commit:
> >https://github.com/oasis-tcs/virtio-spec/commit/4f1981a1ff46b7aeb801c4c524ff76e93d9ce022
> 
> There were couple of fixes to the spec since then, that's why I didn't
> include it. It is trivial to look it up in the spec.
Michael S. Tsirkin Feb. 21, 2023, 5:21 p.m. UTC | #6 
On Tue, Feb 21, 2023 at 03:47:41PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
> set implicates that the driver provides the exact size of the header.

OK but I feel this is not the important point. The important points are:
- this bit means device needs this info
- driver also has to set this bit
For example one might replace above with:

	Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
	which when set implicates that device benefits from knowing the exact
	size of the header. For compatiblity, to signal to the device that the header
	is reliable driver also needs to set this feature.
	Without this feature set by driver, device has to figure
	out the header size itself.

and the below is ok.

> Quoting the original virtio spec:
> "hdr_len is a hint to the device as to how much of the header needs to
>  be kept to copy into each packet"
> 
> "a hint" might not be clear for the reader what does it mean, if it is
> "maybe like that" of "exactly like that". This feature just makes it
> crystal clear and let the device count on the hdr_len being filled up
> by the exact length of header.
> 
> Also note the spec already has following note about hdr_len:
> "Due to various bugs in implementations, this field is not useful
>  as a guarantee of the transport header size."
> 
> Without this feature the device needs to parse the header in core
> data path handling. Accurate information helps the device to eliminate
> such header parsing and directly use the hardware accelerators
> for GSO operation.
> 
> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
> The driver already complies to fill the correct value. Introduce the
> feature and advertise it.
> 
> Note that virtio spec also includes following note for device
> implementation:
> "Caution should be taken by the implementation so as to prevent
>  a malicious driver from attacking the device by setting
>  an incorrect hdr_len."
> 
> There is a plan to support this feature in our emulated device.
> A device of SolidRun offers this feature bit. They claim this feature
> will save the device a few cycles for every GSO packet.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

I'm fine with patch itself. with commit log tweak:

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> ---
> v1->v2:
> - extended patch description
> ---
>  drivers/net/virtio_net.c        | 6 ++++--
>  include/uapi/linux/virtio_net.h | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fb5e68ed3ec2..e85b03988733 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -62,7 +62,8 @@ static const unsigned long guest_offloads[] = {
>  	VIRTIO_NET_F_GUEST_UFO,
>  	VIRTIO_NET_F_GUEST_CSUM,
>  	VIRTIO_NET_F_GUEST_USO4,
> -	VIRTIO_NET_F_GUEST_USO6
> +	VIRTIO_NET_F_GUEST_USO6,
> +	VIRTIO_NET_F_GUEST_HDRLEN
>  };
>  
>  #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
> @@ -4213,7 +4214,8 @@ static struct virtio_device_id id_table[] = {
>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>  	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>  	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> -	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL
> +	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL, \
> +	VIRTIO_NET_F_GUEST_HDRLEN
>  
>  static unsigned int features[] = {
>  	VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index b4062bed186a..12c1c9699935 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -61,6 +61,7 @@
>  #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can handle USOv6 in. */
>  #define VIRTIO_NET_F_HOST_USO	56	/* Host can handle USO in. */
>  #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
> +#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest provides the exact hdr_len value. */
>  #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
>  #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
>  #define VIRTIO_NET_F_STANDBY	  62	/* Act as standby for another device
> -- 
> 2.39.0
Michael S. Tsirkin Feb. 21, 2023, 5:39 p.m. UTC | #7 
On Tue, Feb 21, 2023 at 04:39:32PM +0100, Jiri Pirko wrote:
> Tue, Feb 21, 2023 at 04:11:53PM CET, willemdebruijn.kernel@gmail.com wrote:
> >Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >> 
> >> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
> >> set implicates that the driver provides the exact size of the header.
> >> 
> >> Quoting the original virtio spec:
> >> "hdr_len is a hint to the device as to how much of the header needs to
> >>  be kept to copy into each packet"
> >> 
> >> "a hint" might not be clear for the reader what does it mean, if it is
> >> "maybe like that" of "exactly like that". This feature just makes it
> >> crystal clear and let the device count on the hdr_len being filled up
> >> by the exact length of header.
> >> 
> >> Also note the spec already has following note about hdr_len:
> >> "Due to various bugs in implementations, this field is not useful
> >>  as a guarantee of the transport header size."
> >> 
> >> Without this feature the device needs to parse the header in core
> >> data path handling. Accurate information helps the device to eliminate
> >> such header parsing and directly use the hardware accelerators
> >> for GSO operation.
> >> 
> >> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
> >> The driver already complies to fill the correct value. Introduce the
> >> feature and advertise it.
> >> 
> >> Note that virtio spec also includes following note for device
> >> implementation:
> >> "Caution should be taken by the implementation so as to prevent
> >>  a malicious driver from attacking the device by setting
> >>  an incorrect hdr_len."
> >> 
> >> There is a plan to support this feature in our emulated device.
> >> A device of SolidRun offers this feature bit. They claim this feature
> >> will save the device a few cycles for every GSO packet.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> ---
> >> v1->v2:
> >> - extended patch description
> >
> >Is the expectation that in-kernel devices support this feature, and
> >if so how would it affect them? If I read the spec correctly, devices
> 
> Well, the tap driver actually trusts the hdr_len to be of correct header
> size nowadays.
> 
> 
> >still need to be careful against malicious drivers, so cannot assume
> >much beyond what they do today (i.e., a hint).
> 
> Malicious how? There is upper limit of size in tap which is checked.
> I assume that for hw implementation, that would be the same.
> 
> But anyway, this discussion would be rather part of the spec/device
> patch, don't you think?
> 
> 
> >
> >Might be good to point to the definition commit:
> >https://github.com/oasis-tcs/virtio-spec/commit/4f1981a1ff46b7aeb801c4c524ff76e93d9ce022
> 
> There were couple of fixes to the spec since then, that's why I didn't
> include it. It is trivial to look it up in the spec.

This might be a good link:

https://docs.oasis-open.org/virtio/virtio/v1.2/cs01/virtio-v1.2-cs01.html#x1-230006x3
Jiri Pirko Feb. 22, 2023, 7:58 a.m. UTC | #8 
Tue, Feb 21, 2023 at 05:12:33PM CET, willemdebruijn.kernel@gmail.com wrote:
>Jiri Pirko wrote:
>> Tue, Feb 21, 2023 at 04:11:53PM CET, willemdebruijn.kernel@gmail.com wrote:
>> >Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@nvidia.com>
>> >> 
>> >> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
>> >> set implicates that the driver provides the exact size of the header.
>> >> 
>> >> Quoting the original virtio spec:
>> >> "hdr_len is a hint to the device as to how much of the header needs to
>> >>  be kept to copy into each packet"
>> >> 
>> >> "a hint" might not be clear for the reader what does it mean, if it is
>> >> "maybe like that" of "exactly like that". This feature just makes it
>> >> crystal clear and let the device count on the hdr_len being filled up
>> >> by the exact length of header.
>> >> 
>> >> Also note the spec already has following note about hdr_len:
>> >> "Due to various bugs in implementations, this field is not useful
>> >>  as a guarantee of the transport header size."
>> >> 
>> >> Without this feature the device needs to parse the header in core
>> >> data path handling. Accurate information helps the device to eliminate
>> >> such header parsing and directly use the hardware accelerators
>> >> for GSO operation.
>> >> 
>> >> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
>> >> The driver already complies to fill the correct value. Introduce the
>> >> feature and advertise it.
>> >> 
>> >> Note that virtio spec also includes following note for device
>> >> implementation:
>> >> "Caution should be taken by the implementation so as to prevent
>> >>  a malicious driver from attacking the device by setting
>> >>  an incorrect hdr_len."
>> >> 
>> >> There is a plan to support this feature in our emulated device.
>> >> A device of SolidRun offers this feature bit. They claim this feature
>> >> will save the device a few cycles for every GSO packet.
>> >> 
>> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> >> ---
>> >> v1->v2:
>> >> - extended patch description
>> >
>> >Is the expectation that in-kernel devices support this feature, and
>> >if so how would it affect them? If I read the spec correctly, devices
>> 
>> Well, the tap driver actually trusts the hdr_len to be of correct header
>> size nowadays.
>
>tap_get_user performs basic bounds checking on the length passed.

Sure. It trusts the hdr_len, but it sanitizes the input.


> 
>> 
>> >still need to be careful against malicious drivers, so cannot assume
>> >much beyond what they do today (i.e., a hint).
>> 
>> Malicious how? There is upper limit of size in tap which is checked.
>> I assume that for hw implementation, that would be the same.
>
>A device cannot blindly trust a hdr_len passed from a driver. We have
>had bugs in the kernel with this before, such as the one fixed in
>commit 57031eb79490 ("packet: round up linear to header len").
>
>> But anyway, this discussion would be rather part of the spec/device
>> patch, don't you think?
>
>I disagree. If it's not much effort to make a commit self-documenting
>that is preferable. And if not, then an explicit reference to an
>authoratitive external reference is preferable over "it is trivial to
>look it up".

Sorry, I don't follow. What exactly do you want me to do?


> 
>> 
>> >
>> >Might be good to point to the definition commit:
>> >https://github.com/oasis-tcs/virtio-spec/commit/4f1981a1ff46b7aeb801c4c524ff76e93d9ce022
>> 
>> There were couple of fixes to the spec since then, that's why I didn't
>> include it. It is trivial to look it up in the spec.
Jiri Pirko Feb. 22, 2023, 7:59 a.m. UTC | #9 
Tue, Feb 21, 2023 at 06:21:16PM CET, mst@redhat.com wrote:
>On Tue, Feb 21, 2023 at 03:47:41PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
>> set implicates that the driver provides the exact size of the header.
>
>OK but I feel this is not the important point. The important points are:
>- this bit means device needs this info
>- driver also has to set this bit
>For example one might replace above with:
>
>	Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
>	which when set implicates that device benefits from knowing the exact
>	size of the header. For compatiblity, to signal to the device that the header
>	is reliable driver also needs to set this feature.
>	Without this feature set by driver, device has to figure
>	out the header size itself.
>
>and the below is ok.
>
>> Quoting the original virtio spec:
>> "hdr_len is a hint to the device as to how much of the header needs to
>>  be kept to copy into each packet"
>> 
>> "a hint" might not be clear for the reader what does it mean, if it is
>> "maybe like that" of "exactly like that". This feature just makes it
>> crystal clear and let the device count on the hdr_len being filled up
>> by the exact length of header.
>> 
>> Also note the spec already has following note about hdr_len:
>> "Due to various bugs in implementations, this field is not useful
>>  as a guarantee of the transport header size."
>> 
>> Without this feature the device needs to parse the header in core
>> data path handling. Accurate information helps the device to eliminate
>> such header parsing and directly use the hardware accelerators
>> for GSO operation.
>> 
>> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
>> The driver already complies to fill the correct value. Introduce the
>> feature and advertise it.
>> 
>> Note that virtio spec also includes following note for device
>> implementation:
>> "Caution should be taken by the implementation so as to prevent
>>  a malicious driver from attacking the device by setting
>>  an incorrect hdr_len."
>> 
>> There is a plan to support this feature in our emulated device.
>> A device of SolidRun offers this feature bit. They claim this feature
>> will save the device a few cycles for every GSO packet.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>
>I'm fine with patch itself. with commit log tweak:
>
>Acked-by: Michael S. Tsirkin <mst@redhat.com>

Okay. Will do. I will put link to the spec as well

Thanks!

>
>
>> ---
>> v1->v2:
>> - extended patch description
>> ---
>>  drivers/net/virtio_net.c        | 6 ++++--
>>  include/uapi/linux/virtio_net.h | 1 +
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index fb5e68ed3ec2..e85b03988733 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -62,7 +62,8 @@ static const unsigned long guest_offloads[] = {
>>  	VIRTIO_NET_F_GUEST_UFO,
>>  	VIRTIO_NET_F_GUEST_CSUM,
>>  	VIRTIO_NET_F_GUEST_USO4,
>> -	VIRTIO_NET_F_GUEST_USO6
>> +	VIRTIO_NET_F_GUEST_USO6,
>> +	VIRTIO_NET_F_GUEST_HDRLEN
>>  };
>>  
>>  #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
>> @@ -4213,7 +4214,8 @@ static struct virtio_device_id id_table[] = {
>>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>  	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>  	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
>> -	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL
>> +	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL, \
>> +	VIRTIO_NET_F_GUEST_HDRLEN
>>  
>>  static unsigned int features[] = {
>>  	VIRTNET_FEATURES,
>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>> index b4062bed186a..12c1c9699935 100644
>> --- a/include/uapi/linux/virtio_net.h
>> +++ b/include/uapi/linux/virtio_net.h
>> @@ -61,6 +61,7 @@
>>  #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can handle USOv6 in. */
>>  #define VIRTIO_NET_F_HOST_USO	56	/* Host can handle USO in. */
>>  #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
>> +#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest provides the exact hdr_len value. */
>>  #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
>>  #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
>>  #define VIRTIO_NET_F_STANDBY	  62	/* Act as standby for another device
>> -- 
>> 2.39.0
>
Willem de Bruijn Feb. 22, 2023, 3:14 p.m. UTC | #10 
Jiri Pirko wrote:
> Tue, Feb 21, 2023 at 05:12:33PM CET, willemdebruijn.kernel@gmail.com wrote:
> >Jiri Pirko wrote:
> >> Tue, Feb 21, 2023 at 04:11:53PM CET, willemdebruijn.kernel@gmail.com wrote:
> >> >Jiri Pirko wrote:
> >> >> From: Jiri Pirko <jiri@nvidia.com>
> >> >> 
> >> >> Virtio spec introduced a feature VIRTIO_NET_F_GUEST_HDRLEN which when
> >> >> set implicates that the driver provides the exact size of the header.
> >> >> 
> >> >> Quoting the original virtio spec:
> >> >> "hdr_len is a hint to the device as to how much of the header needs to
> >> >>  be kept to copy into each packet"
> >> >> 
> >> >> "a hint" might not be clear for the reader what does it mean, if it is
> >> >> "maybe like that" of "exactly like that". This feature just makes it
> >> >> crystal clear and let the device count on the hdr_len being filled up
> >> >> by the exact length of header.
> >> >> 
> >> >> Also note the spec already has following note about hdr_len:
> >> >> "Due to various bugs in implementations, this field is not useful
> >> >>  as a guarantee of the transport header size."
> >> >> 
> >> >> Without this feature the device needs to parse the header in core
> >> >> data path handling. Accurate information helps the device to eliminate
> >> >> such header parsing and directly use the hardware accelerators
> >> >> for GSO operation.
> >> >> 
> >> >> virtio_net_hdr_from_skb() fills up hdr_len to skb_headlen(skb).
> >> >> The driver already complies to fill the correct value. Introduce the
> >> >> feature and advertise it.
> >> >> 
> >> >> Note that virtio spec also includes following note for device
> >> >> implementation:
> >> >> "Caution should be taken by the implementation so as to prevent
> >> >>  a malicious driver from attacking the device by setting
> >> >>  an incorrect hdr_len."
> >> >> 
> >> >> There is a plan to support this feature in our emulated device.
> >> >> A device of SolidRun offers this feature bit. They claim this feature
> >> >> will save the device a few cycles for every GSO packet.
> >> >> 
> >> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> >> ---
> >> >> v1->v2:
> >> >> - extended patch description
> >> >
> >> >Is the expectation that in-kernel devices support this feature, and
> >> >if so how would it affect them? If I read the spec correctly, devices
> >> 
> >> Well, the tap driver actually trusts the hdr_len to be of correct header
> >> size nowadays.
> >
> >tap_get_user performs basic bounds checking on the length passed.
> 
> Sure. It trusts the hdr_len, but it sanitizes the input.
>
> 
> > 
> >> 
> >> >still need to be careful against malicious drivers, so cannot assume
> >> >much beyond what they do today (i.e., a hint).
> >> 
> >> Malicious how? There is upper limit of size in tap which is checked.
> >> I assume that for hw implementation, that would be the same.
> >
> >A device cannot blindly trust a hdr_len passed from a driver. We have
> >had bugs in the kernel with this before, such as the one fixed in
> >commit 57031eb79490 ("packet: round up linear to header len").
> >
> >> But anyway, this discussion would be rather part of the spec/device
> >> patch, don't you think?
> >
> >I disagree. If it's not much effort to make a commit self-documenting
> >that is preferable. And if not, then an explicit reference to an
> >authoratitive external reference is preferable over "it is trivial to
> >look it up".
> 
> Sorry, I don't follow. What exactly do you want me to do?

Either including the link that Michael shared or quoting the relevant
part verbatim in the commit message would help, thanks.

Thinking it over, my main concern is that the prescriptive section in
the spec does not state what to do when the value is clearly garbage,
as we have seen with syzkaller.

Having to sanitize input, by dropping if < ETH_HLEN or > length, to
me means that the device cannot trust the field, as the spec says it
should. 

Sanitization is harder in the kernel, because it has to support all
kinds of link layers, including variable length.

Perhaps that's a discussion for the spec rather than this commit. But
it's a point to clarify as we add support to the code.
Michael S. Tsirkin Feb. 22, 2023, 4:11 p.m. UTC | #11 
On Wed, Feb 22, 2023 at 10:14:21AM -0500, Willem de Bruijn wrote:
> Either including the link that Michael shared or quoting the relevant
> part verbatim in the commit message would help, thanks.
> 
> Thinking it over, my main concern is that the prescriptive section in
> the spec does not state what to do when the value is clearly garbage,
> as we have seen with syzkaller.
> 
> Having to sanitize input, by dropping if < ETH_HLEN or > length, to
> me means that the device cannot trust the field, as the spec says it
> should. 

Right. I think the implication is that if device detects and illegal
value it's OK for it to just drop the packet or reset or enter
a broken mode until reset.

By contrast without the feature bit the header size can be
used as a hint e.g. to size allocations but you must
recover if it's incorrect.

And yes tap seems to break if you make it too small or if you make
it huge so it does not really follow the spec in this regard.

Setting the flag will not fix tap because we can't really
affort breaking all drivers who don't set it. But it will
prepare the ground for when tens of years from now we
actually look back and say all drivers set it, no problem.

So that's a good reason to ack this patch.

However if someone is worried about this then fixing tap
so it recovers from incorrect header length without
packet loss is a good idea.

> Sanitization is harder in the kernel, because it has to support all
> kinds of link layers, including variable length.
> 
> Perhaps that's a discussion for the spec rather than this commit. But
> it's a point to clarify as we add support to the code.
Willem de Bruijn Feb. 22, 2023, 5:22 p.m. UTC | #12 
Michael S. Tsirkin wrote:
> On Wed, Feb 22, 2023 at 10:14:21AM -0500, Willem de Bruijn wrote:
> > Either including the link that Michael shared or quoting the relevant
> > part verbatim in the commit message would help, thanks.
> > 
> > Thinking it over, my main concern is that the prescriptive section in
> > the spec does not state what to do when the value is clearly garbage,
> > as we have seen with syzkaller.
> > 
> > Having to sanitize input, by dropping if < ETH_HLEN or > length, to
> > me means that the device cannot trust the field, as the spec says it
> > should. 
> 
> Right. I think the implication is that if device detects and illegal
> value it's OK for it to just drop the packet or reset or enter
> a broken mode until reset.
> 
> By contrast without the feature bit the header size can be
> used as a hint e.g. to size allocations but you must
> recover if it's incorrect.
> 
> And yes tap seems to break if you make it too small or if you make
> it huge so it does not really follow the spec in this regard.
> 
> Setting the flag will not fix tap because we can't really
> affort breaking all drivers who don't set it. But it will
> prepare the ground for when tens of years from now we
> actually look back and say all drivers set it, no problem.
> 
> So that's a good reason to ack this patch.

I also have no concerns with the commit itself. It would become an
issue only if tap would support it and trust hdr_len unconditionally.

Acked-by: Willem de Bruijn <willemb@google.com>
 
> However if someone is worried about this then fixing tap
> so it recovers from incorrect header length without
> packet loss is a good idea.
> 
> > Sanitization is harder in the kernel, because it has to support all
> > kinds of link layers, including variable length.
> > 
> > Perhaps that's a discussion for the spec rather than this commit. But
> > it's a point to clarify as we add support to the code.
> 
> -- 
> MST
>
diff mbox series

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fb5e68ed3ec2..e85b03988733 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -62,7 +62,8 @@  static const unsigned long guest_offloads[] = {
 	VIRTIO_NET_F_GUEST_UFO,
 	VIRTIO_NET_F_GUEST_CSUM,
 	VIRTIO_NET_F_GUEST_USO4,
-	VIRTIO_NET_F_GUEST_USO6
+	VIRTIO_NET_F_GUEST_USO6,
+	VIRTIO_NET_F_GUEST_HDRLEN
 };
 
 #define GUEST_OFFLOAD_GRO_HW_MASK ((1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
@@ -4213,7 +4214,8 @@  static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_CTRL_MAC_ADDR, \
 	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
 	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
-	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL
+	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL, \
+	VIRTIO_NET_F_GUEST_HDRLEN
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index b4062bed186a..12c1c9699935 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -61,6 +61,7 @@ 
 #define VIRTIO_NET_F_GUEST_USO6	55	/* Guest can handle USOv6 in. */
 #define VIRTIO_NET_F_HOST_USO	56	/* Host can handle USO in. */
 #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
+#define VIRTIO_NET_F_GUEST_HDRLEN  59	/* Guest provides the exact hdr_len value. */
 #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
 #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
 #define VIRTIO_NET_F_STANDBY	  62	/* Act as standby for another device