[PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Markus Blöchl]

The rx-vlan-filter feature flag prevents unexpected tagged frames on
the wire from reaching the kernel in promiscuous mode.
Disable this offloading feature in the lan7800 controller whenever
IFF_PROMISC is set and make sure that the hardware features
are updated when IFF_PROMISC changes.

Signed-off-by: Markus Blöchl <markus.bloechl@ipetronik.com>
---

Notes:
    When sniffing ethernet using a LAN7800 ethernet controller, vlan-tagged
    frames are silently dropped by the controller due to the
    RFE_CTL_VLAN_FILTER flag being set by default since commit
    4a27327b156e("net: lan78xx: Add support for VLAN filtering.").

    In order to receive those tagged frames it is necessary to manually disable
    rx vlan filtering using ethtool ( `ethtool -K ethX rx-vlan-filter off` or
    corresponding ioctls ). Setting all bits in the vlan filter table to 1 is
    an even worse approach, imho.

    As a user I would probably expect that setting IFF_PROMISC should be enough
    in all cases to receive all valid traffic.
    Therefore I think this behaviour is a bug in the driver, since other
    drivers (notably ixgbe) automatically disable rx-vlan-filter when
    IFF_PROMISC is set. Please correct me if I am wrong here.

 drivers/net/usb/lan78xx.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 65b315bc60ab..ac6c0beeac21 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2324,13 +2324,15 @@ static int lan78xx_set_mac_addr(struct net_device *netdev, void *p)
 }

 /* Enable or disable Rx checksum offload engine */
