Bug 248306 - if_mvneta: Corrupts TX packets when TXCSUM is not used
Summary: if_mvneta: Corrupts TX packets when TXCSUM is not used
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: arm64 Any
: --- Affects Many People
Assignee: freebsd-net (Nobody)
URL:
Keywords: needs-qa
Depends on:
Blocks:
 
Reported: 2020-07-27 18:48 UTC by Mike Cui
Modified: 2020-08-01 09:41 UTC (History)
5 users (show)

See Also:
koobs: mfc-stable12?
koobs: mfc-stable11?


Attachments
Fix (1.12 KB, patch)
2020-07-27 18:48 UTC, Mike Cui
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Cui 2020-07-27 18:48:50 UTC
Created attachment 216815 [details]
Fix

The mvneta device requires MVNETA_TX_CMD_L4_CHECKSUM_NONE bit to be set in the tx descriptor is checksum not required. However, mvneta_tx_set_csumflag() is not setting this flag currently, causing the hardware to randomly corrupt IP header during transmission.

This affects injected IPv4 packets that skips kernel IP stack processing (e.g. DHCP), as well as all IPv6 packets, since the driver currently does not offload csum for IPv6.

The fix is to remove all the early return paths from mvneta_tx_set_csumflag() which do not set the MVNETA_TX_CMD_L4_CHECKSUM_NONE flag.
Comment 1 Bjoern A. Zeeb freebsd_committer 2020-07-27 20:59:54 UTC
Oh, wow, this might explain some corrupted IPv6 RA packets I have seen.

Are you sure that for the VLAN case we don't have to distinguish anything further?
Comment 2 Mark Johnston freebsd_committer 2020-07-27 21:07:27 UTC
The patch looks right to me.  cc'ed mw@ since this looks like a regression from r357676.
Comment 3 Bjoern A. Zeeb freebsd_committer 2020-07-27 21:54:18 UTC
(In reply to Mark Johnston from comment #2)

Yes, I read the code in the mean time and it seems it is correct currently.

The comment disabling IFCAP_HWCSUM_IPV6 made me wonder, but that's a different issue to the one here.
Comment 4 Mike Cui 2020-07-27 22:19:27 UTC
(In reply to Mark Johnston from comment #2)
Even before r357676, the early return in the switch block is corrupting IPv6 packets. This patch fixes both.
Comment 5 mw 2020-07-28 06:12:44 UTC
(In reply to Mike Cui from comment #4)

Thanks for the patch, LGTM. Do you want me to commit it?
Comment 6 Mike Cui 2020-07-30 05:51:06 UTC
(In reply to mw from comment #5)
Yes, please. I don't know how to commit it myself.

Side question: what's the process to get the latest version of this driver, as well as the accompanying e6000sw etherswitch driver merged into stable/12 so they make it into 12.2? I manually cherry-picked these changes into 12-STABLE and I've been running with them. They work fine.
Comment 7 mw 2020-07-30 06:13:07 UTC
I can commit the patch then.

About updating e6000sw & if_mvneta on stable/12, do you have maybe a list of commit that you locally backported and tested?
Comment 8 Mike Cui 2020-07-30 06:50:59 UTC
(In reply to mw from comment #7)
Here's a git tree with my backports:
https://github.com/iucoen/freebsd-espressobin/commits/stable/12

There is one patch, titled "Set the allocated memory as coherent where is necessary", that I got from pfsense's tree based on 11.2. I'm not sure if that's strictly needed. You can ignore my own patch "Build espressobin-v7.dts", or maybe rework it to build the v7 DTS in addition.

I've tested on on espressobin-v7, currently running as my home dual-stack router, so I've tested both IPv4 and IPv6.
Comment 9 commit-hook freebsd_committer 2020-08-01 09:41:08 UTC
A commit references this bug:

Author: mw
Date: Sat Aug  1 09:40:19 UTC 2020
New revision: 363759
URL: https://svnweb.freebsd.org/changeset/base/363759

Log:
  Fix TX csum handling in if_mvneta

  The mvneta device requires MVNETA_TX_CMD_L4_CHECKSUM_NONE bit to be set in the tx descriptor is checksum not required. However, mvneta_tx_set_csumflag() is not setting this flag currently, causing the hardware to randomly corrupt IP header during transmission.

  This affects injected IPv4 packets that skips kernel IP stack processing (e.g. DHCP), as well as all IPv6 packets, since the driver currently does not offload csum for IPv6.

  The fix is to remove all the early return paths from mvneta_tx_set_csumflag() which do not set the MVNETA_TX_CMD_L4_CHECKSUM_NONE flag.

  PR: 248306
  Submitted by: Mike Cui <cuicui@gmail.com>
  Reported by: Mike Cui <cuicui@gmail.com>

Changes:
  head/sys/dev/neta/if_mvneta.c