netdev - Re: [PATCH net-next 5/6] ionic: use mc sync for multicast filters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 04 Nov 2020 17:18:30 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Shannon Nelson <snelson@...sando.io>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org
Subject: Re: [PATCH net-next 5/6] ionic: use mc sync for multicast filters

On Wed, 2020-11-04 at 14:33 -0800, Shannon Nelson wrote:
> We should be using the multicast sync routines for the
> multicast filters.
> 
> Fixes: 1800eee16676 ("net: ionic: Replace in_interrupt() usage.")
> Signed-off-by: Shannon Nelson <snelson@...sando.io>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_lif.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 28044240caf2..a58bb572b23b 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -1158,6 +1158,14 @@ static void ionic_dev_uc_sync(struct
> net_device *netdev, bool from_ndo)
>  
>  }
>  
> +static void ionic_dev_mc_sync(struct net_device *netdev, bool
> from_ndo)
> +{
> +	if (from_ndo)
> +		__dev_mc_sync(netdev, ionic_ndo_addr_add,
> ionic_ndo_addr_del);
> +	else
> +		__dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del);
> +}
> +

I don't see any point of this function since it is used in one place.
just unfold it in the caller and you will endup with less code.

also keep in mind passing boolean to functions is usually a bad idea, 
and only complicates things, keep things simple and explicit, let the
caller do what is necessary to be done, so if you must do this if
condition, do it at the caller level.

and for a future patch i strongly recommend to remove this from_ndo
flag, it is really straight forward to do for this function
1) you can just pass _addr_add/del function pointers directly
to ionic_set_rx_mode
2) remove _ionic_lif_rx_mode from ionic_set_rx_mode and unfold it in
the caller since the function is basically one giant if condition which
is called only from two places.

>  static void ionic_set_rx_mode(struct net_device *netdev, bool
> from_ndo)
>  {
>  	struct ionic_lif *lif = netdev_priv(netdev);
> @@ -1189,7 +1197,7 @@ static void ionic_set_rx_mode(struct net_device
> *netdev, bool from_ndo)
>  	}
>  
>  	/* same for multicast */
> -	ionic_dev_uc_sync(netdev, from_ndo);
> +	ionic_dev_mc_sync(netdev, from_ndo);
>  	nfilters = le32_to_cpu(lif->identity->eth.max_mcast_filters);
>  	if (netdev_mc_count(netdev) > nfilters) {
>  		rx_mode |= IONIC_RX_MODE_F_ALLMULTI;