-static int lan78xx_set_features(struct net_device *netdev,
-                               netdev_features_t features)
+static void lan78xx_update_features(struct net_device *netdev,
+                                   netdev_features_t features)
 {
        struct lan78xx_net *dev = netdev_priv(netdev);
        struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
        unsigned long flags;
-       int ret;
+
+       if (netdev->flags & IFF_PROMISC)
+               features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;

        spin_lock_irqsave(&pdata->rfe_ctl_lock, flags);

@@ -2353,12 +2355,30 @@ static int lan78xx_set_features(struct net_device *netdev,
                pdata->rfe_ctl &= ~RFE_CTL_VLAN_FILTER_;

        spin_unlock_irqrestore(&pdata->rfe_ctl_lock, flags);
+}
+
+static int lan78xx_set_features(struct net_device *netdev,
+                               netdev_features_t features)
+{
+       struct lan78xx_net *dev = netdev_priv(netdev);
+       struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
+       int ret;
+
+       lan78xx_update_features(netdev, features);

        ret = lan78xx_write_reg(dev, RFE_CTL, pdata->rfe_ctl);

        return 0;
 }

+static void lan78xx_set_rx_mode(struct net_device *netdev)
+{
+       /* Enable or disable checksum offload engines */
+       lan78xx_update_features(netdev, netdev->features);
+
+       lan78xx_set_multicast(netdev);
+}
+
 static void lan78xx_deferred_vlan_write(struct work_struct *param)
 {
        struct lan78xx_priv *pdata =
@@ -2528,10 +2548,7 @@ static int lan78xx_reset(struct lan78xx_net *dev)
        pdata->rfe_ctl |= RFE_CTL_BCAST_EN_ | RFE_CTL_DA_PERFECT_;
        ret = lan78xx_write_reg(dev, RFE_CTL, pdata->rfe_ctl);

-       /* Enable or disable checksum offload engines */
-       lan78xx_set_features(dev->net, dev->net->features);
-
-       lan78xx_set_multicast(dev->net);
+       lan78xx_set_rx_mode(dev->net);

        /* reset PHY */
        ret = lan78xx_read_reg(dev, PMT_CTL, &buf);
@@ -3613,7 +3630,7 @@ static const struct net_device_ops lan78xx_netdev_ops = {
        .ndo_set_mac_address    = lan78xx_set_mac_addr,
        .ndo_validate_addr      = eth_validate_addr,
        .ndo_do_ioctl           = phy_do_ioctl_running,
-       .ndo_set_rx_mode        = lan78xx_set_multicast,
+       .ndo_set_rx_mode        = lan78xx_set_rx_mode,
        .ndo_set_features       = lan78xx_set_features,
        .ndo_vlan_rx_add_vid    = lan78xx_vlan_rx_add_vid,
        .ndo_vlan_rx_kill_vid   = lan78xx_vlan_rx_kill_vid,

base-commit: 4e0396c59559264442963b349ab71f66e471f84d
--
2.29.2


Impressum/Imprint: https://www.ipetronik.com/impressum

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Jakub Kicinski]

On Tue, 10 Nov 2020 16:39:58 +0100 Markus Blöchl wrote:
> The rx-vlan-filter feature flag prevents unexpected tagged frames on
> the wire from reaching the kernel in promiscuous mode.
> Disable this offloading feature in the lan7800 controller whenever
> IFF_PROMISC is set and make sure that the hardware features
> are updated when IFF_PROMISC changes.
> 
> Signed-off-by: Markus Blöchl <markus.bloechl@ipetronik.com>

Doesn't apply to neither net or net-next, please respin.

Could you also add a Fixes tag since this is a fix?

Thanks!

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Jakub Kicinski]

On Tue, 10 Nov 2020 16:39:58 +0100 Markus Blöchl wrote:
> The rx-vlan-filter feature flag prevents unexpected tagged frames on
> the wire from reaching the kernel in promiscuous mode.
> Disable this offloading feature in the lan7800 controller whenever
> IFF_PROMISC is set and make sure that the hardware features
> are updated when IFF_PROMISC changes.
> 
> Signed-off-by: Markus Blöchl <markus.bloechl@ipetronik.com>
> ---
> 
> Notes:
>     When sniffing ethernet using a LAN7800 ethernet controller, vlan-tagged
>     frames are silently dropped by the controller due to the
>     RFE_CTL_VLAN_FILTER flag being set by default since commit
>     4a27327b156e("net: lan78xx: Add support for VLAN filtering.").
> 
>     In order to receive those tagged frames it is necessary to manually disable
>     rx vlan filtering using ethtool ( `ethtool -K ethX rx-vlan-filter off` or
>     corresponding ioctls ). Setting all bits in the vlan filter table to 1 is
>     an even worse approach, imho.
> 
>     As a user I would probably expect that setting IFF_PROMISC should be enough
>     in all cases to receive all valid traffic.
>     Therefore I think this behaviour is a bug in the driver, since other
>     drivers (notably ixgbe) automatically disable rx-vlan-filter when
>     IFF_PROMISC is set. Please correct me if I am wrong here.

I've been mulling over this, I'm not 100% sure that disabling VLAN
filters on promisc is indeed the right thing to do. The ixgbe doing
this is somewhat convincing. OTOH users would not expect flow filters
to get disabled when promisc is on, so why disable vlan filters?

Either way we should either document this as an expected behavior or
make the core clear the features automatically rather than force
drivers to worry about it.

Does anyone else have an opinion, please?

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 65b315bc60ab..ac6c0beeac21 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2324,13 +2324,15 @@ static int lan78xx_set_mac_addr(struct net_device *netdev, void *p)
>  }
> 
>  /* Enable or disable Rx checksum offload engine */
> -static int lan78xx_set_features(struct net_device *netdev,
> -                               netdev_features_t features)
> +static void lan78xx_update_features(struct net_device *netdev,
> +                                   netdev_features_t features)
>  {
>         struct lan78xx_net *dev = netdev_priv(netdev);
>         struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
>         unsigned long flags;
> -       int ret;
> +
> +       if (netdev->flags & IFF_PROMISC)
> +               features &= ~NETIF_F_HW_VLAN_CTAG_FILTER;
> 
>         spin_lock_irqsave(&pdata->rfe_ctl_lock, flags);
> 
> @@ -2353,12 +2355,30 @@ static int lan78xx_set_features(struct net_device *netdev,
>                 pdata->rfe_ctl &= ~RFE_CTL_VLAN_FILTER_;
> 
>         spin_unlock_irqrestore(&pdata->rfe_ctl_lock, flags);
> +}
> +
> +static int lan78xx_set_features(struct net_device *netdev,
> +                               netdev_features_t features)
> +{
> +       struct lan78xx_net *dev = netdev_priv(netdev);
> +       struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
> +       int ret;
> +
> +       lan78xx_update_features(netdev, features);
> 
>         ret = lan78xx_write_reg(dev, RFE_CTL, pdata->rfe_ctl);
> 
>         return 0;
>  }
> 
> +static void lan78xx_set_rx_mode(struct net_device *netdev)
> +{
> +       /* Enable or disable checksum offload engines */
> +       lan78xx_update_features(netdev, netdev->features);
> +
> +       lan78xx_set_multicast(netdev);
> +}
> +
>  static void lan78xx_deferred_vlan_write(struct work_struct *param)
>  {
>         struct lan78xx_priv *pdata =
> @@ -2528,10 +2548,7 @@ static int lan78xx_reset(struct lan78xx_net *dev)
>         pdata->rfe_ctl |= RFE_CTL_BCAST_EN_ | RFE_CTL_DA_PERFECT_;
>         ret = lan78xx_write_reg(dev, RFE_CTL, pdata->rfe_ctl);
> 
> -       /* Enable or disable checksum offload engines */
> -       lan78xx_set_features(dev->net, dev->net->features);
> -
> -       lan78xx_set_multicast(dev->net);
> +       lan78xx_set_rx_mode(dev->net);
> 
>         /* reset PHY */
>         ret = lan78xx_read_reg(dev, PMT_CTL, &buf);
> @@ -3613,7 +3630,7 @@ static const struct net_device_ops lan78xx_netdev_ops = {
>         .ndo_set_mac_address    = lan78xx_set_mac_addr,
>         .ndo_validate_addr      = eth_validate_addr,
>         .ndo_do_ioctl           = phy_do_ioctl_running,
> -       .ndo_set_rx_mode        = lan78xx_set_multicast,
> +       .ndo_set_rx_mode        = lan78xx_set_rx_mode,
>         .ndo_set_features       = lan78xx_set_features,
>         .ndo_vlan_rx_add_vid    = lan78xx_vlan_rx_add_vid,
>         .ndo_vlan_rx_kill_vid   = lan78xx_vlan_rx_kill_vid,
> 
> base-commit: 4e0396c59559264442963b349ab71f66e471f84d
> --
> 2.29.2
> 
> 
> Impressum/Imprint: https://www.ipetronik.com/impressum

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Florian Fainelli]

On 11/11/2020 7:43 AM, Jakub Kicinski wrote:
> On Tue, 10 Nov 2020 16:39:58 +0100 Markus Blöchl wrote:
>> The rx-vlan-filter feature flag prevents unexpected tagged frames on
>> the wire from reaching the kernel in promiscuous mode.
>> Disable this offloading feature in the lan7800 controller whenever
>> IFF_PROMISC is set and make sure that the hardware features
>> are updated when IFF_PROMISC changes.
>>
>> Signed-off-by: Markus Blöchl <markus.bloechl@ipetronik.com>
>> ---
>>
>> Notes:
>>     When sniffing ethernet using a LAN7800 ethernet controller, vlan-tagged
>>     frames are silently dropped by the controller due to the
>>     RFE_CTL_VLAN_FILTER flag being set by default since commit
>>     4a27327b156e("net: lan78xx: Add support for VLAN filtering.").
>>
>>     In order to receive those tagged frames it is necessary to manually disable
>>     rx vlan filtering using ethtool ( `ethtool -K ethX rx-vlan-filter off` or
>>     corresponding ioctls ). Setting all bits in the vlan filter table to 1 is
>>     an even worse approach, imho.
>>
>>     As a user I would probably expect that setting IFF_PROMISC should be enough
>>     in all cases to receive all valid traffic.
>>     Therefore I think this behaviour is a bug in the driver, since other
>>     drivers (notably ixgbe) automatically disable rx-vlan-filter when
>>     IFF_PROMISC is set. Please correct me if I am wrong here.
> 
> I've been mulling over this, I'm not 100% sure that disabling VLAN
> filters on promisc is indeed the right thing to do. The ixgbe doing
> this is somewhat convincing. OTOH users would not expect flow filters
> to get disabled when promisc is on, so why disable vlan filters?
> 
> Either way we should either document this as an expected behavior or
> make the core clear the features automatically rather than force
> drivers to worry about it.
> 
> Does anyone else have an opinion, please?

The semantics of promiscuous are pretty clear though, and if you have a
NIC with VLAN filtering capability which could prevent the stack from
seeing *all* packets, that would be considered a bug. I suppose that you
could not disable VLAN filtering but instead install all 4096 - N VLANs
(N being currently used) into the filter to guarantee receiving those
VLAN tagged frames?

As far as flow filters, this is actually a good question, it sounds like
there are some possibly interesting problems to solve there. For
instance with an Ethernet switch, if you had a rule that diverted
packets to be switched directly to a particular port, what should happen
when either of these ports is in promiscuous mode? Should the switch be
instructed to replace all of the rules to forward + copy to the CPU?
-- 
Florian

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Vladimir Oltean]

On Wed, Nov 11, 2020 at 07:56:58AM -0800, Florian Fainelli wrote:
> The semantics of promiscuous are pretty clear though, and if you have a
> NIC with VLAN filtering capability which could prevent the stack from
> seeing *all* packets, that would be considered a bug. I suppose that you
> could not disable VLAN filtering but instead install all 4096 - N VLANs
> (N being currently used) into the filter to guarantee receiving those
> VLAN tagged frames?

Are they?

IEEE 802.3 clause 30.3.1.1.16 aPromiscuousStatus says:

APPROPRIATE SYNTAX:
BOOLEAN

BEHAVIOUR DEFINED AS:
A GET operation returns the value “true” for promiscuous mode enabled, and “false” otherwise.

Frames without errors received solely because this attribute has the value “true” are counted as
frames received correctly; frames received in this mode that do contain errors update the
appropriate error counters.

A SET operation to the value “true” provides a means to cause the LayerMgmtRecognizeAddress
function to accept frames regardless of their destination address.

A SET operation to the value “false” causes the MAC sublayer to return to the normal operation
of carrying out address recognition procedures for station, broadcast, and multicast group
addresses (LayerMgmtRecognizeAddress function).;


As for IEEE 802.1Q, there's nothing about promiscuity in the context of
VLAN there.

Sadly, I think promiscuity refers only to address recognition for the
purpose of packet termination. I cannot find any reference to VLAN in
the context of promiscuity, or, for that matter, I cannot find any
reference coming from a standards body that promiscuity would mean
"accept all packets".

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Vladimir Oltean]

