| 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: <CAMpxmJWLbUR5ojGi3vdMw-vrG3ias9yUE+ycrhZ8m=EL-GrreQ@mail.gmail.com>
Date: Thu, 26 Mar 2020 18:48:00 +0100
From: Bartosz Golaszewski <bgolaszewski@...libre.com>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: Bartosz Golaszewski <brgl@...ev.pl>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
LKML <linux-kernel@...r.kernel.org>,
netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: core: provide devm_register_netdev()
czw., 26 mar 2020 o 18:39 Heiner Kallweit <hkallweit1@...il.com> napisał(a):
>
> On 26.03.2020 14:17, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@...libre.com>
> >
> > Create a new source file for networking devres helpers and provide
> > devm_register_netdev() - a managed variant of register_netdev().
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > ---
> > I'm writing a new ethernet driver and I realized there's no devres
> > variant for register_netdev(). Since this is the only function I need
> > to get rid of the remove() callback, I thought I'll just go ahead and
> > add it and send it even before the driver to make it available to other
> > drivers.
> >
>
> Such a new functionality typically is accepted as part of series adding
> at least one user only. Therefore best submit it together with the new
> network driver.
>
Sure, will do.
> > .../driver-api/driver-model/devres.rst | 3 ++
> > include/linux/netdevice.h | 1 +
> > net/core/Makefile | 2 +-
> > net/core/devres.c | 41 +++++++++++++++++++
> > 4 files changed, 46 insertions(+), 1 deletion(-)
> > create mode 100644 net/core/devres.c
> >
> > diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> > index 46c13780994c..11a03b65196e 100644
> > --- a/Documentation/driver-api/driver-model/devres.rst
> > +++ b/Documentation/driver-api/driver-model/devres.rst
> > @@ -372,6 +372,9 @@ MUX
> > devm_mux_chip_register()
> > devm_mux_control_get()
> >
> > +NET
> > + devm_register_netdev()
> > +
> > PER-CPU MEM
> > devm_alloc_percpu()
> > devm_free_percpu()
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 6c3f7032e8d9..710a7bcfc3dc 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -4196,6 +4196,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 device *dev, struct net_device *ndev);
> > void unregister_netdev(struct net_device *dev);
> >
> > /* General hardware address lists handling functions */
> > diff --git a/net/core/Makefile b/net/core/Makefile
> > index 3e2c378e5f31..f530894068d2 100644
> > --- a/net/core/Makefile
> > +++ b/net/core/Makefile
> > @@ -8,7 +8,7 @@ obj-y := sock.o request_sock.o skbuff.o datagram.o stream.o scm.o \
> >
> > obj-$(CONFIG_SYSCTL) += sysctl_net_core.o
> >
> > -obj-y += dev.o dev_addr_lists.o dst.o netevent.o \
> > +obj-y += dev.o devres.o dev_addr_lists.o dst.o netevent.o \
> > neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
> > sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
> > fib_notifier.o xdp.o flow_offload.o
> > diff --git a/net/core/devres.c b/net/core/devres.c
> > new file mode 100644
> > index 000000000000..3c080abd1935
> > --- /dev/null
> > +++ b/net/core/devres.c
>
> Why a new source file and not just add the function to net/core/dev.c?
>
This is a common approach in most sub-systems to have a dedicated
devres.c source file for managed helpers. Eventually we could move
devm_alloc_etherdev() here as well.
>
> > @@ -0,0 +1,41 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2020 BayLibre SAS
> > + * Author: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/netdevice.h>
> > +
> > +struct netdevice_devres {
> > + struct net_device *ndev;
> > +};
> > +
>
> Adding such a struct isn't strictly needed.
>
I believe it's better in terms of readability.
> > +static void devm_netdev_release(struct device *dev, void *res)
> > +{
> > + struct netdevice_devres *this = res;
> > +
> > + unregister_netdev(this->ndev);
> > +}
> > +
> > +int devm_register_netdev(struct device *dev, struct net_device *ndev)
> > +{
>
> In this function you'd need to consider the dependency on a previous
> call to devm_alloc_etherdev(). If the netdevice is allocated non-managed,
> then free_netdev() would be called whilst the netdevice is still
> registered, what would trigger a BUG_ON(). Therefore devm_register_netdev()
> should return an error if the netdevice was allocated non-managed.
> The mentioned scenario would result from a severe programming error
> of course, but there are less experienced driver authors and the net core
> should deal gently with wrong API usage.
>
Thank you for bringing this to my attention, I wasn't aware of that.
I'll rework this.
Best regards,
Bartosz
> An example how this could be done you can find in the PCI subsystem,
> see pcim_release() and related functions like pcim_enable() and
> pcim_set_mwi().
>
> > + struct netdevice_devres *devres;
> > + int ret;
> > +
> > + devres = devres_alloc(devm_netdev_release, sizeof(*devres), GFP_KERNEL);
> > + if (!devres)
> > + return -ENOMEM;
> > +
> > + ret = register_netdev(ndev);
> > + if (ret) {
> > + devres_free(devres);
> > + return ret;
> > + }
> > +
> > + devres->ndev = ndev;
> > + devres_add(dev, devres);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(devm_register_netdev);
> >
>
Powered by blists - more mailing lists