Thread (7 messages) flat view 7 messages, 3 authors, 2021-11-17

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

From: Leon Romanovsky <leon@kernel.org>
Date: 2021-11-17 18:24:22

On Wed, Nov 17, 2021 at 09:47:22AM -0800, Jakub Kicinski wrote:
quoted hunk ↗ jump to hunk
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help