Bug 140647 - [em] [patch] e1000 driver does not correctly handle multicast promiscuous mode with 128 or more multicast addresses
Summary: [em] [patch] e1000 driver does not correctly handle multicast promiscuous mod...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 9.0-CURRENT
Hardware: Any Any
: Normal Affects Only Me
Assignee: Kevin Bowling
URL:
Keywords: IntelNetworking
Depends on:
Blocks:
 
Reported: 2009-11-17 22:30 UTC by Jonathan Looney
Modified: 2021-05-28 06:27 UTC (History)
5 users (show)

See Also:
kbowling: mfc-stable13+
kbowling: mfc-stable12+
kbowling: mfc-stable11-


Attachments
file.diff (1.57 KB, patch)
2009-11-17 22:30 UTC, Jonathan Looney
no flags Details | Diff
mtest-cmds-step1 (1.63 KB, application/octet-stream)
2009-11-17 22:36 UTC, Jonathan Looney
no flags Details
mtest-cmds-step2 (540 bytes, application/octet-stream)
2009-11-17 22:36 UTC, Jonathan Looney
no flags Details
mtest-cmds-step3 (540 bytes, application/octet-stream)
2009-11-17 22:36 UTC, Jonathan Looney
no flags Details
mtest-cmds-step4 (900 bytes, application/octet-stream)
2009-11-17 22:36 UTC, Jonathan Looney
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Looney 2009-11-17 22:30:02 UTC
The e1000 driver contains two related bugs in the way it handles multicast promiscuous mode for an interface when there are MAX_NUM_MULTICAST_ADDRESSES (currently, 128) or more multicast addresses in use on an interface.

Currently, when there are less than MAX_NUM_MULTICAST_ADDRESSES (currently, 128) multicast addresses in use on an interface, the code downloads the list to the card.  When there are MAX_NUM_MULTICAST_ADDRESSES or more multicast addresses in use on an interface, the code instead just sets the card to be in multicast promiscuous mode.  The current code (from em_set_multi())is here:

	if (mcnt >= MAX_NUM_MULTICAST_ADDRESSES) {
		reg_rctl = E1000_READ_REG(&adapter->hw, E1000_RCTL);
		reg_rctl |= E1000_RCTL_MPE;
		E1000_WRITE_REG(&adapter->hw, E1000_RCTL, reg_rctl);
	} else
		e1000_update_mc_addr_list(&adapter->hw, mta, mcnt);

When there are MAX_NUM_MULTICAST_ADDRESSES or more multicast addresses in use on an interface, the code does not update the multicast address list on the card.  Therefore, if multicast promiscuous mode becomes disabled, the card will not have a current and complete list of all multicast addresses.

There are two bugs related to the handling of multicast promiscuous mode when there are MAX_NUM_MULTICAST_ADDRESSES (currently, 128) or more multicast addresses in use on an interface:

1) When there are MAX_NUM_MULTICAST_ADDRESSES or more multicast addresses in use on an interface, the code enables multicast promiscuous mode.  However, when the number of multicast addresses in use on an interface drops below MAX_NUM_MULTICAST_ADDRESSES, the code does not disable multicast promiscuous mode.  This may result in the system unnecessarily receiving extra multicast packets to process.

2) When the number of multicast addresses reaches MAX_NUM_MULTICAST_ADDRESSES, the code enables multicast promiscuous mode.  However, when the IFF_ALLMULTI or IFF_PROMISC flag are removed on an interface, the driver will disable multicast promiscuous mode without regard to whether multicast promiscuous mode is still needed for multicast support.  This bug could be triggered, for example, if someone ran tcpdump in promiscuous mode (the defaut mode for tcpdump) and then stopped it while the number of multicast groups joined on an interface was greater than or equal to MAX_NUM_MULTICAST_ADDRESSES.  In this case, the card may not receive traffic for all joined multicast addresses.

I have verified that these bugs are present in the CVS HEAD; however, note that these bugs are present at least as far back as 6.3.

Fix: I have written a patch that addresses these bugs in these ways:

1) When the number of multicast addresses in use on an interface is below MAX_NUM_MULTICAST_ADDRESSES, the code checks the E1000_RCTL to see if multicast promiscuous mode is enabled.  If it is enabled, it checks to see if the IFF_ALLMULTI or IFF_PROMISC flags are set in the if_flags.  If neither is set, it disables multicast promiscuous mode.  In order to avoid brief outages, the code only disables multicast promiscuous mode after it has already written the new multicast addresses to the card.

2) When the IFF_ALLMULTI or IFF_PROMISC if_flags change, the code currently calls em_disable_promisc() and then em_set_promisc().  These two functions will reset the unicast promiscuous, multicast promiscuous, and save bad packet modes as appropriate for the if_flags.  However, in order to ensure that changes to these flags do not inadvertently disable multicast promiscuous mode when multicast promiscuous mode is needed due to the number of multicast addresses joined on an interface, I added a call to em_set_multi() after em_set_promisc().  This will allow em_set_multi() to re-enable multicast promiscuous mode, if needed.

