Re: [RFC net-next 1/2] net: add netdev_refs debug

[Leon Romanovsky <leon@kernel.org>]

On Wed, Nov 17, 2021 at 09:47:22AM -0800, Jakub Kicinski wrote:
> Debugging netdev ref leaks is still pretty hard. Eric added
> optional use of a normal refcount which is useful for tracking
> abuse of existing users.
> 
> For new code, however, it'd be great if we could actually track
> the refs per-user. Allowing us to detect leaks where they happen.
> This patch introduces a netdev_ref type and uses the debug_objects
> infra to track refs being lost or misused.
> 
> In the future we can extend this structure to also catch those
> who fail to release the ref on unregistering notification.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  MAINTAINERS                 |   1 +
>  include/linux/netdev_refs.h | 104 ++++++++++++++++++++++++++++++++++++
>  lib/Kconfig.debug           |   7 +++
>  net/core/dev.c              |   8 +++
>  4 files changed, 120 insertions(+)
>  create mode 100644 include/linux/netdev_refs.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4c74516e4353..47fe27175c9f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18482,6 +18482,7 @@ F:	include/uapi/linux/pkt_sched.h
>  F:	include/uapi/linux/tc_act/
>  F:	include/uapi/linux/tc_ematch/
>  F:	net/sched/
> +F:	tools/testing/selftests/tc-testing/
>  
>  TC90522 MEDIA DRIVER
>  M:	Akihiro Tsukada <tskd08@gmail.com>
> diff --git a/include/linux/netdev_refs.h b/include/linux/netdev_refs.h
> new file mode 100644
> index 000000000000..326772ea0a63
> --- /dev/null
> +++ b/include/linux/netdev_refs.h
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef _LINUX_NETDEV_REFS_H
> +#define _LINUX_NETDEV_REFS_H
> +
> +#include <linux/debugobjects.h>
> +#include <linux/netdevice.h>
> +
> +/* Explicit netdevice references
> + * struct netdev_ref is a storage for a reference. It's equivalent
> + * to a netdev pointer, but when debug is enabled it performs extra checks.
> + * Most users will want to take a reference with netdev_hold(), access it
> + * via netdev_ref_ptr() and release with netdev_put().
> + */
> +
> +struct netdev_ref {
> +	struct net_device *dev;
> +#ifdef CONFIG_DEBUG_OBJECTS_NETDEV_REFS
> +	refcount_t cnt;
> +#endif
> +};
> +
> +extern const struct debug_obj_descr netdev_ref_debug_descr;
> +
> +/* Store a raw, unprotected pointer */
> +static inline void __netdev_ref_store(struct netdev_ref *ref,
> +				      struct net_device *dev)
> +{
> +	ref->dev = dev;
> +
> +#ifdef CONFIG_DEBUG_OBJECTS_NETDEV_REFS
> +	refcount_set(&ref->cnt, 0);

This is very uncommon pattern. I would expect that first pointer access
will start from 1, like all refcount_t users. If you still prefer to
start from 0, i suggest you to use atomic_t. 

IMHO, much better will be to use kref for this type of reference counting.

Thanks