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

Tue, Feb 21, 2023 at 05:12:33PM CET, willemdebruijn.kernel@gmail.com wrote:

Jiri Pirko wrote:

quoted

Tue, Feb 21, 2023 at 04:11:53PM CET, willemdebruijn.kernel@gmail.com wrote:

quoted

Jiri Pirko wrote:

quoted

From: Jiri Pirko <redacted>

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 <redacted>
---
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.

quoted

quoted

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").

quoted

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?

quoted

quoted

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.