I have compiled and tested this patch on CURRENT (CVS head, updated today).  I have also applied this same patch to 8.0-RC3 (CVS RELENG_8_0 tag, updated today) and compiled and tested on that release.  I also have patches that I have compiled and tested for older releases, were that to be necessary.

Patch attached with submission follows:
How-To-Repeat: In order to demonstrate these bugs, I have written four files for use with mtest:

mtest-cmds-step1: This joins 99 groups (225.0.0.x, where x is a number between 1 and 99, inclusive).
mtest-cmds-step2: This joins 30 groups (225.0.0.x, where x is a number between 100 and 129, inclusive).
mtest-cmds-step3: This leaves 30 groups (225.0.0.x, where x is a number between 100 and 129, inclusive).
mtest-cmds-step4: This joins 50 groups (225.0.0.x, where x is a number between 100 and 149, inclusive).

To use these files to demonstrate this bug, you need the following:
a) The device under test: a FreeBSD host with interface em0.  (You will probably need two windows open to this host.  You can use an interface other than em0 if you edit the mtest files and change the interface in the commands in these instructions.)

b) A second host to generate multicast traffic.  This host should be on the same network as the device under test's em0 interface.

Procedure to demonstrate these bugs:

1) On the host generating the traffic, ping 225.0.0.145.

2) On the device under test, start tcpdump in non-promiscuous mode (for example, 'tcpdump -i em0 -p icmp').  You should not see the multicast traffic yet.

3) On a different window on the device under test, start mtest.

4) Type 'f mtest-cmds-step1' to join the 99 groups.  Unless you already have joined 29 other groups on this interface, this should not cause the number of multicast addresses to reach MAX_NUM_MULTICAST_ADDRESSES.  Therefore, the tcpdump should not receive the traffic destined to 225.0.0.145 (as that was not one of the addresses joined).

5) Type 'f mtest-cmds-step2' to join 30 additional groups.  This should cause the number of multicast addresses to reach MAX_NUM_MULTICAST_ADDRESSES.  Therefore, the driver should enable multicast promiscuous mode for the interface and tcpdump should now begin to receive traffic destined to 225.0.0.145 (even though that was not one of the addresses that was joined).

6) Type 'f mtest-cmds-step3' to leave the 30 groups joined in the last step.  This should cause the number of multicast addresses to fall below MAX_NUM_MULTICAST_ADDRESSES.  That *should* mean that the card should leave multicast promiscuous mode; however, because of the first bug listed above, the card will remain in multicast promiscuous mode.  You can verify this by observing that tcpdump is still receiving traffic destined to 225.0.0.145 (even though that is not one of the addresses that is currently joined).

7) Type 'f mtest-cmds-step4' to join 50 additional groups.  This should cause the number of multicast addresses to reach MAX_NUM_MULTICAST_ADDRESSES.  Therefore, the driver should put the interface in multicast promiscuous mode and tcpdump should receive traffic destined to 225.0.0.145 (and, that is one of the addresses that was joined in this step).

8) Type 'p em0 1' to cause the interface to enter promiscuous mode. Then, type 'p em0 0' to cause the interface to leave promiscuous mode.  That *should not* prevent the card from receiving traffic destined for multicast addresses that are joined on the interface; however, because of the second bug listed above, the interface will leave multicast promiscuous mode and the interface will only receive traffic for the multicast addresses last programmed into the card.  You can verify this by observing that tcpdump stops receiving traffic destined to 225.0.0.145, even though that multicast address was joined in step 7 (and is still joined).
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2009-11-17 23:04:37 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-net

Over to maintainer(s).
Comment 2 Andre Oppermann freebsd_committer freebsd_triage 2010-08-23 18:44:29 UTC
Responsible Changed
From-To: freebsd-net->jfv

Over to maintainer.
Comment 3 lampa 2012-04-04 10:36:44 UTC
This bug is still not fixed in em/igb/lem driver in HEAD. And it looks 
that similar bug is in the ixgbe driver, ixgbe_set_multi() doesn't even
check count of multicast addresses and it will overflow static mta array 
causing kernel panic. It should enable IXGBE_FCTRL_MPE if mcnt > 128 like
em driver!

Sincerely,
Petr Lampa

-- 
Computer Centre                             E-mail: lampa@fit.vutbr.cz
Faculty of Information Technology           Web: http://www.fit.vutbr.cz/
Brno University of Technology               Fax:  +420 54114-1270
Bozetechova 2, 612 66 Brno, Czech Republic  Phone: +420 54114-1225
Comment 4 Sean Bruno freebsd_committer freebsd_triage 2015-06-30 17:16:35 UTC
head still does not have this code in it.  Should be reviewed for inclusion.
Comment 5 Sean Bruno freebsd_committer freebsd_triage 2015-07-23 16:58:38 UTC
Because this problem is being propagated to all Intel drivers, this really should be addressed.
Comment 6 Sean Bruno freebsd_committer freebsd_triage 2016-10-31 16:54:04 UTC
I'll take this for review/test.
Comment 7 Eitan Adler freebsd_committer freebsd_triage 2018-05-28 19:45:02 UTC
batch change:

