netdev - Re: [RFC 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-next>] [day] [month] [year] [list]

Message-ID: <YO2avXo6XSBEzZb/@lunn.ch>
Date:   Tue, 13 Jul 2021 15:53:01 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     "shenjian (K)" <shenjian15@...wei.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        linuxarm@...neuler.org
Subject: Re: [RFC net-next] net: extend netdev features

> Hi Andrew,
> 
> Thanks for your advice.
> 
> I have thought of using linkmode_ before (https://lists.openwall.net/netdev/
> 2021/06/19/11).
> 
> In my humble opinion, there are too many logical operations in stack and
> ethernet drivers,
> I'm not sure changing them to the linkmode_set_ API is the best way. For 
> example,
> 
> below is codes in ethernet drivre:
> netdev->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_GSO_UDP_TUNNEL |
>         NETIF_F_SCTP_CRC | NETIF_F_FRAGLIST;
> 
> When using linkmode_ API, I have two choices, one is to define several feature
> arrays or
> a whole features array including all the feature bits supported by the ethernet
> driver.
> const int hns3_nic_vlan_features_array[] = {
>     NETIF_F_HW_VLAN_CTAG_FILTER,
>     NETIF_F_HW_VLAN_CTAG_TX,
>     NETIF_F_HW_VLAN_CTAG_RX,
> };
> const int hns3_nic_tso_features_array[] = {
>     NETIF_F_GSO,
>     NETIF_F_TSO,
>     NETIF_F_TSO6,
>     NETIF_F_GSO_GRE,
>     ...
> };

Using arrays is the way to go. Hopefully Coccinelle can do most of the
work for you.

> linkmode_set_bit_array(hns3_nic_vlan_features_array, ARRAY_SIZE
> (hns3_nic_vlan_features_array), netedv->features);

I would probably add wrappers. So an API like

netdev_feature_set_array(ndev, int hns3_nic_tso_features_array),
                         ARRAY_SIZE(int hns3_nic_tso_features_array);

netdev_hw_feature_set_array(ndev, int hns3_nic_tso_features_array),
                            ARRAY_SIZE(int hns3_nic_vlan_features_array);

etc. This will help during the conversion. You can keep
netdev_features_t as a u64 while you convert to the mew API. Once the
conversion is done, you can then convert the implementation of the API
to a linux bitmap.

> When using netdev_feature_t *, then just need to add an arrary index.
> 
> netdev->features[0] |= NETIF_F_HW_VLAN_CTAG_FILTER |
>         NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX | xxxxxx
> 

And you want to add

netdev->features[1] |= NETIF_F_NEW_FEATURE;

This is going to result in errors, where developers add
NETIF_F_NEW_FEATURE to feature[0], and the compiler will happily do
it, no warnings. Either you need the compiler to enforce the correct
assignment to the array members somehow, or you need a uniform API
which you cannot get wrong.

> By the way, could you introduce me some code re-writing tools ? 

Coccinelle.

https://www.kernel.org/doc/html/latest/dev-tools/coccinelle.html
https://coccinelle.gitlabpages.inria.fr/website/

I've no idea if it can do this level of code re-write. You probably
want to post on the mailing list, describe what you want to do.

     Andrew