Hello, I'm using stable/12 (aba2dc46dfa5, Oct 24 2021). I'm hitting some problems with if_vlan + parent interface netmap. It was working before the https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=258258 was fixed. Maybe something missing for netmap implementation. The way to reproduce: [HostA] <----> [HostB] HostA - ifconfig em1.110 create 10.10.10.2/24 up - ping 10.10.10.1 - tcpdump -eni em1 17:05:11.393411 00:50:56:26:69:ea > 00:0c:29:84:5d:88, ethertype 802.1Q (0x8100), length 102: vlan 110, p 0, ethertype IPv4, 10.10.10.1 > 10.10.10.2: ICMP echo reply, id 32844, seq 53, length 64 HostB - ifconfig em1.110 create 10.10.10.1/24 up - ifconfig em1 promisc -tso -lro -rxcsum -txcsum -tso6 -rxcsum -txcsum -tso6 -rxcsum6 -txcsum6 -vlanhwtag -vlanhwcsum -vlanhwtso - ./bridge -i em1 -i em1^ & # tcpdump -eni em1 17:05:11.391215 00:0c:29:84:5d:88 > 00:50:56:26:69:ea, ethertype IPv4 (0x0800), length 98: 10.10.10.2 > 10.10.10.1: ICMP echo request, id 32844, seq 53, length 64 Pinging from HostA to HostB through if_vlan. When netmap bridge is closed, everything is okey, we can see the original packet on tcpdump. But when netmap bridge is started, packet's vlan header was lost as you can see above. The netmap bridge app is the original tools/tools/netmap/bridge.c application. HostA and HostB connected back to back directly with a patch cable. There is no switch between them. I tried this test on real hardware em, igb and vmware e1000 (em) nics. Problem is easy to reproduce. But there is no such problem on ix and ixl cards. Is it possible to check and fix? Best Regards, Özkan KIRIK
Question: what happens if you don't create the VLAN interface on host B? At least when running netmap. I will try to reproduce this with QEMU-emulated e1000.
on HostB, still vlan headers are lost for incoming packets even with no if_vlans created. But i hit a strange issue to test this. To receive packages without creating em1.110 interface, I removed the vlanhwfilter flag as: - ifconfig em1 -vlanhwfilter tcpdump can receive tagged frames without netmap app. After starting the netmap bridge, tcpdump couldnt receive tagged frames. I checked the ifconfig em1 flags for if the promisc exists and vlanhwfilter not exists. Flags are good: promisc enabled, and vlanhwfilter not enabled. After running the commands below while netmap bridge is running: - ifconfig em1.110 create; ifconfig em1.110 destroy tcpdump could recevie packages but, vlan tags stripped. It's possible to reproduce this issue using jails with single host. Create a virtual host which has at least 3 e1000 interfaces. - em0 for managing host - em1 and em2 should be connected same network use this commands to reproduce: Open two ssh terminal. First terminal: * ifconfig em1.110 create 10.10.10.1/24 up * ifconfig em1 promisc -tso -lro -rxcsum -txcsum -tso6 -rxcsum -txcsum -tso6 -rxcsum6 -txcsum6 -vlanhwtag -vlanhwcsum -vlanhwtso -vlanhwfilter up * ./bridge -i netmap:em1 -i netmap:em1^ 2>/dev/null & * tcpdump -eni em1 Second terminal: * jail -c -n host2 persist vnet vnet.interface=em2 * jexec host2 ifconfig em2 up * jexec host2 ifconfig em2.110 create 10.10.10.2/24 up * jexec host2 ping 10.10.10.1
Created attachment 229767 [details] Patch to fix the issue
I think that after the latest changes, the e1000 drivers (em, lem, igb) are looking at the wrong "capability" bitmaps, and therefore they are basically ignoring your ifconfig commands to disable capabilities (vlanhwtag, rxcsum, ...). In particular, VLAN hw stripping is always done by the device, even if you disable it. This explains why you don't see the VLAN header when using netmap. In the non-netmap case, the stripped VLAN information gets stored in the mbuf metadata, so that the kernel is able to insert that back when passing the packet to tcpdump. In the netmap case, the stripped VLAN information is lost, because netmap has no space for that kind of metadata. The patch I attached fixes the e1000 drivers so that device capabilities are actually used to configure the device accordingly (i.e., VLAN stripping is disabled in the hardware). The patch also does some cleanup to make the e1000 drivers more uniform w.r.t. VLAN/CSUM offload processing. Could you please test the patch for em and igb? The patch is meant for FreeBSD HEAD.
Thank you so much Vincenzo. I'm going to test and report back.
Patch applied and worked properly on HEAD (main branch). Thank you again!
Patch was tested for em driver. I'll test the igb driver tomorrow.
Thanks. FYI https://reviews.freebsd.org/D33154
Tests on igb (Intel i350) also successful. Thank you again. I applied the patches to stable/12 also. It works on both main and stable/12 branch properly.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=e0f4cdba533693bb6ef9d90243acdad89605b150 commit e0f4cdba533693bb6ef9d90243acdad89605b150 Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-08 08:55:04 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2021-12-08 08:55:40 +0000 e1000: fix interface capabilities management The e1000 drivers (em, lem, igb) are currently looking at the iflib copies of the capabilities bitvectors (scctx->isc_capabilities and scctx->isc_capenable) rather than the ifnet ones (ifp->if_capabilities and ifp->if_capenable). However, the latter are the ones that are actually updated by ifconfig and that should be used by the drivers during interface operation. The former are set by the driver on interface attach (for iflib internal use) and should not be used anymore by the driver. This patch fixes the e1000 driver to use the correct bitvectors. PR: 260068 Reviewed by: markj MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D33154 sys/dev/e1000/if_em.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-)
Hi, Would it be possible to test the latest change: https://reviews.freebsd.org/D33156 ? This is meant to be applied against HEAD. Thanks!
Hi Vincenzo, I'll test the D33156 tomorrow. Thanks
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=ecb7f44be90190974ee97389827563882345eb2a commit ecb7f44be90190974ee97389827563882345eb2a Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-08 08:55:04 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2021-12-22 08:50:35 +0000 e1000: fix interface capabilities management The e1000 drivers (em, lem, igb) are currently looking at the iflib copies of the capabilities bitvectors (scctx->isc_capabilities and scctx->isc_capenable) rather than the ifnet ones (ifp->if_capabilities and ifp->if_capenable). However, the latter are the ones that are actually updated by ifconfig and that should be used by the drivers during interface operation. The former are set by the driver on interface attach (for iflib internal use) and should not be used anymore by the driver. This patch fixes the e1000 driver to use the correct bitvectors. PR: 260068 Reviewed by: markj MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D33154 (cherry picked from commit e0f4cdba533693bb6ef9d90243acdad89605b150) sys/dev/e1000/if_em.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=677d9b1694e0062dbd0ceb86f4f13dd30c7af6a4 commit 677d9b1694e0062dbd0ceb86f4f13dd30c7af6a4 Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-08 08:55:04 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2021-12-22 13:44:39 +0000 e1000: fix interface capabilities management The e1000 drivers (em, lem, igb) are currently looking at the iflib copies of the capabilities bitvectors (scctx->isc_capabilities and scctx->isc_capenable) rather than the ifnet ones (ifp->if_capabilities and ifp->if_capenable). However, the latter are the ones that are actually updated by ifconfig and that should be used by the drivers during interface operation. The former are set by the driver on interface attach (for iflib internal use) and should not be used anymore by the driver. This patch fixes the e1000 driver to use the correct bitvectors. PR: 260068 Reviewed by: markj MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D33154 (cherry picked from commit e0f4cdba533693bb6ef9d90243acdad89605b150) sys/dev/e1000/if_em.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=4561c4f0ca59da5b704238074bd488ff907b4b50 commit 4561c4f0ca59da5b704238074bd488ff907b4b50 Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 10:47:13 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2021-12-28 10:55:21 +0000 net: iflib: sync isc_capenable to if_capenable On SIOCSIFCAP, some bits in ifp->if_capenable may be toggled. When this happens, apply the same change to isc_capenable, which is the iflib private copy of if_capenable (for a subset of the IFCAP_* bits). In this way the iflib drivers can check the bits using isc_capenable rather than if_capenable. This is convenient because the latter access requires an additional indirection through the ifp, and it is also less likely to be in cache. PR: 260068 Reviewed by: kbowling, gallatin MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D33156 sys/net/iflib.c | 1 + 1 file changed, 1 insertion(+)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=52f45d8acee95199159b65a33c94142492c38e41 commit 52f45d8acee95199159b65a33c94142492c38e41 Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 11:00:32 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2021-12-28 11:03:52 +0000 net: iflib: let the drivers use isc_capenable Since isc_capenable (private copy of ifp->if_capenable) is now synchronized to if_capenable, use it in the drivers when checking the IFCAP_* bits. This results in better cache usage and avoids indirection through the ifp pointer. PR: 260068 Reviewed by: kbowling, gallatin MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D33156 sys/dev/e1000/em_txrx.c | 2 +- sys/dev/iavf/iavf_txrx_iflib.c | 3 ++- sys/dev/ice/ice_iflib_txrx.c | 3 ++- sys/dev/ixgbe/ix_txrx.c | 6 +++--- sys/dev/ixl/ixl_txrx.c | 3 ++- 5 files changed, 10 insertions(+), 7 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=f7926a6d0c1029c8da265769e7c57b4065faa2df commit f7926a6d0c1029c8da265769e7c57b4065faa2df Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 11:05:31 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2021-12-28 11:11:41 +0000 net: iflib: fix vlan processing in the drivers The logic that sets iri_vtag and M_VLANTAG does not handle the case where the 802.11q VLAN tag is 0. Fix this issue across the iflib drivers. While there, also improve and align the VLAN tag check extraction, by moving it outside the RX descriptor loop, eliminating a local variable and additional checks. PR: 260068 Reviewed by: kbowling, gallatin Reported by: erj MFC after: 1 month Differential Revision: https://reviews.freebsd.org/D33156 sys/dev/e1000/em_txrx.c | 11 ++++------- sys/dev/e1000/igb_txrx.c | 21 +++++++++------------ sys/dev/iavf/iavf_txrx_iflib.c | 13 +++++-------- sys/dev/ice/ice_iflib_txrx.c | 13 +++++-------- sys/dev/igc/igc_txrx.c | 8 +++----- sys/dev/ixgbe/ix_txrx.c | 15 +++++---------- sys/dev/ixl/ixl_txrx.c | 13 +++++-------- 7 files changed, 36 insertions(+), 58 deletions(-)
All the required changes are in HEAD now. Any testing would be really appreciated! Thanks
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=b4a58b3d5831dd2e7e79d9d7cbc3e920803cb4f6 commit b4a58b3d5831dd2e7e79d9d7cbc3e920803cb4f6 Author: Kevin Bowling <kbowling@FreeBSD.org> AuthorDate: 2021-12-29 16:37:34 +0000 Commit: Kevin Bowling <kbowling@FreeBSD.org> CommitDate: 2021-12-29 16:37:34 +0000 igc: Remove redundant IFCAP_VLAN_HWTAGGING check Match igb(4) as in f7926a6d0c10. From Vincenzo, this check is redundant to setup providing us an IGC_RXD_STAT_VP bit and would make for an unexpected condition if IFCAP_VLAN_HWTAGGING were not set but the tag was stripped, which would be passed up the stack breaking isolation. PR: 260068 Approved by: vmaffione MFC after: 1 month sys/dev/igc/igc_txrx.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=6000a417d32a417911ed3d96d3bb3f59fce5a9ae commit 6000a417d32a417911ed3d96d3bb3f59fce5a9ae Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 10:47:13 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2022-01-06 09:53:08 +0000 net: iflib: sync isc_capenable to if_capenable On SIOCSIFCAP, some bits in ifp->if_capenable may be toggled. When this happens, apply the same change to isc_capenable, which is the iflib private copy of if_capenable (for a subset of the IFCAP_* bits). In this way the iflib drivers can check the bits using isc_capenable rather than if_capenable. This is convenient because the latter access requires an additional indirection through the ifp, and it is also less likely to be in cache. PR: 260068 Reviewed by: kbowling, gallatin MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D33156 (cherry picked from commit 4561c4f0ca59da5b704238074bd488ff907b4b50) sys/net/iflib.c | 1 + 1 file changed, 1 insertion(+)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=5df59718ed52e66963e81bb16e400a257745ae96 commit 5df59718ed52e66963e81bb16e400a257745ae96 Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 10:47:13 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2022-01-06 10:14:44 +0000 net: iflib: sync isc_capenable to if_capenable On SIOCSIFCAP, some bits in ifp->if_capenable may be toggled. When this happens, apply the same change to isc_capenable, which is the iflib private copy of if_capenable (for a subset of the IFCAP_* bits). In this way the iflib drivers can check the bits using isc_capenable rather than if_capenable. This is convenient because the latter access requires an additional indirection through the ifp, and it is also less likely to be in cache. PR: 260068 Reviewed by: kbowling, gallatin MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D33156 (cherry picked from commit 4561c4f0ca59da5b704238074bd488ff907b4b50) sys/net/iflib.c | 1 + 1 file changed, 1 insertion(+)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=e99828dfbdd75aeafc2cdb47c43a0a336d61d98c commit e99828dfbdd75aeafc2cdb47c43a0a336d61d98c Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 11:00:32 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2022-01-27 22:26:30 +0000 net: iflib: let the drivers use isc_capenable Since isc_capenable (private copy of ifp->if_capenable) is now synchronized to if_capenable, use it in the drivers when checking the IFCAP_* bits. This results in better cache usage and avoids indirection through the ifp pointer. PR: 260068 Reviewed by: kbowling, gallatin MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D33156 (cherry picked from commit 52f45d8acee95199159b65a33c94142492c38e41) sys/dev/e1000/em_txrx.c | 2 +- sys/dev/ice/ice_iflib_txrx.c | 3 ++- sys/dev/ixgbe/ix_txrx.c | 6 +++--- sys/dev/ixl/ixl_txrx.c | 3 ++- 4 files changed, 8 insertions(+), 6 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=05c9fb008763191f89622a8af2149d14892ce059 commit 05c9fb008763191f89622a8af2149d14892ce059 Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 11:05:31 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2022-01-27 22:39:09 +0000 net: iflib: fix vlan processing in the drivers The logic that sets iri_vtag and M_VLANTAG does not handle the case where the 802.11q VLAN tag is 0. Fix this issue across the iflib drivers. While there, also improve and align the VLAN tag check extraction, by moving it outside the RX descriptor loop, eliminating a local variable and additional checks. PR: 260068 Reviewed by: kbowling, gallatin Reported by: erj MFC after: 1 month Differential Revision: https://reviews.freebsd.org/D33156 (cherry picked from commit f7926a6d0c1029c8da265769e7c57b4065faa2df) sys/dev/e1000/em_txrx.c | 11 ++++------- sys/dev/e1000/igb_txrx.c | 21 +++++++++------------ sys/dev/ice/ice_iflib_txrx.c | 13 +++++-------- sys/dev/igc/igc_txrx.c | 8 +++----- sys/dev/ixgbe/ix_txrx.c | 15 +++++---------- sys/dev/ixl/ixl_txrx.c | 13 +++++-------- 6 files changed, 31 insertions(+), 50 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=cf101bd5ceebe2b2d229faa949dbf3e146d04382 commit cf101bd5ceebe2b2d229faa949dbf3e146d04382 Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 11:05:31 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2022-01-27 22:32:58 +0000 net: iflib: fix vlan processing in the drivers The logic that sets iri_vtag and M_VLANTAG does not handle the case where the 802.11q VLAN tag is 0. Fix this issue across the iflib drivers. While there, also improve and align the VLAN tag check extraction, by moving it outside the RX descriptor loop, eliminating a local variable and additional checks. PR: 260068 Reviewed by: kbowling, gallatin Reported by: erj MFC after: 1 month Differential Revision: https://reviews.freebsd.org/D33156 (cherry picked from commit f7926a6d0c1029c8da265769e7c57b4065faa2df) sys/dev/e1000/em_txrx.c | 11 ++++------- sys/dev/e1000/igb_txrx.c | 21 +++++++++------------ sys/dev/ice/ice_iflib_txrx.c | 13 +++++-------- sys/dev/igc/igc_txrx.c | 8 +++----- sys/dev/ixgbe/ix_txrx.c | 15 +++++---------- sys/dev/ixl/ixl_txrx.c | 13 +++++-------- 6 files changed, 31 insertions(+), 50 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=a61341bf6e51cfc8a1855e9a97391af534da185f commit a61341bf6e51cfc8a1855e9a97391af534da185f Author: Vincenzo Maffione <vmaffione@FreeBSD.org> AuthorDate: 2021-12-28 11:00:32 +0000 Commit: Vincenzo Maffione <vmaffione@FreeBSD.org> CommitDate: 2022-01-27 22:32:16 +0000 net: iflib: let the drivers use isc_capenable Since isc_capenable (private copy of ifp->if_capenable) is now synchronized to if_capenable, use it in the drivers when checking the IFCAP_* bits. This results in better cache usage and avoids indirection through the ifp pointer. PR: 260068 Reviewed by: kbowling, gallatin MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D33156 (cherry picked from commit 52f45d8acee95199159b65a33c94142492c38e41) sys/dev/e1000/em_txrx.c | 2 +- sys/dev/ice/ice_iflib_txrx.c | 3 ++- sys/dev/ixgbe/ix_txrx.c | 6 +++--- sys/dev/ixl/ixl_txrx.c | 3 ++- 4 files changed, 8 insertions(+), 6 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=8082242b96cbd6be7b3c6cb2297ea2d3207185d9 commit 8082242b96cbd6be7b3c6cb2297ea2d3207185d9 Author: Kevin Bowling <kbowling@FreeBSD.org> AuthorDate: 2021-12-29 16:37:34 +0000 Commit: Kevin Bowling <kbowling@FreeBSD.org> CommitDate: 2022-01-30 20:40:02 +0000 igc: Remove redundant IFCAP_VLAN_HWTAGGING check Match igb(4) as in f7926a6d0c10. From Vincenzo, this check is redundant to setup providing us an IGC_RXD_STAT_VP bit and would make for an unexpected condition if IFCAP_VLAN_HWTAGGING were not set but the tag was stripped, which would be passed up the stack breaking isolation. PR: 260068 Approved by: vmaffione (cherry picked from commit b4a58b3d5831dd2e7e79d9d7cbc3e920803cb4f6) sys/dev/igc/igc_txrx.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
A commit in branch stable/12 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=b02fa10699b28b81d35475d23b9812fb15d7e2cb commit b02fa10699b28b81d35475d23b9812fb15d7e2cb Author: Kevin Bowling <kbowling@FreeBSD.org> AuthorDate: 2021-12-29 16:37:34 +0000 Commit: Kevin Bowling <kbowling@FreeBSD.org> CommitDate: 2022-01-30 20:42:28 +0000 igc: Remove redundant IFCAP_VLAN_HWTAGGING check Match igb(4) as in f7926a6d0c10. From Vincenzo, this check is redundant to setup providing us an IGC_RXD_STAT_VP bit and would make for an unexpected condition if IFCAP_VLAN_HWTAGGING were not set but the tag was stripped, which would be passed up the stack breaking isolation. PR: 260068 Approved by: vmaffione (cherry picked from commit b4a58b3d5831dd2e7e79d9d7cbc3e920803cb4f6) sys/dev/igc/igc_txrx.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Is there anything left to do for this PR, or can it be closed?
I think we are done here.