Bug 260068 - e1000 & igb in netmap mode removes VLAN headers
Summary: e1000 & igb in netmap mode removes VLAN headers
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.2-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: Vincenzo Maffione
URL:
Keywords: IntelNetworking
Depends on:
Blocks:
 
Reported: 2021-11-26 18:19 UTC by Ozkan KIRIK
Modified: 2022-01-06 10:24 UTC (History)
3 users (show)

See Also:
kbowling: mfc-stable13?
kbowling: mfc-stable12?


Attachments
Patch to fix the issue (7.58 KB, patch)
2021-11-28 18:11 UTC, Vincenzo Maffione
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ozkan KIRIK 2021-11-26 18:19:56 UTC
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
Comment 1 Vincenzo Maffione freebsd_committer 2021-11-27 22:35:08 UTC
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.
Comment 2 Ozkan KIRIK 2021-11-28 06:56:07 UTC
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
Comment 3 Vincenzo Maffione freebsd_committer 2021-11-28 18:11:46 UTC
Created attachment 229767 [details]
Patch to fix the issue
Comment 4 Vincenzo Maffione freebsd_committer 2021-11-28 18:24:23 UTC
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.
Comment 5 Ozkan KIRIK 2021-11-28 19:10:42 UTC
Thank you so much Vincenzo. I'm going to test and report back.
Comment 6 Ozkan KIRIK 2021-11-28 19:33:23 UTC
Patch applied and worked properly on HEAD (main branch). Thank you again!
Comment 7 Ozkan KIRIK 2021-11-28 19:37:59 UTC
Patch was tested for em driver. I'll test the igb driver tomorrow.
Comment 8 Vincenzo Maffione freebsd_committer 2021-11-28 22:19:52 UTC
Thanks. FYI https://reviews.freebsd.org/D33154
Comment 9 Ozkan KIRIK 2021-11-29 10:11:20 UTC
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.
Comment 10 commit-hook freebsd_committer 2021-12-08 09:00:45 UTC
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(-)
Comment 11 Vincenzo Maffione freebsd_committer 2021-12-21 19:41:04 UTC
Hi,
  Would it be possible to test the latest change:
https://reviews.freebsd.org/D33156
?
This is meant to be applied against HEAD.

Thanks!
Comment 12 Ozkan KIRIK 2021-12-21 19:46:49 UTC
Hi Vincenzo,

I'll test the D33156 tomorrow. Thanks
Comment 13 commit-hook freebsd_committer 2021-12-22 09:04:07 UTC
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(-)
Comment 14 commit-hook freebsd_committer 2021-12-22 13:44:58 UTC
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(-)
Comment 15 commit-hook freebsd_committer 2021-12-28 10:56:43 UTC
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(+)
Comment 16 commit-hook freebsd_committer 2021-12-28 11:04:46 UTC
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(-)
Comment 17 commit-hook freebsd_committer 2021-12-28 11:12:48 UTC
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(-)
Comment 18 Vincenzo Maffione freebsd_committer 2021-12-28 11:14:33 UTC
All the required changes are in HEAD now.
Any testing would be really appreciated!
Thanks
Comment 19 commit-hook freebsd_committer 2021-12-29 16:45:21 UTC
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(-)
Comment 20 commit-hook freebsd_committer 2022-01-06 10:13:32 UTC
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(+)
Comment 21 commit-hook freebsd_committer 2022-01-06 10:24:36 UTC
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(+)