On Wed, Nov 11, 2020 at 06:47:27PM +0200, Vladimir Oltean wrote:
> On Wed, Nov 11, 2020 at 07:56:58AM -0800, Florian Fainelli wrote:
> > The semantics of promiscuous are pretty clear though, and if you have a
> > NIC with VLAN filtering capability which could prevent the stack from
> > seeing *all* packets, that would be considered a bug. I suppose that you
> > could not disable VLAN filtering but instead install all 4096 - N VLANs
> > (N being currently used) into the filter to guarantee receiving those
> > VLAN tagged frames?
> 
> Are they?
> 
> IEEE 802.3 clause 30.3.1.1.16 aPromiscuousStatus says:
> 
> APPROPRIATE SYNTAX:
> BOOLEAN
> 
> BEHAVIOUR DEFINED AS:
> A GET operation returns the value “true” for promiscuous mode enabled, and “false” otherwise.
> 
> Frames without errors received solely because this attribute has the value “true” are counted as
> frames received correctly; frames received in this mode that do contain errors update the
> appropriate error counters.
> 
> A SET operation to the value “true” provides a means to cause the LayerMgmtRecognizeAddress
> function to accept frames regardless of their destination address.
> 
> A SET operation to the value “false” causes the MAC sublayer to return to the normal operation
> of carrying out address recognition procedures for station, broadcast, and multicast group
> addresses (LayerMgmtRecognizeAddress function).;
> 
> 
> As for IEEE 802.1Q, there's nothing about promiscuity in the context of
> VLAN there.
> 
> Sadly, I think promiscuity refers only to address recognition for the
> purpose of packet termination. I cannot find any reference to VLAN in
> the context of promiscuity, or, for that matter, I cannot find any
> reference coming from a standards body that promiscuity would mean
> "accept all packets".

I realize I did not tell you what the LayerMgmtRecognizeAddress function
does.

function LayerMgmtRecognizeAddress(address: AddressValue): Boolean;
begin
	if {promiscuous receive enabled} then LayerMgmtRecognizeAddress := true;
	if address = ... {MAC station address} then LayerMgmtRecognizeAddress := true;
	if address = ... {Broadcast address} then LayerMgmtRecognizeAddress := true;
	if address = ... {One of the addresses on the multicast list and multicast reception is enabled} then LayerMgmtRecognizeAddress := true;
	LayerMgmtRecognizeAddress := false
end; {LayerMgmtRecognizeAddress}

Markus complained about the tcpdump program in particular. Well, tcpdump
is a complex beast, and far too often, people seem to conflate tcpdump
with promiscuity, even though:
- promiscuity is not what enables tcpdump to see "all packets" being
  sent/received by the network stack on that interface, but ETH_P_ALL
  sockets are what do the magic there
- tcpdump also has a --no-promiscuous-mode option.

I would expect that tcpdump could gain a feature to disable (even if
temporarily) the rx-vlan-filter offload, through an ethtool netlink
message. Then users could get what they expect.

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Markus Blöchl]

On Wed, Nov 11, 2020 at 06:47:27PM +0200, Vladimir Oltean wrote:
> On Wed, Nov 11, 2020 at 07:56:58AM -0800, Florian Fainelli wrote:
> > The semantics of promiscuous are pretty clear though, and if you have a
> > NIC with VLAN filtering capability which could prevent the stack from
> > seeing *all* packets, that would be considered a bug. I suppose that you
> > could not disable VLAN filtering but instead install all 4096 - N VLANs
> > (N being currently used) into the filter to guarantee receiving those
> > VLAN tagged frames?

Adding all VLAN ids to the filter is certainly a possibility, I just don't
see why that redundant extra lookup per frame should be done in the NIC.

I guess, we could also put that feature on WISH while promisc ist active.
That, at least, makes it clear what happened.

> 
> Are they?
> 
> IEEE 802.3 clause 30.3.1.1.16 aPromiscuousStatus says:
> 
> APPROPRIATE SYNTAX:
> BOOLEAN
> 
> BEHAVIOUR DEFINED AS:
> A GET operation returns the value “true” for promiscuous mode enabled, and “false” otherwise.
> 
> Frames without errors received solely because this attribute has the value “true” are counted as
> frames received correctly; frames received in this mode that do contain errors update the
> appropriate error counters.
> 
> A SET operation to the value “true” provides a means to cause the LayerMgmtRecognizeAddress
> function to accept frames regardless of their destination address.
> 
> A SET operation to the value “false” causes the MAC sublayer to return to the normal operation
> of carrying out address recognition procedures for station, broadcast, and multicast group
> addresses (LayerMgmtRecognizeAddress function).;
> 
> 
> As for IEEE 802.1Q, there's nothing about promiscuity in the context of
> VLAN there.
> 
> Sadly, I think promiscuity refers only to address recognition for the
> purpose of packet termination. I cannot find any reference to VLAN in
> the context of promiscuity, or, for that matter, I cannot find any
> reference coming from a standards body that promiscuity would mean
> "accept all packets".

