netdev - Re: [PATCH net-next v2 3/4] devlink: expose get/put functions

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:   Sun, 31 Oct 2021 08:29:20 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, jiri@...nulli.us,
        mkubecek@...e.cz, andrew@...n.ch, f.fainelli@...il.com
Subject: Re: [PATCH net-next v2 3/4] devlink: expose get/put functions

On Sat, Oct 30, 2021 at 10:18:50AM -0700, Jakub Kicinski wrote:
> Allow those who hold implicit reference on a devlink instance
> to try to take a full ref on it. This will be used from netdev
> code which has an implicit ref because of driver call ordering.
> 
> Note that after recent changes devlink_unregister() may happen
> before netdev unregister, but devlink_free() should still happen
> after, so we are safe to try, but we can't just refcount_inc()
> and assume it's not zero.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  include/net/devlink.h | 12 ++++++++++++
>  net/core/devlink.c    |  8 +++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)

I really like this series, but your latest netdevsim RFC made me worry.

It is important to make sure that these devlink_put() and devlink_get()
calls will be out-of-reach from the drivers. Only core code should use
them.

Can you please add it as a comment above these functions?
At least for now, till we discuss your RFC.

Reviewed-by: Leon Romanovsky <leonro@...dia.com>

Thanks

> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 1b1317d378de..991ce48f77ca 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1726,6 +1726,9 @@ devlink_trap_policers_unregister(struct devlink *devlink,
>  
>  #if IS_ENABLED(CONFIG_NET_DEVLINK)
>  
> +struct devlink *__must_check devlink_try_get(struct devlink *devlink);
> +void devlink_put(struct devlink *devlink);
> +
>  void devlink_compat_running_version(struct net_device *dev,
>  				    char *buf, size_t len);
>  int devlink_compat_flash_update(struct net_device *dev, const char *file_name);
> @@ -1736,6 +1739,15 @@ int devlink_compat_switch_id_get(struct net_device *dev,
>  
>  #else
>  
> +static inline struct devlink *devlink_try_get(struct devlink *devlink)
> +{
> +	return NULL;
> +}
> +
> +static inline void devlink_put(struct devlink *devlink)
> +{
> +}
> +
>  static inline void
>  devlink_compat_running_version(struct net_device *dev, char *buf, size_t len)
>  {
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 2d8abe88c673..100d87fd3f65 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -182,15 +182,17 @@ struct net *devlink_net(const struct devlink *devlink)
>  }
>  EXPORT_SYMBOL_GPL(devlink_net);
>  
> -static void devlink_put(struct devlink *devlink)
> +void devlink_put(struct devlink *devlink)
>  {
>  	if (refcount_dec_and_test(&devlink->refcount))
>  		complete(&devlink->comp);
>  }
>  
> -static bool __must_check devlink_try_get(struct devlink *devlink)
> +struct devlink *__must_check devlink_try_get(struct devlink *devlink)
>  {
> -	return refcount_inc_not_zero(&devlink->refcount);
> +	if (refcount_inc_not_zero(&devlink->refcount))
> +		return devlink;
> +	return NULL;
>  }
>  
>  static struct devlink *devlink_get_from_attrs(struct net *net,
> -- 
> 2.31.1
>