| 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
| ||
|
Message-ID: <0c45431b-ad76-87c6-c498-f19584ae6840@huawei.com>
Date: Mon, 1 Nov 2021 17:11:04 +0800
From: "shenjian (K)" <shenjian15@...wei.com>
To: Andrew Lunn <andrew@...n.ch>
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
Hi Andrew,
Thanks for your comments!
在 2021/11/1 10:29, Andrew Lunn 写道:
>> +#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.
OK, I will change it like features_init() in drivers/net/phy/phy_device.c
I used this at fist until I add helpers for handling existing 64 bits.
>> 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.
Yes, but it can be return directly, so we don't have to change
the prototype of functions which return netdev_features_t,
like ndo_features_check.
>>
>> #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.
I kept them for I use helpers for handling existing 64 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.
This function is used to expression like below:
"lowerdev_features &= (features | ~NETIF_F_LRO);" in drivers/net/macvlan.c
>>
>> -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.
Yes, not quite necessary. The original motivation is "netdev_features_t"
can be return directly, and more close to "A = B & C".
I will change it to keep the style as bitmap_and.
>> +/* 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?
These special helpers for existing 64 bits are defined to reduce the work
of defining features groups, and peoples may get used to for using
featureA | featureB | featureC.
The main problem is the compiler can't report error when mistake passing
NETIF_F_XXX_BIT to netdev_features_set_bits.
So the solution is removing these helpers and the NETIF_F_XXX macroes ?
>> {
>> - 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?
_active is used to differentiate "netdev_active_features_xxx" with
"netdev_features_xxx"
>> +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
> .
I prefered to rename the netdev field active_features .
Jian
>
Powered by blists - more mailing lists