For bugs that match the following
-  Status Is In progress 
AND
- Untouched since 2018-01-01.
AND
- Affects Base System OR Documentation

DO:

Reset to open status.


Note:
I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.
Comment 8 Kevin Bowling freebsd_committer freebsd_triage 2021-04-16 01:32:24 UTC
https://reviews.freebsd.org/D29789
Comment 9 commit-hook freebsd_committer freebsd_triage 2021-04-19 19:52:00 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=4b38eed76da9c36f09bff33b5cf15687cd99016f

commit 4b38eed76da9c36f09bff33b5cf15687cd99016f
Author:     Kevin Bowling <kbowling@FreeBSD.org>
AuthorDate: 2021-04-16 23:15:50 +0000
Commit:     Kevin Bowling <kbowling@FreeBSD.org>
CommitDate: 2021-04-19 19:49:55 +0000

    e1000: Correct promisc multicast filter handling

    There are a number of issues in the e1000 multicast filter handling
    that have been present for a long time. Take the updated approach from
    ixgbe(4) which does not have the issues.

    The issues are outlined in the PR, in particular this solves crossing
    over and under the hardware's filter limit, not programming the
    hardware filter when we are above its limit, disabling SBP (show bad
    packets) when the tunable is enabled and exiting promiscuous mode, and
    an off-by-one error in the em_copy_maddr function.

    PR:             140647
    Reported by:    jtl
    Reviewed by:    markj
    MFC after:      1 month
    Differential Revision:  https://reviews.freebsd.org/D29789

 sys/dev/e1000/if_em.c | 62 +++++++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 34 deletions(-)
Comment 10 commit-hook freebsd_committer freebsd_triage 2021-05-28 05:38:23 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=938b01476a3c0a3c75a0fd263238729b6a38360a

commit 938b01476a3c0a3c75a0fd263238729b6a38360a
Author:     Kevin Bowling <kbowling@FreeBSD.org>
AuthorDate: 2021-04-16 23:15:50 +0000
Commit:     Kevin Bowling <kbowling@FreeBSD.org>
CommitDate: 2021-05-28 05:30:03 +0000

    e1000: Correct promisc multicast filter handling

    There are a number of issues in the e1000 multicast filter handling
    that have been present for a long time. Take the updated approach from
    ixgbe(4) which does not have the issues.

    The issues are outlined in the PR, in particular this solves crossing
    over and under the hardware's filter limit, not programming the
    hardware filter when we are above its limit, disabling SBP (show bad
    packets) when the tunable is enabled and exiting promiscuous mode, and
    an off-by-one error in the em_copy_maddr function.

    PR:             140647
    Reported by:    jtl
    Reviewed by:    markj
    MFC after:      1 month
    Differential Revision:  https://reviews.freebsd.org/D29789

    (cherry picked from commit 4b38eed76da9c36f09bff33b5cf15687cd99016f)

 sys/dev/e1000/if_em.c | 62 +++++++++++++++++++++++----------------------------
 1 file changed, 28 insertions(+), 34 deletions(-)
Comment 11 commit-hook freebsd_committer freebsd_triage 2021-05-28 06:14:38 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ab177386aa8790c66826be227a1d9e6b482444bb

commit ab177386aa8790c66826be227a1d9e6b482444bb
Author:     Kevin Bowling <kbowling@FreeBSD.org>
AuthorDate: 2021-04-16 23:15:50 +0000
Commit:     Kevin Bowling <kbowling@FreeBSD.org>
CommitDate: 2021-05-28 06:10:48 +0000

    e1000: Correct promisc multicast filter handling

    There are a number of issues in the e1000 multicast filter handling
    that have been present for a long time. Take the updated approach from
    ixgbe(4) which does not have the issues.

    The issues are outlined in the PR, in particular this solves crossing
    over and under the hardware's filter limit, not programming the
    hardware filter when we are above its limit, disabling SBP (show bad
    packets) when the tunable is enabled and exiting promiscuous mode, and
    an off-by-one error in the em_copy_maddr function.

    PR:             140647
    Reported by:    jtl
    Reviewed by:    markj
    MFC after:      1 month
    Differential Revision:  https://reviews.freebsd.org/D29789

    (cherry picked from commit 4b38eed76da9c36f09bff33b5cf15687cd99016f)

 sys/dev/e1000/if_em.c | 54 +++++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)
Comment 12 Kevin Bowling freebsd_committer freebsd_triage 2021-05-28 06:27:37 UTC
No MFC to stable11 due to diverged driver but it shouldn't be hard if someone wants to.