netdev - Re: [PATCH 05/11] net: core: provide devm_register

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:   Tue, 5 May 2020 12:25:03 -0700
From:   Edwin Peer <edwin.peer@...adcom.com>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Rob Herring <robh+dt@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Felix Fietkau <nbd@...nwrt.org>,
        John Crispin <john@...ozen.org>,
        Sean Wang <sean.wang@...iatek.com>,
        Mark Lee <Mark-MC.Lee@...iatek.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Fabien Parent <fparent@...libre.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH 05/11] net: core: provide devm_register_netdev()

On Tue, May 5, 2020 at 7:05 AM Bartosz Golaszewski <brgl@...ev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@...libre.com>
>
> Provide devm_register_netdev() - a device resource managed variant
> of register_netdev(). This new helper will only work for net_device
> structs that have a parent device assigned and are devres managed too.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> ---
>  include/linux/netdevice.h |  4 ++++
>  net/core/dev.c            | 48 +++++++++++++++++++++++++++++++++++++++
>  net/ethernet/eth.c        |  1 +
>  3 files changed, 53 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 130a668049ab..433bd5ca2efc 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1515,6 +1515,8 @@ struct net_device_ops {
>   * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
>   * @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
>   * @IFF_LIVE_RENAME_OK: rename is allowed while device is up and running
> + * @IFF_IS_DEVRES: this structure was allocated dynamically and is managed by
> + *     devres
>   */
>  enum netdev_priv_flags {
>         IFF_802_1Q_VLAN                 = 1<<0,
> @@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
>         IFF_FAILOVER_SLAVE              = 1<<28,
>         IFF_L3MDEV_RX_HANDLER           = 1<<29,
>         IFF_LIVE_RENAME_OK              = 1<<30,
> +       IFF_IS_DEVRES                   = 1<<31,
>  };
>
>  #define IFF_802_1Q_VLAN                        IFF_802_1Q_VLAN
> @@ -4206,6 +4209,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>                          count)
>
>  int register_netdev(struct net_device *dev);
> +int devm_register_netdev(struct net_device *ndev);
>  void unregister_netdev(struct net_device *dev);
>
>  /* General hardware address lists handling functions */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 522288177bbd..99db537c9468 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9519,6 +9519,54 @@ int register_netdev(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(register_netdev);
>
> +struct netdevice_devres {
> +       struct net_device *ndev;
> +};
> +
> +static void devm_netdev_release(struct device *dev, void *this)
> +{
> +       struct netdevice_devres *res = this;
> +
> +       unregister_netdev(res->ndev);
> +}
> +
> +/**
> + *     devm_register_netdev - resource managed variant of register_netdev()
> + *     @ndev: device to register
> + *
> + *     This is a devres variant of register_netdev() for which the unregister
> + *     function will be call automatically when the parent device of ndev
> + *     is detached.
> + */
> +int devm_register_netdev(struct net_device *ndev)
> +{
> +       struct netdevice_devres *dr;
> +       int ret;
> +
> +       /* struct net_device itself must be devres managed. */
> +       BUG_ON(!(ndev->priv_flags & IFF_IS_DEVRES));
> +       /* struct net_device must have a parent device - it will be the device
> +        * managing this resource.
> +        */

Catching static programming errors seems like an expensive use of the
last runtime flag in the enum. It would be weird to devres manage the
unregister and not also choose to manage the underlying memory in the
same fashion, so it wouldn't be an obvious mistake to make. If it must
be enforced, one could also iterate over the registered release
functions and check for the presence of devm_free_netdev without
burning the flag.

> +       BUG_ON(!ndev->dev.parent);
> +
> +       dr = devres_alloc(devm_netdev_release, sizeof(*dr), GFP_KERNEL);
> +       if (!dr)
> +               return -ENOMEM;
> +
> +       ret = register_netdev(ndev);
> +       if (ret) {
> +               devres_free(dr);
> +               return ret;
> +       }
> +
> +       dr->ndev = ndev;
> +       devres_add(ndev->dev.parent, dr);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(devm_register_netdev);
> +
>  int netdev_refcnt_read(const struct net_device *dev)
>  {
>         int i, refcnt = 0;
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index c8b903302ff2..ce9b5e576f20 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -423,6 +423,7 @@ struct net_device *devm_alloc_etherdev_mqs(struct device *dev, int sizeof_priv,
>
>         *dr = netdev;
>         devres_add(dev, dr);
> +       netdev->priv_flags |= IFF_IS_DEVRES;
>
>         return netdev;
>  }
> --
> 2.25.0
>

Regards,
Edwin Peer