From what I can see, most other drivers use a special hardware register
flag to enable promiscuous mode, which overrules all other filters.
e.g. from the ASIX AX88178 datasheet:

  PRO:  PACKET_TYPE_PROMISCUOUS.
    1: All frames received by the ASIC are forwarded up toward the host.
    0: Disabled (default).

It is just so that the lan78xx controllers don't have this explicit flag.


But since my change is more controversial than I anticipated, I would like
to take a step back and ask some general questions first:

We used to connect something akin to a port mirror to a lan7800 NIC
(and a few others) in order to record and debug some VLAN heavy traffic.
On older kernel versions putting the interface into promiscuous mode
and opening and binding an AF_PACKET socket (or just throwing tcpdump
or libpcap at it) was sufficient.
After a kernel upgrade the same setup missed most of the traffic.
Does this qualify as a regression? Why not?

Should there be a documented and future proof way to receive *all*
valid ethernet frames from an interface? This could be something like:

a) - Bring up the interface
   - Put the interface into promiscuous mode
   - Open, bind and read a raw AF_PACKET socket with ETH_P_ALL
   - Patch up the 801.1Q headers if required.

b) - The same as a)
   - Additionally enumerate and disable all available offloading features

c) - Use libpcap / Do whatever libpcap does (like with TPACKET)
   In this case you need to help me convince the tcpdump folks that this
   is a bug on their side... ;-)

d) - Read the controller datasheet
   - Read the kernel documentation
   - Read your kernels and drivers sources
   - Do whatever might be necessary

e) - No, there is no guaranteed way to to this

Any opinions on these questions?
After those are answered, I am open to suggestions on how to fix this
differently (if still needed).
I'd rather not get involved into discussions on flow filters as I am
absolutely clueless in this regard.


PS: Sorry for sending from my companies mail server. It does some nasty
transformations on the body to outgoing mails I just complained about.
If this isn't resolved soon, I might follow up using another
address which should preserve patches.

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 11:53:15AM +0100, Markus Blöchl wrote:
> From what I can see, most other drivers use a special hardware register
> flag to enable promiscuous mode, which overrules all other filters.

Yes, but it may not mean what you think.

> e.g. from the ASIX AX88178 datasheet:
>
>   PRO:  PACKET_TYPE_PROMISCUOUS.
>     1: All frames received by the ASIC are forwarded up toward the host.
>     0: Disabled (default).

See, that's one definition of promiscuity that is really strange, and
not backed by any standards body that I know of (if you know otherwise,
please speak up).

> It is just so that the lan78xx controllers don't have this explicit flag.

Which is not surprising, at least under that description. Others may be
inclined to call that behavior "packet trapping" when applying it to
e.g. an Ethernet switch.

There might be an entire discussion about how promiscuity does _not_
mean "deliver all packets to the CPU" that might be of interest to you:
https://lkml.org/lkml/2019/8/29/255

> But since my change is more controversial than I anticipated, I would like
> to take a step back and ask some general questions first:
>
> We used to connect something akin to a port mirror to a lan7800 NIC
> (and a few others) in order to record and debug some VLAN heavy traffic.
> On older kernel versions putting the interface into promiscuous mode
> and opening and binding an AF_PACKET socket (or just throwing tcpdump
> or libpcap at it) was sufficient.
> After a kernel upgrade the same setup missed most of the traffic.
> Does this qualify as a regression? Why not?

If something used to work but no longer does, that's what a regression
is. But fixing it depends on whether it should have worked like that in
the first place or not. That we don't know.

> Should there be a documented and future proof way to receive *all*
> valid ethernet frames from an interface?

Yes, there should.

> This could be something like:
>
> a) - Bring up the interface
>    - Put the interface into promiscuous mode

This one would be necessary in order to not drop packets with unknown
addresses, not more.

>    - Open, bind and read a raw AF_PACKET socket with ETH_P_ALL
>    - Patch up the 801.1Q headers if required.

What do you mean by "patching up"? Are you talking about the fact that
packets are untagged by the stack in the receive path anyway, and the
VLAN header would need to be reconstructed?
https://patchwork.ozlabs.org/project/netdev/patch/e06dbb47-2d1c-03ca-4cd7-cc958b6a939e@gmail.com/

>
> b) - The same as a)
>    - Additionally enumerate and disable all available offloading features

If said offloading features have to do with the CPU not receiving some
frames any longer, and you _do_ want the CPU to see them, then obviously
said offloading features should be disabled. This includes not only VLAN
filtering, but also bridging offload, IP forwarding offload, etc.

I'd say that (b) should be sufficient, but not future-proof in the sense
that new offloading features might come every day, and they would need
to be disabled on a case-by-case basis.

For the forwarding offload, there would be no question whatsoever that
you'd need to turn it off, or add a mirroring rule towards the CPU, or
do _something_ user-visible, to get that traffic. But as for VLAN
filtering offload in particular, there's the (not negligible at all)
precedent created by the bridge driver, that Ido pointed out. That would
be a decision for the maintainers to make, if the Linux network stack
creates its own definition of promiscuity which openly contradicts IEEE's.
One might perhaps try to argue that the VLAN ID is an integral part of
the station's address (which is true if you look at it from the
perspective of an IEEE 802.1Q bridge), and therefore promiscuity should
apply on the VLAN ID too, not just the MAC address. Either way, that
should be made more clear than it currently is, once a decision is
taken.

> c) - Use libpcap / Do whatever libpcap does (like with TPACKET)
>    In this case you need to help me convince the tcpdump folks that this
>    is a bug on their side... ;-)

Well, that assumes that today, tcpdump can always capture all traffic on
all types of interfaces, something which is already false (see bridging
offload / IP forwarding offload). There, it was even argued that you'd
better be 100% sure that you want to see all received traffic, as the
interfaces can be very high-speed, and not even a mirroring rule might
guarantee reception of 100% of the traffic. That's why things like sFlow
/ tc-sample exist.

> d) - Read the controller datasheet
>    - Read the kernel documentation
>    - Read your kernels and drivers sources
>    - Do whatever might be necessary

Yes, in general reading is helpful, but I'm not quite sure where you're
going with this?

> e) - No, there is no guaranteed way to to this

No, there should be a way to achieve that through some sort of
configuration.

> Any opinions on these questions?

My 2 cents have just been shared.

> After those are answered, I am open to suggestions on how to fix this
> differently (if still needed).

Turn off VLAN filtering, or get a commonly accepted definition of
promiscuity?

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Markus Blöchl]

On Sat, Nov 14, 2020 at 08:11:03PM +0200, Vladimir Oltean wrote:
> On Thu, Nov 12, 2020 at 11:53:15AM +0100, Markus Blöchl wrote:
> > From what I can see, most other drivers use a special hardware register
> > flag to enable promiscuous mode, which overrules all other filters.
> 
> Yes, but it may not mean what you think.
> 
> > e.g. from the ASIX AX88178 datasheet:
> >
> >   PRO:  PACKET_TYPE_PROMISCUOUS.
> >     1: All frames received by the ASIC are forwarded up toward the host.
> >     0: Disabled (default).
> 
> See, that's one definition of promiscuity that is really strange, and
> not backed by any standards body that I know of (if you know otherwise,
> please speak up).
> 
> > It is just so that the lan78xx controllers don't have this explicit flag.
> 
> Which is not surprising, at least under that description. Others may be
> inclined to call that behavior "packet trapping" when applying it to
> e.g. an Ethernet switch.
> 
> There might be an entire discussion about how promiscuity does _not_
> mean "deliver all packets to the CPU" that might be of interest to you:
> https://lkml.org/lkml/2019/8/29/255

If I glanced over this discussion correctly, it is about avoiding
promiscuity under certain circumstances (while promiscuity was
only requested for switching and not by userspace) because HW promiscuity
is not needed in that particular case.

As I currently see it, there are two common use cases for promiscuity:

1) Applying filtering, switching and other logic on the CPU.
   This could be due to limited resources in the NIC, e.g. when there
   are too many unicast addresses configured on that interface or
   simply an unavailable hardware capability.

2) Sniffing the wire.

The kernel uses IFF_PROMISC (or `__dev_set_promiscuity()`) for both,
which obviously can be overkill in the first case.
The question remains, what does IFF_PROMISC exactly mean for userspace
(which, I guess, most often uses it for 2)?

> 
> > But since my change is more controversial than I anticipated, I would like
> > to take a step back and ask some general questions first:
> >
> > We used to connect something akin to a port mirror to a lan7800 NIC
> > (and a few others) in order to record and debug some VLAN heavy traffic.
> > On older kernel versions putting the interface into promiscuous mode
> > and opening and binding an AF_PACKET socket (or just throwing tcpdump
> > or libpcap at it) was sufficient.
> > After a kernel upgrade the same setup missed most of the traffic.
> > Does this qualify as a regression? Why not?
> 
> If something used to work but no longer does, that's what a regression
> is. But fixing it depends on whether it should have worked like that in
> the first place or not. That we don't know.

Admittedly, I certainly don't know, but hoped to find whom who does on
this list. ;-)

> 
> > Should there be a documented and future proof way to receive *all*
> > valid ethernet frames from an interface?
> 
> Yes, there should.
> 
> > This could be something like:
> >
> > a) - Bring up the interface
> >    - Put the interface into promiscuous mode
> 
> This one would be necessary in order to not drop packets with unknown
> addresses, not more.
> 
> >    - Open, bind and read a raw AF_PACKET socket with ETH_P_ALL
> >    - Patch up the 801.1Q headers if required.
> 
> What do you mean by "patching up"? Are you talking about the fact that
> packets are untagged by the stack in the receive path anyway, and the
> VLAN header would need to be reconstructed?
> https://patchwork.ozlabs.org/project/netdev/patch/e06dbb47-2d1c-03ca-4cd7-cc958b6a939e@gmail.com/

Yes, that's exactly what I was referring to.
I find it slightly annoying on RAW sockets, but it's documented and
(hopefully) consistent behaviour now, so okay for me.

> 
> >
> > b) - The same as a)
> >    - Additionally enumerate and disable all available offloading features
> 
> If said offloading features have to do with the CPU not receiving some
> frames any longer, and you _do_ want the CPU to see them, then obviously
> said offloading features should be disabled. This includes not only VLAN
> filtering, but also bridging offload, IP forwarding offload, etc.
> 
> I'd say that (b) should be sufficient, but not future-proof in the sense
> that new offloading features might come every day, and they would need
> to be disabled on a case-by-case basis.
> 
> For the forwarding offload, there would be no question whatsoever that
> you'd need to turn it off, or add a mirroring rule towards the CPU, or
> do _something_ user-visible, to get that traffic. But as for VLAN
> filtering offload in particular, there's the (not negligible at all)
> precedent created by the bridge driver, that Ido pointed out. That would
> be a decision for the maintainers to make, if the Linux network stack
> creates its own definition of promiscuity which openly contradicts IEEE's.
> One might perhaps try to argue that the VLAN ID is an integral part of
> the station's address (which is true if you look at it from the
> perspective of an IEEE 802.1Q bridge), and therefore promiscuity should
> apply on the VLAN ID too, not just the MAC address. Either way, that
> should be made more clear than it currently is, once a decision is
> taken.

In that case, maybe new features which could alter user-visible behaviour
should not be enabled by default?
If I am not mistaken, bridging offload, IP forwarding offload and
similar have to be enabled explicitly, at least.

I am not convinced that this definition would indeed contradict the
IEEE standard. It might just be a stronger one with more guarantees.
Assuming you have a very stupid NIC without any filtering or offloading
capabilities, which would always forward all frames to the CPU.
Would this NIC not comply to the standard?

@Jakub
May I take your answer as a final decision or would you prefer some more
input on this?

> 
> > c) - Use libpcap / Do whatever libpcap does (like with TPACKET)
> >    In this case you need to help me convince the tcpdump folks that this
> >    is a bug on their side... ;-)
> 
> Well, that assumes that today, tcpdump can always capture all traffic on
> all types of interfaces, something which is already false (see bridging
> offload / IP forwarding offload). There, it was even argued that you'd
> better be 100% sure that you want to see all received traffic, as the
> interfaces can be very high-speed, and not even a mirroring rule might
> guarantee reception of 100% of the traffic. That's why things like sFlow
> / tc-sample exist.
> 
> > d) - Read the controller datasheet
> >    - Read the kernel documentation
> >    - Read your kernels and drivers sources
> >    - Do whatever might be necessary
> 
> Yes, in general reading is helpful, but I'm not quite sure where you're
> going with this?

Well, that just meant, that there should always be a way, but no
universal one.
Which one depends on your exact hardware setup or maybe even the current
constellation of stars...

> 
> > e) - No, there is no guaranteed way to to this
> 
> No, there should be a way to achieve that through some sort of
> configuration.
> 
> > Any opinions on these questions?
> 
> My 2 cents have just been shared.
> 
> > After those are answered, I am open to suggestions on how to fix this
> > differently (if still needed).
> 
> Turn off VLAN filtering, or get a commonly accepted definition of
> promiscuity?


Other Drivers
=============

So I tried to figure out what other existing hardware drivers do
by grepping for drivers which do something on a change to
NETIF_F_HW_VLAN_CTAG_FILTER.

Here are the results of me trying to understand the drivers quickly.
I hope it's somewhat close and helps:

1) aqc111
   This controller has a HW register flag for promiscuous mode.
   I am not sure what it does, but since this is another ASIX
   device, the documentation from above should apply.

2) lan78xx
   Here it began ...

3) ixgbe
   This driver disables HW_VLAN_CTAG_FILTER if IFF_PROMISC is set.

4) ice
   I don't know.

5) ocelot_net
   This driver does not support promiscuous mode with the following
   explanation:
	/* This doesn't handle promiscuous mode because the bridge core is
	 * setting IFF_PROMISC on all slave interfaces and all frames would be
	 * forwarded to the CPU port.
	 */
   See the already mentioned discussion https://lkml.org/lkml/2019/8/29/255
   for more.

6) liquidio
   This driver also has a HW flag for promiscuous mode.
   I don't know what it does, though.

7) mvpp2
   This driver disables vid filtering if IFF_PROMISC is set.

8) efx
	/* Disable VLAN filtering by default.  It may be enforced if
	 * the feature is fixed (i.e. VLAN filters are required to
	 * receive VLAN tagged packets due to vPort restrictions).
	 */
   I did not find a variant that really honors this feature.

9) ef100
   I don't know.

10) mlx5
   This driver sets a HW promisc flag, adds an `ANY` vlan filter rule
   and ignores further changes to vlan filtering if IFF_PROMISC is set.

11) nfp
   This driver uses a HW promisc flag.

12) atlantic
   I don't know.

13) xlgmac
   This one has an interesting comment in `xlgmac_set_promiscuous_mode`:
	/* Hardware will still perform VLAN filtering in promiscuous mode */
	if (enable) {
		xlgmac_disable_rx_vlan_filtering(pdata);
	} else {
		if (pdata->netdev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
			xlgmac_enable_rx_vlan_filtering(pdata);
	}

14) enetc
   I think, the feature is overridden everytime in `set_rx_mode` as long
   as IFF_PROMISC is set.

15) xgbe
   In `xgbe_set_promiscuous_mode` once again:
	/* Hardware will still perform VLAN filtering in promiscuous mode */
	if (enable) {
		xgbe_disable_rx_vlan_filtering(pdata);
	} else {
		if (pdata->netdev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
			xgbe_enable_rx_vlan_filtering(pdata);
	}


Implementation
==============

I then tried to come up with a solution in net/core that would
universally disable vlan filtering in promiscuous mode.
Removing the features in `netdev_fix_features` is easily done, but
updating the active set of features whenever IFF_PROMISC changes
seems hard.

`__dev_set_promiscuity` is often called in atomic context but
`.ndo_set_features` can sleep in many drivers.

Adding a work_queue or similar to every net_device seems clumsy and
inappropriate.

Rewriting ndo_set_features of all drivers to be atomic is not a task
you should ask from me...

Calling `netdev_update_features` after every entrypoint that locks
the rtnl seems too error-prone and also clumsy.

Only updating the features when promiscuity was explicitly requested
by userspace in `dev_change_flags` might leave the device in a
weird inconsistent state.

Continue to let each driver enforce the kernels definition of
promiscuity. They know how to update the features atomically.
Then I am back at my original patch...

I'm afraid, I might need some guidance on how to approach this.


Thanks for your help and all of your responses

Markus

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Vladimir Oltean]

On Thu, Nov 19, 2020 at 04:37:51PM +0100, Markus Blöchl wrote:
> > There might be an entire discussion about how promiscuity does _not_
> > mean "deliver all packets to the CPU" that might be of interest to you:
> > https://lkml.org/lkml/2019/8/29/255
>
> If I glanced over this discussion correctly, it is about avoiding
> promiscuity under certain circumstances (while promiscuity was
> only requested for switching and not by userspace) because HW promiscuity
> is not needed in that particular case.
>
> As I currently see it, there are two common use cases for promiscuity:
>
> 1) Applying filtering, switching and other logic on the CPU.
>    This could be due to limited resources in the NIC, e.g. when there
>    are too many unicast addresses configured on that interface or
>    simply an unavailable hardware capability.
>
> 2) Sniffing the wire.
>
> The kernel uses IFF_PROMISC (or `__dev_set_promiscuity()`) for both,
> which obviously can be overkill in the first case.
> The question remains, what does IFF_PROMISC exactly mean for userspace
> (which, I guess, most often uses it for 2)?

And that discussion ended with the conclusion that promiscuity is never
enough to "sniff the wire".

> > > b) - The same as a)
> > >    - Additionally enumerate and disable all available offloading features
> >
> > If said offloading features have to do with the CPU not receiving some
> > frames any longer, and you _do_ want the CPU to see them, then obviously
> > said offloading features should be disabled. This includes not only VLAN
> > filtering, but also bridging offload, IP forwarding offload, etc.
> >
> > I'd say that (b) should be sufficient, but not future-proof in the sense
> > that new offloading features might come every day, and they would need
> > to be disabled on a case-by-case basis.
> >
> > For the forwarding offload, there would be no question whatsoever that
> > you'd need to turn it off, or add a mirroring rule towards the CPU, or
> > do _something_ user-visible, to get that traffic. But as for VLAN
> > filtering offload in particular, there's the (not negligible at all)
> > precedent created by the bridge driver, that Ido pointed out. That would
> > be a decision for the maintainers to make, if the Linux network stack
> > creates its own definition of promiscuity which openly contradicts IEEE's.
> > One might perhaps try to argue that the VLAN ID is an integral part of
> > the station's address (which is true if you look at it from the
> > perspective of an IEEE 802.1Q bridge), and therefore promiscuity should
> > apply on the VLAN ID too, not just the MAC address. Either way, that
> > should be made more clear than it currently is, once a decision is
> > taken.
>
> In that case, maybe new features which could alter user-visible behaviour
> should not be enabled by default?
> If I am not mistaken, bridging offload, IP forwarding offload and
> similar have to be enabled explicitly, at least.

Uhm, no? If L2 and L3 forwarding offload are supported by the hardware,
the way things work is that said driver simply monitors the user commands
and configures itself to seamlessly perform those operations in hardware.
There isn't a knob that says "I don't want to do bridging in hardware
between these two switchdev interfaces", if the driver implements bridging.

> I am not convinced that this definition would indeed contradict the
> IEEE standard. It might just be a stronger one with more guarantees.
> Assuming you have a very stupid NIC without any filtering or offloading
> capabilities, which would always forward all frames to the CPU.
> Would this NIC not comply to the standard?

I don't understand this.

> > > After those are answered, I am open to suggestions on how to fix this
> > > differently (if still needed).
> >
> > Turn off VLAN filtering, or get a commonly accepted definition of
> > promiscuity?
>
>
> Other Drivers
> =============
>
> So I tried to figure out what other existing hardware drivers do
> by grepping for drivers which do something on a change to
> NETIF_F_HW_VLAN_CTAG_FILTER.
>
> Here are the results of me trying to understand the drivers quickly.
> I hope it's somewhat close and helps:
>
> 1) aqc111
>    This controller has a HW register flag for promiscuous mode.
>    I am not sure what it does, but since this is another ASIX
>    device, the documentation from above should apply.
>
> 2) lan78xx
>    Here it began ...
>
> 3) ixgbe
>    This driver disables HW_VLAN_CTAG_FILTER if IFF_PROMISC is set.
>
> 4) ice
>    I don't know.
>
> 5) ocelot_net
>    This driver does not support promiscuous mode with the following
>    explanation:
> 	/* This doesn't handle promiscuous mode because the bridge core is
> 	 * setting IFF_PROMISC on all slave interfaces and all frames would be
> 	 * forwarded to the CPU port.
> 	 */
>    See the already mentioned discussion https://lkml.org/lkml/2019/8/29/255
>    for more.

The explanation for ocelot is bogus. Ocelot is a switch and it doesn't
implement IFF_PROMISC precisely because of that: a switch port is
promiscuous by definition. Also check out mlxsw_sp_set_rx_mode() in the
mlxsw driver. It is deliberately empty for the same reason.

> 6) liquidio
>    This driver also has a HW flag for promiscuous mode.
>    I don't know what it does, though.
>
> 7) mvpp2
>    This driver disables vid filtering if IFF_PROMISC is set.
>
> 8) efx
> 	/* Disable VLAN filtering by default.  It may be enforced if
> 	 * the feature is fixed (i.e. VLAN filters are required to
> 	 * receive VLAN tagged packets due to vPort restrictions).
> 	 */
>    I did not find a variant that really honors this feature.
>
> 9) ef100
>    I don't know.
>
> 10) mlx5
>    This driver sets a HW promisc flag, adds an `ANY` vlan filter rule
>    and ignores further changes to vlan filtering if IFF_PROMISC is set.
>
> 11) nfp
>    This driver uses a HW promisc flag.
>
> 12) atlantic
>    I don't know.
>
> 13) xlgmac
>    This one has an interesting comment in `xlgmac_set_promiscuous_mode`:
> 	/* Hardware will still perform VLAN filtering in promiscuous mode */
> 	if (enable) {
> 		xlgmac_disable_rx_vlan_filtering(pdata);
> 	} else {
> 		if (pdata->netdev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
> 			xlgmac_enable_rx_vlan_filtering(pdata);
> 	}
>
> 14) enetc
>    I think, the feature is overridden everytime in `set_rx_mode` as long
>    as IFF_PROMISC is set.

This one was added by me in 7070eea5e95a ("enetc: permit configuration
of rx-vlan-filter with ethtool"). I'll fully admit that I disabled VLAN
filtering when entering IFF_PROMISC out of stupidity, and the main goal
of that patch was something different anyway. I didn't even think of
checking the IEEE 802.3 standard, but then you had to ask :). I'm sure
many more are in the same situation, and it's what led to this chaos.
But I don't have any users that rely on this behavior of IFF_PROMISC,
and if we were to agree on a definition of promiscuity that does not
assume anything about VLAN filtering, I would be more than happy to
delete that code. In fact I've been waiting for this to reach a conclusion
so that I could do that.

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Jakub Kicinski]

On Thu, 19 Nov 2020 16:37:51 +0100 Markus Blöchl wrote:
> Implementation
> ==============
> 
> I then tried to come up with a solution in net/core that would
> universally disable vlan filtering in promiscuous mode.

Thanks for taking a look!

> Removing the features in `netdev_fix_features` is easily done, but
> updating the active set of features whenever IFF_PROMISC changes
> seems hard.
> 
> `__dev_set_promiscuity` is often called in atomic context but
> `.ndo_set_features` can sleep in many drivers.

Are there paths other than __dev_set_rx_mode() which would call
__dev_set_promiscuity() in atomic context? The saving grace about 
__dev_set_rx_mode() is that it sets promisc explicitly to disable 
unicast filtering (dev->uc_promisc), so IMO that case
(dev->promiscuity == dev->uc_promisc) does not need to disable VLAN
filtering.

But IDK if that's the only atomic path.

> Adding a work_queue or similar to every net_device seems clumsy and
> inappropriate.
> 
> Rewriting ndo_set_features of all drivers to be atomic is not a task
> you should ask from me...
> 
> Calling `netdev_update_features` after every entrypoint that locks
> the rtnl seems too error-prone and also clumsy.
> 
> Only updating the features when promiscuity was explicitly requested
> by userspace in `dev_change_flags` might leave the device in a
> weird inconsistent state.
> 
> Continue to let each driver enforce the kernels definition of
> promiscuity. They know how to update the features atomically.
> Then I am back at my original patch...
> 
> I'm afraid, I might need some guidance on how to approach this.

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Ido Schimmel]

On Wed, Nov 11, 2020 at 07:43:41AM -0800, Jakub Kicinski wrote:
> On Tue, 10 Nov 2020 16:39:58 +0100 Markus Blöchl wrote:
> > The rx-vlan-filter feature flag prevents unexpected tagged frames on
> > the wire from reaching the kernel in promiscuous mode.
> > Disable this offloading feature in the lan7800 controller whenever
> > IFF_PROMISC is set and make sure that the hardware features
> > are updated when IFF_PROMISC changes.
> > 
> > Signed-off-by: Markus Blöchl <markus.bloechl@ipetronik.com>
> > ---
> > 
> > Notes:
> >     When sniffing ethernet using a LAN7800 ethernet controller, vlan-tagged
> >     frames are silently dropped by the controller due to the
> >     RFE_CTL_VLAN_FILTER flag being set by default since commit
> >     4a27327b156e("net: lan78xx: Add support for VLAN filtering.").
> > 
> >     In order to receive those tagged frames it is necessary to manually disable
> >     rx vlan filtering using ethtool ( `ethtool -K ethX rx-vlan-filter off` or
> >     corresponding ioctls ). Setting all bits in the vlan filter table to 1 is
> >     an even worse approach, imho.
> > 
> >     As a user I would probably expect that setting IFF_PROMISC should be enough
> >     in all cases to receive all valid traffic.
> >     Therefore I think this behaviour is a bug in the driver, since other
> >     drivers (notably ixgbe) automatically disable rx-vlan-filter when
> >     IFF_PROMISC is set. Please correct me if I am wrong here.
> 
> I've been mulling over this, I'm not 100% sure that disabling VLAN
> filters on promisc is indeed the right thing to do. The ixgbe doing
> this is somewhat convincing. OTOH users would not expect flow filters
> to get disabled when promisc is on, so why disable vlan filters?
> 
> Either way we should either document this as an expected behavior or
> make the core clear the features automatically rather than force
> drivers to worry about it.
> 
> Does anyone else have an opinion, please?

I agree it is weird, but it seems to be justified by the bridge code.
Note that the code was added around 2013, so it is possible it was
influenced by the behavior of existing drivers.

1. vi net/bridge/br_vlan.c +245

/* Add VLAN to the device filter if it is supported.
 * This ensures tagged traffic enters the bridge when
 * promiscuous mode is disabled by br_manage_promisc().
 */

2. The bridge device itself will not filter packets based on their VID
when it is in promiscuous mode:

vi net/bridge/br_input.c +45

vg = br_vlan_group_rcu(br);
/* Bridge is just like any other port.  Make sure the
 * packet is allowed except in promisc modue when someone
 * may be running packet capture.
 */
if (!(brdev->flags & IFF_PROMISC) &&
    !br_allowed_egress(vg, skb)) {
	kfree_skb(skb);
	return NET_RX_DROP;
}

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Alexander Duyck]

On Wed, Nov 11, 2020 at 7:43 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 10 Nov 2020 16:39:58 +0100 Markus Blöchl wrote:
> > The rx-vlan-filter feature flag prevents unexpected tagged frames on
> > the wire from reaching the kernel in promiscuous mode.
> > Disable this offloading feature in the lan7800 controller whenever
> > IFF_PROMISC is set and make sure that the hardware features
> > are updated when IFF_PROMISC changes.
> >
> > Signed-off-by: Markus Blöchl <markus.bloechl@ipetronik.com>
> > ---
> >
> > Notes:
> >     When sniffing ethernet using a LAN7800 ethernet controller, vlan-tagged
> >     frames are silently dropped by the controller due to the
> >     RFE_CTL_VLAN_FILTER flag being set by default since commit
> >     4a27327b156e("net: lan78xx: Add support for VLAN filtering.").
> >
> >     In order to receive those tagged frames it is necessary to manually disable
> >     rx vlan filtering using ethtool ( `ethtool -K ethX rx-vlan-filter off` or
> >     corresponding ioctls ). Setting all bits in the vlan filter table to 1 is
> >     an even worse approach, imho.
> >
> >     As a user I would probably expect that setting IFF_PROMISC should be enough
> >     in all cases to receive all valid traffic.
> >     Therefore I think this behaviour is a bug in the driver, since other
> >     drivers (notably ixgbe) automatically disable rx-vlan-filter when
> >     IFF_PROMISC is set. Please correct me if I am wrong here.
>
> I've been mulling over this, I'm not 100% sure that disabling VLAN
> filters on promisc is indeed the right thing to do. The ixgbe doing
> this is somewhat convincing. OTOH users would not expect flow filters
> to get disabled when promisc is on, so why disable vlan filters?
>
> Either way we should either document this as an expected behavior or
> make the core clear the features automatically rather than force
> drivers to worry about it.
>
> Does anyone else have an opinion, please?

My personal preference is for the setting of all 4096 VLANs when
promiscuous mode is enabled which is what we do with ixgbe if
virtualization is enabled. If we want to provide an option to disable
Rx VLAN filtering then just do it via the ethtool feature bit. With
that things would be at least consistent as I suspect disabling Rx
VLAN filtering may also impact Rx VLAN tag stripping so both might
need to be updated at the same time. I know in the case of
virtualization most NICs have to leave the VLAN filtering enabled for
SR-IOV to be able to handle per VF VLAN filters regardless of the PF
setting.

Anyway that is my $.02.

- Alex

Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in promiscuous mode

[Jakub Kicinski]

On Wed, 11 Nov 2020 07:43:41 -0800 Jakub Kicinski wrote:
> >     In order to receive those tagged frames it is necessary to manually disable
> >     rx vlan filtering using ethtool ( `ethtool -K ethX rx-vlan-filter off` or
> >     corresponding ioctls ). Setting all bits in the vlan filter table to 1 is
> >     an even worse approach, imho.
> > 
> >     As a user I would probably expect that setting IFF_PROMISC should be enough
> >     in all cases to receive all valid traffic.
> >     Therefore I think this behaviour is a bug in the driver, since other
> >     drivers (notably ixgbe) automatically disable rx-vlan-filter when
> >     IFF_PROMISC is set. Please correct me if I am wrong here.  
> 
> I've been mulling over this, I'm not 100% sure that disabling VLAN
> filters on promisc is indeed the right thing to do. The ixgbe doing
> this is somewhat convincing. OTOH users would not expect flow filters
> to get disabled when promisc is on, so why disable vlan filters?
> 
> Either way we should either document this as an expected behavior or
> make the core clear the features automatically rather than force
> drivers to worry about it.
> 
> Does anyone else have an opinion, please?

Okay, I feel convinced that we should indeed let all vlan traffic thru,
thanks everyone! :)

Markus could you try to come up with a patch for the net/core/dev.c
handling which would clear NETIF_F_HW_VLAN_CTAG_FILTER and
NETIF_F_HW_VLAN_STAG_FILTER automatically so the drivers don't have 
to worry?

Whatever the driver chooses to do to disable vlan filtering is a
separate discussion AFAICT.