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

[Jakub Kicinski <kuba@kernel.org>]

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