Page MenuHomeFreeBSD

ifnet: Add handling for toggling IFF_ALLMULTI in ifhwioctl()
ClosedPublic

Authored by markj on Sep 3 2024, 5:00 PM.
Tags
None
Referenced Files
F102722474: D46524.id142971.diff
Sat, Nov 16, 8:58 AM
F102720157: D46524.id142823.diff
Sat, Nov 16, 8:11 AM
F102716986: D46524.diff
Sat, Nov 16, 7:09 AM
Unknown Object (File)
Sun, Nov 3, 8:03 PM
Unknown Object (File)
Sun, Nov 3, 8:02 PM
Unknown Object (File)
Sun, Nov 3, 8:02 PM
Unknown Object (File)
Sun, Nov 3, 8:02 PM
Unknown Object (File)
Sun, Nov 3, 7:48 PM

Details

Summary

IFF_ALLMULTI has an associated activation counter and so needs special
treatment, like IFF_PROMISC.

Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Sep 3 2024, 5:00 PM
This revision is now accepted and ready to land.Sep 3 2024, 10:44 PM
zlei added inline comments.
sys/net/if.c
2616

I think the original comment is accurate.

ifconfig cxl0 promisc set permanently promiscuous mode which has the equivalent flag bit IFF_PPROMISC. That is different from IFF_PROMISC. Be aware the additional P which indicates permanently .

# ifconfig cxl0
cxl0: flags=1028943<UP,BROADCAST,RUNNING,PROMISC,SIMPLEX,MULTICAST,PPROMISC,LOWER_UP> metric 0 mtu 1500
	options=6ec07bb<RXCSUM,TXCSUM,VLAN_MTU,VLAN_HWTAGGING,JUMBO_MTU,VLAN_HWCSUM,TSO4,TSO6,LRO,VLAN_HWTSO,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6,HWSTATS,HWRXTSTMP,MEXTPG>
...
2630

I'm still evaluating this. Maybe we need a permanently allmulti mode ? There're in-kernel consumers call if_allmulti() which set IFF_ALLMULTI flag, and some drivers also set it when necessary.

sys/net/if.c
2630

One day we needed to enable allmulti in FreeBSD Guest machine, but were unable to. We use this patch https://people.freebsd.org/~ae/allmulti.diff internally.
From some Intel's doc why it may be needed:

Trusted VFs and VF Promiscuous Mode
-----------------------------------
This feature allows you to designate a particular VF as trusted and allows that
trusted VF to request selective promiscuous mode on the Physical Function (PF).

To set a VF as trusted or untrusted, enter the following command in the
Hypervisor:

# ip link set dev <ethX> vf 1 trust [on|off]

NOTE: It's important to set the VF to trusted before setting promiscuous mode.
If the VM is not trusted, the PF will ignore promiscuous mode requests from the
VF. If the VM becomes trusted after the VF driver is loaded, you must make a
new request to set the VF to promiscuous.

Once the VF is designated as trusted, use the following commands in the VM to
set the VF to promiscuous mode. For promiscuous all:

# ip link set <ethX> promisc on
    Where <ethX> is a VF interface in the VM

For promiscuous Multicast:

# ip link set <ethX> allmulticast on
    Where <ethX> is a VF interface in the VM

NOTE: By default, the ethtool private flag vf-true-promisc-support is set to
"off," meaning that promiscuous mode for the VF will be limited. To set the
promiscuous mode for the VF to true promiscuous and allow the VF to see all
ingress traffic, use the following command:

# ethtool --set-priv-flags <ethX> vf-true-promisc-support on

The vf-true-promisc-support private flag does not enable promiscuous mode;
rather, it designates which type of promiscuous mode (limited or true) you will
get when you enable promiscuous mode using the ip link commands above. Note
that this is a global setting that affects the entire device. However, the
vf-true-promisc-support private flag is only exposed to the first PF of the
device. The PF remains in limited promiscuous mode (unless it is in MFP mode)
regardless of the vf-true-promisc-support setting.
sys/net/if.c
2616

Thank you, I missed that. :(

2630

Hmm, do we really need IFF_PALLMULTI? There are some places in the network stack that specifically check for IFF_PPROMISC instead of IFF_PROMISC, but it's not clear that that's needed here.

From my reading, it's not really correct to modify IFF_CANTCHANGE as suggested, we need to avoid clearing the flag if it was set by if_allmulti().

Add a IFF_PALLMULTI flag, to match IFF_PROMISC.

This revision now requires review to proceed.Sep 5 2024, 2:22 PM
sys/net/if.c
2630

Hmm, do we really need IFF_PALLMULTI?

Probably yes. Think about ip_mroute.c or ip6_mroute.c set IFF_ALLMULTI flag on interface, then how do end user distinguish it from user requested ALLMULTI ? He / her may repeat ifconfig -allmulti but that will make no differences ( if_amcount != 0 ) then he / her would complain -allmulti does not work.

There are some places in the network stack that specifically check for IFF_PPROMISC instead of IFF_PROMISC, but it's not clear that that's needed here.

IFF_PPROMISC is user manually requested promisc mode ( to distinguish from requests from if_bridge / bpf etc. ) . It implies IFF_PROMISC , but not vice versa. Normally device driver does not need to have contract with it, i.e., drivers should respect only IFF_PROMISC, no matter whom requests that. Currently only axgbe and net80211 consumes IFF_PPROMISC.

For axgbe, the very first version 44b781cfe0b561909686778153915ec2b0ba5a21 consult IFF_PROMISC and removed in 9c6d6488faa5a0c98034068ad18597921589e3c3 then changed to use IFF_PPROMISC in 7113afc84c0b68f1e531dbd6d57d024d868d11c0 . I'm not sure whether that is intended or not but that seems unintentional. I do not have axgbe devices so I can not test that. I guess some functions such as bridge via if_bridge or do traffic sniffer via tcpdump would not work correctly for axgbe.

As for net80211, the usage of IFF_PPROMISC if more a HACK IMO. I guess it is better to move the checking to upper layer but I'm not sure as I am not familiar with net80211, really.

ether_demux() consults IFF_PPROMISC to pass promiscuously received frames to the upper layer if user requests. That is intentional.

From my reading, it's not really correct to modify IFF_CANTCHANGE as suggested, we need to avoid clearing the flag if it was set by if_allmulti().

No if we introduce IFF_PALLMULTI, if I read the code correctly.

Looks good to me.

This revision is now accepted and ready to land.Sep 5 2024, 4:20 PM
sys/net/if.c
2630

@ae

I'm still evaluating this. Maybe we need a permanently allmulti mode ?

I meant re-using IFF_ALLMULTI from userland when user configure the allmulti mode seems to be wrong. It has drawbacks so I said evaluating. English is not my native tone. Sorry for the misleading.