Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virtio-net: correct conditions for devices to validate packet checksum #185

Open
hengqiali opened this issue Jan 3, 2024 · 4 comments
Open

Comments

@hengqiali
Copy link

hengqiali commented Jan 3, 2024

There is a historical error in virtio spec:
"If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags
to zero and SHOULD supply a fully checksummed packet to the driver."

Currently in Linux and virtio-related implementations, the device validates
the packet checksum and sets DATA_VALID regardless of whether
VIRTIO_NET_F_GUEST_CSUM is negotiated.

Please refer to the following summary for details and reasons:

Summary

  1. GUEST_CSUM at virtio spec 0.95 is intended to be compatible with partially
    checksummed packets (NEEDS_CSUM <-> CHECKSUM_PARTIAL). So GUEST_CSUM is mapped
    to NETIF_F_RXCSUM.
    GUEST_CSUM only indicates whether the driver handles partially checksummed packets.
    When XDP is loaded, the GUEST_CSUM's offload will be disabled, which means that
    packets have NEEDS_CSUM set will be dropped, and packets have DATA_VALID set will
    still be received.

  2. When DATA_VALID was added to Linux in 2011[2] and virtio1.0, it was actually
    expected that rx checksum offload (the driver can set CHECKSUM_UNNECESSARY) had
    nothing to do with whether GUEST_CSUM was negotiated. But due to an error, below
    desctiption was added incorrectly:
    "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags
    to zero and SHOULD supply a fully checksummed packet to the driver."

  3. We now hope to correct this error. Let the setting of DATA_VALID not be
    controlled by whether GUEST_CSUM is negotiated, but only controlled by whether
    rx checksum offload is enabled on the OS side. The state of this rx checksum
    offload is not aware of the device.

  4. [Optional] NETIF_F_RXCSUM corresponding to rx checksum offload should be
    added to dev->hw_features. When the user turns off rx checksum offload through
    ethtool -K, neither NEEDS_CSUM nor DATA_VALID should be taken care of, that is,
    all packets will be marked as CHECKSUM_NONE.

Solution: https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html
(This patch already has Jason and Xuan's R-B )

@hengqiali
Copy link
Author

hengqiali commented Jun 6, 2024

This patch already has Jason and Xuan's R-B[1][2], and Huck has said that there is no need to post a new version to carry their R-B tags, so I ask for a friendly vote.
(If you think a new version is needed, I can re-post V9.)

[1] Jason's R-B link: https://lists.oasis-open.org/archives/virtio-comment/202401/msg00020.html
[2] Xuan's R-B link: https://lists.oasis-open.org/archives/virtio-comment/202401/msg00072.html

@mstsirkin
Copy link
Contributor

It's ok but please edit the issue description making sure the first link included is the
proposal patchset. It is unclear which of the links is the proposal.

@hengqiali
Copy link
Author

It's ok but please edit the issue description making sure the first link included is the proposal patchset. It is unclear which of the links is the proposal.

Fixed.

@mstsirkin
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants