netdev - Re: [RFCv3 PATCH net-next] net: extend netdev_features

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]

Message-ID: <YX9RCqTOAHtiGD3n@lunn.ch>
Date:   Mon, 1 Nov 2021 03:29:30 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Jian Shen <shenjian15@...wei.com>
Cc:     davem@...emloft.net, kuba@...nel.org, ecree.xilinx@...il.com,
        hkallweit1@...il.com, alexandr.lobakin@...el.com, saeed@...nel.org,
        netdev@...r.kernel.org, linuxarm@...neuler.org
Subject: Re: [RFCv3 PATCH net-next] net: extend netdev_features_t

> +#define HNS3_DEFAULT_ACTIVE_FEATURES   \
> +	(NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_CTAG_TX |  \
> +	NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_RXCSUM | NETIF_F_SG |  \
> +	NETIF_F_GSO | NETIF_F_GRO | NETIF_F_TSO | NETIF_F_TSO6 | \
> +	NETIF_F_GSO_GRE | NETIF_F_GSO_GRE_CSUM | NETIF_F_SCTP_CRC \
> +	NETIF_F_GSO_UDP_TUNNEL | NETIF_F_FRAGLIST)

This is a problem, it only works for the existing 64 bit values, but
not for the values added afterwards. I would suggest you change this
into an array of u8 bit values. That scales to 256 feature flags. And
when that overflows, we can change from an array of u8 to u16, without
any major API changes.

>  static int hns3_alloc_buffer(struct hns3_enet_ring *ring,
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 16f778887e14..9b3ab11e19c8 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -101,12 +101,12 @@ enum {
>  
>  typedef struct {
>  	DECLARE_BITMAP(bits, NETDEV_FEATURE_COUNT);
> -} netdev_features_t; 
> +} netdev_features_t;

That hunk looks odd.

>  
>  #define NETDEV_FEATURE_DWORDS	DIV_ROUND_UP(NETDEV_FEATURE_COUNT, 64)
>  
>  /* copy'n'paste compression ;) */
> -#define __NETIF_F_BIT(bit)	((netdev_features_t)1 << (bit))
> +#define __NETIF_F_BIT(bit)	((u64)1 << (bit))

You need to get away from this representation. It does not scale.

At the end of this conversion, either all NETIF_F_* macros need to be
gone, or they need to be aliases for NETIF_F_*_BIT. 

> -static inline void netdev_feature_zero(netdev_features_t *dst)
> +static inline void netdev_features_zero(netdev_features_t *dst)
>  {
>  	bitmap_zero(dst->bits, NETDEV_FEATURE_COUNT);
>  }
>  
> -static inline void netdev_feature_fill(netdev_features_t *dst)
> +static inline void netdev_features_fill(netdev_features_t *dst)
>  {
>  	bitmap_fill(dst->bits, NETDEV_FEATURE_COUNT);
>  }

I'm wondering that the value here is? What do we gain by added the s.
These changes cause a lot of churn in the users of these functions.

>  
> -static inline void netdev_feature_and(netdev_features_t *dst,
> -				      const netdev_features_t a,
> -				      const netdev_features_t b)
> +static inline netdev_features_t netdev_features_and(netdev_features_t a,
> +						    netdev_features_t b)
>  {
> -	bitmap_and(dst->bits, a.bits, b.bits, NETDEV_FEATURE_COUNT);
> +	netdev_features_t dst;
> +
> +	bitmap_and(dst.bits, a.bits, b.bits, NETDEV_FEATURE_COUNT);
> +	return dst;
>  }

The implementation needs to change, but do we need to change the
function signature? Why remove dst as a parameter?

It can be good to deliberately break the API so the compiler tells us
when we fail to update something. But do we actually need that here?
The API is nicely abstract, so i don't think a breaking change is
required.

> +/* only be used for the first 64 bits features */
> +static inline void netdev_features_set_bits(u64 bits, netdev_features_t *src)

Do we really want this special feature which only works for some
values? Does it clearly explode at compile time when used for bits
above 64?

>  {
> -	return (addr & __NETIF_F_BIT(nr)) > 0;
> +	netdev_features_t tmp;
> +
> +	bitmap_from_u64(tmp.bits, bits);
> +	*src = netdev_features_or(*src, tmp);
>  }

> +static inline void netdev_set_active_features(struct net_device *netdev,
> +					      netdev_features_t src)
> +{
> +	netdev->features = src;
> +}

_active_ is new here? 

> +static inline void netdev_set_hw_features(struct net_device *netdev,
> +					  netdev_features_t src)
> +{
> +	netdev->hw_features = src;
> +}

Here _hw_ makes sense. But i think we need some sort of
consistency. Either drop the _active_ from the function name, or
rename the netdev field active_features. 

       Andrew