netdev - Re: [PATCH net-next 1/2] net: create device lookup API with reference tracking

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: <CANn89iKe0XUMHqpSM7aKr1r=O-g3K2FwMyHVqvmEU17LhKy5SQ@mail.gmail.com>
Date: Fri, 9 Jun 2023 20:59:40 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, pabeni@...hat.com, 
	dsahern@...il.com
Subject: Re: [PATCH net-next 1/2] net: create device lookup API with reference tracking

On Fri, Jun 9, 2023 at 8:57 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Fri, Jun 9, 2023 at 8:32 PM Jakub Kicinski <kuba@...nel.org> wrote:
> >
>
>
> > +/**
> > + *     netdev_get_by_name() - find a device by its name
> > + *     @net: the applicable net namespace
> > + *     @name: name to find
> > + *     @tracker: tracking object for the acquired reference
> > + *     @gfp: allocation flags for the tracker
> > + *
> > + *     Find an interface by name. This can be called from any
> > + *     context and does its own locking. The returned handle has
> > + *     the usage count incremented and the caller must use netdev_put() to
> > + *     release it when it is no longer needed. %NULL is returned if no
> > + *     matching device is found.
> > + */
> > +struct net_device *netdev_get_by_name(struct net *net, const char *name,
> > +                                     netdevice_tracker *tracker, gfp_t gfp)
> > +{
> > +       struct net_device *dev;
> > +
> > +       dev = dev_get_by_name(net, name);
> > +       if (dev)
> > +               netdev_tracker_alloc(dev, tracker, gfp);
> > +       return dev;
> > +}
> > +EXPORT_SYMBOL(netdev_get_by_name);
>
>
> What about making instead dev_get_by_name(net, name) a wrapper around
> the real thing ?
>
> static inline struct net_device *dev_get_by_name(struct net *net,
> const char *name)
> {
>     return netdev_get_by_name(net, name, NULL, 0);
> }
>
> This means netdev_get_by_name() could directly use netdev_hold()
> instead of netdev_tracker_alloc() which is a bit confusing IMO.
>
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b3c13e0419356b943e90b1f46dd7e035c6ec1a9c..b99f25c7fa0aad1eeb7fa117aaea2a0e16813fe0
> 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -759,9 +759,11 @@ struct net_device *dev_get_by_name_rcu(struct net
> *net, const char *name)
>  EXPORT_SYMBOL(dev_get_by_name_rcu);
>
>  /**
> - *     dev_get_by_name         - find a device by its name
> + *     netdev_get_by_name              - find a device by its name
>   *     @net: the applicable net namespace
>   *     @name: name to find
> + *     @tracker: tracker
> + *     @gfp: allocation flag for tracker
>   *
>   *     Find an interface by name. This can be called from any
>   *     context and does its own locking. The returned handle has
> @@ -770,17 +772,18 @@ EXPORT_SYMBOL(dev_get_by_name_rcu);
>   *     matching device is found.
>   */
>
> -struct net_device *dev_get_by_name(struct net *net, const char *name)
> +struct net_device *netdev_get_by_name(struct net *net, const char *name,
> +                                    netdevice_tracker *tracker, gfp_t gfp))
>  {
>         struct net_device *dev;
>
>         rcu_read_lock();
>         dev = dev_get_by_name_rcu(net, name);
> -       dev_hold(dev);
> +       netdev_hold(dev, tracker, gfp);

Oh well, this would need GFP_ATOMIC, I guess this might be the reason
you kept netdev_tracker_alloc()
that can run after rcu_read_unlock()

>         rcu_read_unlock();
>         return dev;
>  }
> -EXPORT_SYMBOL(dev_get_by_name);
> +EXPORT_SYMBOL(netdev_get_by_name);