From: Eric Dumazet <hidden> Date: 2021-12-02 03:21:49
From: Eric Dumazet <redacted>
Two first patches add a generic infrastructure, that will be used
to get tracking of refcount increments/decrements.
The general idea is to be able to precisely pair each decrement with
a corresponding prior increment. Both share a cookie, basically
a pointer to private data storing stack traces.
Then a series of 17 patches converts some dev_hold()/dev_put()
pairs to new hepers : dev_hold_track() and dev_put_track().
Hopefully this will be used by developpers and syzbot to
root cause bugs that cause netdevice dismantles freezes.
With CONFIG_PCPU_DEV_REFCNT=n option, we were able to detect
some class of bugs, but too late (when too many dev_put()
were happening).
Eric Dumazet (19):
lib: add reference counting tracking infrastructure
lib: add tests for reference tracker
net: add dev_hold_track() and dev_put_track() helpers
net: add net device refcount tracker to struct netdev_rx_queue
net: add net device refcount tracker to struct netdev_queue
net: add net device refcount tracker to ethtool_phys_id()
net: add net device refcount tracker to dev_ifsioc()
drop_monitor: add net device refcount tracker
net: dst: add net device refcount tracking to dst_entry
ipv6: add net device refcount tracker to rt6_probe_deferred()
sit: add net device refcount tracking to ip_tunnel
ipv6: add net device refcount tracker to struct ip6_tnl
net: add net device refcount tracker to struct neighbour
net: add net device refcount tracker to struct pneigh_entry
net: add net device refcount tracker to struct neigh_parms
net: add net device refcount tracker to struct netdev_adjacent
ipv6: add net device refcount tracker to struct inet6_dev
ipv4: add net device refcount tracker to struct in_device
net/sched: add net device refcount tracker to struct Qdisc
include/linux/inetdevice.h | 2 +
include/linux/netdevice.h | 53 ++++++++++++++
include/linux/ref_tracker.h | 73 +++++++++++++++++++
include/net/devlink.h | 3 +
include/net/dst.h | 1 +
include/net/if_inet6.h | 1 +
include/net/ip6_tunnel.h | 1 +
include/net/ip_tunnels.h | 3 +
include/net/neighbour.h | 3 +
include/net/sch_generic.h | 2 +-
lib/Kconfig | 4 ++
lib/Kconfig.debug | 10 +++
lib/Makefile | 4 +-
lib/ref_tracker.c | 140 ++++++++++++++++++++++++++++++++++++
lib/test_ref_tracker.c | 116 ++++++++++++++++++++++++++++++
net/Kconfig | 8 +++
net/core/dev.c | 10 ++-
net/core/dev_ioctl.c | 5 +-
net/core/drop_monitor.c | 4 +-
net/core/dst.c | 8 +--
net/core/neighbour.c | 18 ++---
net/core/net-sysfs.c | 8 +--
net/ethtool/ioctl.c | 5 +-
net/ipv4/devinet.c | 4 +-
net/ipv4/route.c | 7 +-
net/ipv6/addrconf.c | 4 +-
net/ipv6/addrconf_core.c | 2 +-
net/ipv6/ip6_gre.c | 8 +--
net/ipv6/ip6_tunnel.c | 4 +-
net/ipv6/ip6_vti.c | 4 +-
net/ipv6/route.c | 10 +--
net/ipv6/sit.c | 4 +-
net/sched/sch_generic.c | 4 +-
33 files changed, 481 insertions(+), 52 deletions(-)
create mode 100644 include/linux/ref_tracker.h
create mode 100644 lib/ref_tracker.c
create mode 100644 lib/test_ref_tracker.c
--
2.34.0.rc2.393.gf8c9666880-goog
From: Eric Dumazet <hidden> Date: 2021-12-02 03:21:52
From: Eric Dumazet <redacted>
It can be hard to track where references are taken and released.
In networking, we have annoying issues at device or netns dismantles,
and we had various proposals to ease root causing them.
This patch adds new infrastructure pairing refcount increases
and decreases. This will self document code, because programmers
will have to associate increments/decrements.
This is controled by CONFIG_REF_TRACKER which can be selected
by users of this feature.
This adds both cpu and memory costs, and thus should probably be
used with care.
Signed-off-by: Eric Dumazet <redacted>
---
include/linux/ref_tracker.h | 73 +++++++++++++++++++
lib/Kconfig | 4 ++
lib/Makefile | 2 +
lib/ref_tracker.c | 140 ++++++++++++++++++++++++++++++++++++
4 files changed, 219 insertions(+)
create mode 100644 include/linux/ref_tracker.h
create mode 100644 lib/ref_tracker.c
@@ -0,0 +1,73 @@+// SPDX-License-Identifier: GPL-2.0-or-later+#ifndef _LINUX_REF_TRACKER_H+#define _LINUX_REF_TRACKER_H+#include<linux/refcount.h>+#include<linux/types.h>+#include<linux/spinlock.h>++structref_tracker;++structref_tracker_dir{+#ifdef CONFIG_REF_TRACKER+spinlock_tlock;+unsignedintquarantine_avail;+refcount_tuntracked;+structlist_headlist;/* List of active trackers */+structlist_headquarantine;/* List of dead trackers */+#endif+};++#ifdef CONFIG_REF_TRACKER+staticinlinevoidref_tracker_dir_init(structref_tracker_dir*dir,+unsignedintquarantine_count)+{+INIT_LIST_HEAD(&dir->list);+INIT_LIST_HEAD(&dir->quarantine);+spin_lock_init(&dir->lock);+dir->quarantine_avail=quarantine_count;+refcount_set(&dir->untracked,1);+}++voidref_tracker_dir_exit(structref_tracker_dir*dir);++voidref_tracker_dir_print(structref_tracker_dir*dir,+unsignedintdisplay_limit);++intref_tracker_alloc(structref_tracker_dir*dir,+structref_tracker**trackerp,gfp_tgfp);++intref_tracker_free(structref_tracker_dir*dir,+structref_tracker**trackerp);++#else /* CONFIG_REF_TRACKER */++staticinlinevoidref_tracker_dir_init(structref_tracker_dir*dir,+unsignedintquarantine_count)+{+}++staticinlinevoidref_tracker_dir_exit(structref_tracker_dir*dir)+{+}++staticinlinevoidref_tracker_dir_print(structref_tracker_dir*dir,+unsignedintdisplay_limit)+{+}++staticinlineintref_tracker_alloc(structref_tracker_dir*dir,+structref_tracker**trackerp,+gfp_tgfp)+{+return0;+}++staticinlineintref_tracker_free(structref_tracker_dir*dir,+structref_tracker**trackerp)+{+return0;+}++#endif++#endif /* _LINUX_REF_TRACKER_H */
@@ -2106,6 +2106,16 @@ config BACKTRACE_SELF_TESTSayNifyouareunsure.+configTEST_REF_TRACKER+tristate"Self test for reference tracker"+depends onDEBUG_KERNEL+selectREF_TRACKER+help+Thisoptionprovidesakernelmoduleperformingtests+usingreferencetrackerinfrastructure.++SayNifyouareunsure.+configRBTREE_TESTtristate"Red-Black tree test"depends onDEBUG_KERNEL
@@ -101,7 +101,7 @@ obj-$(CONFIG_TEST_LOCKUP) += test_lockup.oobj-$(CONFIG_TEST_HMM)+=test_hmm.oobj-$(CONFIG_TEST_FREE_PAGES)+=test_free_pages.oobj-$(CONFIG_KPROBES_SANITY_TEST)+=test_kprobes.o-+obj-$(CONFIG_TEST_REF_TRACKER)+=test_ref_tracker.o## CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns# off the generation of FPU/SSE* instructions for kernel proper but FPU_FLAGS
@@ -0,0 +1,116 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*Referrencetrackerselftest.+*+*Copyright(c)2021EricDumazet<edumazet@google.com>+*/+#include<linux/init.h>+#include<linux/module.h>+#include<linux/delay.h>+#include<linux/ref_tracker.h>+#include<linux/slab.h>+#include<linux/timer.h>++staticstructref_tracker_dirref_dir;+staticstructref_tracker*tracker[20];++#define TRT_ALLOC(X) static noinline void \+alloctest_ref_tracker_alloc##X(structref_tracker_dir*dir,\+structref_tracker**trackerp)\+{\+ref_tracker_alloc(dir,trackerp,GFP_KERNEL);\+}++TRT_ALLOC(0)+TRT_ALLOC(1)+TRT_ALLOC(2)+TRT_ALLOC(3)+TRT_ALLOC(4)+TRT_ALLOC(5)+TRT_ALLOC(6)+TRT_ALLOC(7)+TRT_ALLOC(8)+TRT_ALLOC(9)+TRT_ALLOC(10)+TRT_ALLOC(11)+TRT_ALLOC(12)+TRT_ALLOC(13)+TRT_ALLOC(14)+TRT_ALLOC(15)+TRT_ALLOC(16)+TRT_ALLOC(17)+TRT_ALLOC(18)+TRT_ALLOC(19)++#undef TRT_ALLOC++staticnoinlinevoid+alloctest_ref_tracker_free(structref_tracker_dir*dir,+structref_tracker**trackerp)+{+ref_tracker_free(dir,trackerp);+}+++staticstructtimer_listtest_ref_tracker_timer;+staticatomic_ttest_ref_timer_done=ATOMIC_INIT(0);++staticvoidtest_ref_tracker_timer_func(structtimer_list*t)+{+alloctest_ref_tracker_alloc0(&ref_dir,&tracker[0]);+atomic_set(&test_ref_timer_done,1);+}++staticint__inittest_ref_tracker_init(void)+{+inti;++ref_tracker_dir_init(&ref_dir,100);++timer_setup(&test_ref_tracker_timer,test_ref_tracker_timer_func,0);+mod_timer(&test_ref_tracker_timer,jiffies+1);++alloctest_ref_tracker_alloc1(&ref_dir,&tracker[1]);+alloctest_ref_tracker_alloc2(&ref_dir,&tracker[2]);+alloctest_ref_tracker_alloc3(&ref_dir,&tracker[3]);+alloctest_ref_tracker_alloc4(&ref_dir,&tracker[4]);+alloctest_ref_tracker_alloc5(&ref_dir,&tracker[5]);+alloctest_ref_tracker_alloc6(&ref_dir,&tracker[6]);+alloctest_ref_tracker_alloc7(&ref_dir,&tracker[7]);+alloctest_ref_tracker_alloc8(&ref_dir,&tracker[8]);+alloctest_ref_tracker_alloc9(&ref_dir,&tracker[9]);+alloctest_ref_tracker_alloc10(&ref_dir,&tracker[10]);+alloctest_ref_tracker_alloc11(&ref_dir,&tracker[11]);+alloctest_ref_tracker_alloc12(&ref_dir,&tracker[12]);+alloctest_ref_tracker_alloc13(&ref_dir,&tracker[13]);+alloctest_ref_tracker_alloc14(&ref_dir,&tracker[14]);+alloctest_ref_tracker_alloc15(&ref_dir,&tracker[15]);+alloctest_ref_tracker_alloc16(&ref_dir,&tracker[16]);+alloctest_ref_tracker_alloc17(&ref_dir,&tracker[17]);+alloctest_ref_tracker_alloc18(&ref_dir,&tracker[18]);+alloctest_ref_tracker_alloc19(&ref_dir,&tracker[19]);++/* free all trackers but first 0 and 1. */+for(i=2;i<ARRAY_SIZE(tracker);i++)+alloctest_ref_tracker_free(&ref_dir,&tracker[i]);++/* Attempt to free an already freed tracker. */+alloctest_ref_tracker_free(&ref_dir,&tracker[2]);++while(!atomic_read(&test_ref_timer_done))+msleep(1);++/* This should warn about tracker[0] & tracker[1] being not freed. */+ref_tracker_dir_exit(&ref_dir);++return0;+}++staticvoid__exittest_ref_tracker_exit(void)+{+}++module_init(test_ref_tracker_init);+module_exit(test_ref_tracker_exit);++MODULE_LICENSE("GPL v2");
From: Eric Dumazet <hidden> Date: 2021-12-02 03:22:05
From: Eric Dumazet <redacted>
They should replace dev_hold() and dev_put().
To use these helpers, each data structure owning a refcount
should also use a "netdevice_tracker" to pair the hold and put.
Whenever a leak happens, we will get precise stack traces
of the point dev_hold_track() happened, at device dismantle phase.
Signed-off-by: Eric Dumazet <redacted>
---
include/linux/netdevice.h | 32 ++++++++++++++++++++++++++++++++
net/Kconfig | 8 ++++++++
net/core/dev.c | 3 +++
3 files changed, 43 insertions(+)
@@ -1044,7 +1044,7 @@ static int rx_queue_add_kobject(struct net_device *dev, int index)/* Kobject_put later will trigger rx_queue_release call which*decreasesdevrefcount:Takethatreferencehere*/-dev_hold(queue->dev);+dev_hold_track(queue->dev,&queue->dev_tracker,GFP_KERNEL);kobj->kset=dev->queues_kset;error=kobject_init_and_add(kobj,&rx_queue_ktype,NULL,
@@ -1647,7 +1647,7 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index)/* Kobject_put later will trigger netdev_queue_release call*whichdecreasesdevrefcount:Takethatreferencehere*/-dev_hold(queue->dev);+dev_hold_track(queue->dev,&queue->dev_tracker,GFP_KERNEL);kobj->kset=dev->queues_kset;error=kobject_init_and_add(kobj,&netdev_queue_ktype,NULL,
From: Eric Dumazet <hidden> Date: 2021-12-02 03:22:42
From: Eric Dumazet <redacted>
This helper might hold a netdev reference for a long time,
lets add reference tracking.
Signed-off-by: Eric Dumazet <redacted>
---
net/ethtool/ioctl.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: Eric Dumazet <hidden> Date: 2021-12-02 03:22:54
From: Eric Dumazet <redacted>
Note that other ip_tunnel users do not seem to hold a reference
on tunnel->dev. Probably needs some investigations.
Signed-off-by: Eric Dumazet <redacted>
---
include/net/ip_tunnels.h | 3 +++
net/ipv6/sit.c | 4 ++--
2 files changed, 5 insertions(+), 2 deletions(-)
@@ -104,7 +104,10 @@ struct metadata_dst;structip_tunnel{structip_tunnel__rcu*next;structhlist_nodehash_node;+structnet_device*dev;+netdevice_trackerdev_tracker;+structnet*net;/* netns for packet i/o */unsignedlongerr_time;/* Time when the last ICMP error
@@ -6534,6 +6534,7 @@ static __latent_entropy void net_rx_action(struct softirq_action *h)structnetdev_adjacent{structnet_device*dev;+netdevice_trackerdev_tracker;/* upper master flag, there can only be one master device per list */boolmaster;
@@ -7298,7 +7299,7 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,adj->ref_nr=1;adj->private=private;adj->ignore=false;-dev_hold(adj_dev);+dev_hold_track(adj_dev,&adj->dev_tracker,GFP_KERNEL);pr_debug("Insert adjacency: dev %s adj_dev %s adj->ref_nr %d; dev_hold on %s\n",dev->name,adj_dev->name,adj->ref_nr,adj_dev->name);
@@ -7327,8 +7328,8 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,if(netdev_adjacent_is_neigh_list(dev,adj_dev,dev_list))netdev_adjacent_sysfs_del(dev,adj_dev->name,dev_list);free_adj:+dev_put_track(adj_dev,&adj->dev_tracker);kfree(adj);-dev_put(adj_dev);returnret;}
@@ -7369,7 +7370,7 @@ static void __netdev_adjacent_dev_remove(struct net_device *dev,list_del_rcu(&adj->list);pr_debug("adjacency: dev_put for %s, because link removed from %s to %s\n",adj_dev->name,dev->name,adj_dev->name);-dev_put(adj_dev);+dev_put_track(adj_dev,&adj->dev_tracker);kfree_rcu(adj,rcu);}
From: Eric Dumazet <hidden> Date: 2021-12-02 03:31:18
On Wed, Dec 1, 2021 at 7:21 PM Eric Dumazet [off-list ref] wrote:
From: Eric Dumazet <redacted>
This module uses reference tracker, forcing two issues.
1) Double free of a tracker
2) leak of two trackers, one being allocated from softirq context.
"modprobe test_ref_tracker" would emit the following traces.
(Use scripts/decode_stacktrace.sh if necessary)
[
@@ -2106,6 +2106,16 @@ config BACKTRACE_SELF_TESTSayNifyouareunsure.+configTEST_REF_TRACKER+tristate"Self test for reference tracker"+depends onDEBUG_KERNEL+selectREF_TRACKER+help+Thisoptionprovidesakernelmoduleperformingtests+usingreferencetrackerinfrastructure.++SayNifyouareunsure.+configRBTREE_TESTtristate"Red-Black tree test"depends onDEBUG_KERNEL
@@ -101,7 +101,7 @@ obj-$(CONFIG_TEST_LOCKUP) += test_lockup.oobj-$(CONFIG_TEST_HMM)+=test_hmm.oobj-$(CONFIG_TEST_FREE_PAGES)+=test_free_pages.oobj-$(CONFIG_KPROBES_SANITY_TEST)+=test_kprobes.o-+obj-$(CONFIG_TEST_REF_TRACKER)+=test_ref_tracker.o## CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns# off the generation of FPU/SSE* instructions for kernel proper but FPU_FLAGS
net/core/drop_monitor.c:869:47: warning: passing argument 2 of 'dev_put_track' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
869 | dev_put_track(hw_metadata->input_dev, &hw_metadata->dev_tracker);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from net/core/drop_monitor.c:10:
include/linux/netdevice.h:3863:53: note: expected 'struct ref_tracker **' but argument is of type 'struct ref_tracker * const*'
3863 | netdevice_tracker *tracker)
| ~~~~~~~~~~~~~~~~~~~^~~~~~~
vim +869 net/core/drop_monitor.c
865
866 static void
867 net_dm_hw_metadata_free(const struct devlink_trap_metadata *hw_metadata)
868 {
> 869 dev_put_track(hw_metadata->input_dev, &hw_metadata->dev_tracker);
870 kfree(hw_metadata->fa_cookie);
871 kfree(hw_metadata->trap_name);
872 kfree(hw_metadata->trap_group_name);
873 kfree(hw_metadata);
874 }
875
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
net/core/drop_monitor.c:869:47: warning: passing argument 2 of 'dev_put_track' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
869 | dev_put_track(hw_metadata->input_dev, &hw_metadata->dev_tracker);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from net/core/drop_monitor.c:10:
include/linux/netdevice.h:3863:53: note: expected 'struct ref_tracker **' but argument is of type 'struct ref_tracker * const*'
3863 | netdevice_tracker *tracker)
| ~~~~~~~~~~~~~~~~~~~^~~~~~~
vim +869 net/core/drop_monitor.c
865
866 static void
867 net_dm_hw_metadata_free(const struct devlink_trap_metadata *hw_metadata)
Yep, I have removed this not really useful const qualifier
net_dm_hw_metadata_free(struct devlink_trap_metadata *hw_metadata)
...
On Thu, 2 Dec 2021 at 04:21, Eric Dumazet [off-list ref] wrote:
From: Eric Dumazet <redacted>
It can be hard to track where references are taken and released.
In networking, we have annoying issues at device or netns dismantles,
and we had various proposals to ease root causing them.
This patch adds new infrastructure pairing refcount increases
and decreases. This will self document code, because programmers
will have to associate increments/decrements.
This is controled by CONFIG_REF_TRACKER which can be selected
by users of this feature.
This adds both cpu and memory costs, and thus should probably be
used with care.
Signed-off-by: Eric Dumazet <redacted>
@@ -0,0 +1,73 @@+// SPDX-License-Identifier: GPL-2.0-or-later+#ifndef _LINUX_REF_TRACKER_H+#define _LINUX_REF_TRACKER_H+#include<linux/refcount.h>+#include<linux/types.h>+#include<linux/spinlock.h>++structref_tracker;++structref_tracker_dir{+#ifdef CONFIG_REF_TRACKER+spinlock_tlock;+unsignedintquarantine_avail;+refcount_tuntracked;+structlist_headlist;/* List of active trackers */+structlist_headquarantine;/* List of dead trackers */+#endif+};++#ifdef CONFIG_REF_TRACKER+staticinlinevoidref_tracker_dir_init(structref_tracker_dir*dir,+unsignedintquarantine_count)+{+INIT_LIST_HEAD(&dir->list);+INIT_LIST_HEAD(&dir->quarantine);+spin_lock_init(&dir->lock);+dir->quarantine_avail=quarantine_count;+refcount_set(&dir->untracked,1);+}++voidref_tracker_dir_exit(structref_tracker_dir*dir);++voidref_tracker_dir_print(structref_tracker_dir*dir,+unsignedintdisplay_limit);++intref_tracker_alloc(structref_tracker_dir*dir,+structref_tracker**trackerp,gfp_tgfp);++intref_tracker_free(structref_tracker_dir*dir,+structref_tracker**trackerp);++#else /* CONFIG_REF_TRACKER */++staticinlinevoidref_tracker_dir_init(structref_tracker_dir*dir,+unsignedintquarantine_count)+{+}++staticinlinevoidref_tracker_dir_exit(structref_tracker_dir*dir)+{+}++staticinlinevoidref_tracker_dir_print(structref_tracker_dir*dir,+unsignedintdisplay_limit)+{+}++staticinlineintref_tracker_alloc(structref_tracker_dir*dir,+structref_tracker**trackerp,+gfp_tgfp)+{+return0;+}++staticinlineintref_tracker_free(structref_tracker_dir*dir,+structref_tracker**trackerp)+{+return0;+}++#endif++#endif /* _LINUX_REF_TRACKER_H */
arch/sparc/kernel/kstack.h:23:13: error: 'hardirq_stack' undeclared (first use in this function)
23 | if (hardirq_stack[tp->cpu]) {
| ^~~~~~~~~~~~~
arch/sparc/kernel/kstack.h:23:13: note: each undeclared identifier is reported only once for each function it appears in
quoted
arch/sparc/kernel/kstack.h:28:40: error: 'softirq_stack' undeclared (first use in this function)
28 | base = (unsigned long) softirq_stack[tp->cpu];
| ^~~~~~~~~~~~~
arch/sparc/kernel/kstack.h: In function 'kstack_is_trap_frame':
arch/sparc/kernel/kstack.h:46:13: error: 'hardirq_stack' undeclared (first use in this function)
46 | if (hardirq_stack[tp->cpu]) {
| ^~~~~~~~~~~~~
arch/sparc/kernel/kstack.h:51:40: error: 'softirq_stack' undeclared (first use in this function)
51 | base = (unsigned long) softirq_stack[tp->cpu];
| ^~~~~~~~~~~~~
quoted
arch/sparc/kernel/kstack.h:59:18: error: 'struct pt_regs' has no member named 'magic'
59 | if ((regs->magic & ~0x1ff) == PT_REGS_MAGIC)
| ^~
quoted
arch/sparc/kernel/kstack.h:59:39: error: 'PT_REGS_MAGIC' undeclared (first use in this function); did you mean 'PT_V9_MAGIC'?
59 | if ((regs->magic & ~0x1ff) == PT_REGS_MAGIC)
| ^~~~~~~~~~~~~
| PT_V9_MAGIC
arch/sparc/kernel/kstack.h: In function 'set_hardirq_stack':
arch/sparc/kernel/kstack.h:67:30: error: 'hardirq_stack' undeclared (first use in this function); did you mean 'set_hardirq_stack'?
67 | void *orig_sp, *sp = hardirq_stack[smp_processor_id()];
| ^~~~~~~~~~~~~
| set_hardirq_stack
arch/sparc/kernel/stacktrace.c: In function '__save_stack_trace':
quoted
arch/sparc/kernel/stacktrace.c:46:35: error: 'struct pt_regs' has no member named 'tstate'
46 | if (!(regs->tstate & TSTATE_PRIV))
| ^~
quoted
arch/sparc/kernel/stacktrace.c:46:46: error: 'TSTATE_PRIV' undeclared (first use in this function)
46 | if (!(regs->tstate & TSTATE_PRIV))
| ^~~~~~~~~~~
quoted
arch/sparc/kernel/stacktrace.c:48:36: error: 'struct pt_regs' has no member named 'tpc'; did you mean 'pc'?
48 | pc = regs->tpc;
| ^~~
| pc
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for STACKTRACE
Depends on STACKTRACE_SUPPORT
Selected by
- STACKDEPOT
vim +/hardirq_stack +23 arch/sparc/kernel/kstack.h
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 9
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 10 /* SP must be STACK_BIAS adjusted already. */
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 11 static inline bool kstack_valid(struct thread_info *tp, unsigned long sp)
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 12 {
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 13 unsigned long base = (unsigned long) tp;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 14
232486e1e9f348 arch/sparc/kernel/kstack.h David S. Miller 2010-02-12 15 /* Stack pointer must be 16-byte aligned. */
232486e1e9f348 arch/sparc/kernel/kstack.h David S. Miller 2010-02-12 16 if (sp & (16UL - 1))
232486e1e9f348 arch/sparc/kernel/kstack.h David S. Miller 2010-02-12 17 return false;
232486e1e9f348 arch/sparc/kernel/kstack.h David S. Miller 2010-02-12 18
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 19 if (sp >= (base + sizeof(struct thread_info)) &&
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 20 sp <= (base + THREAD_SIZE - sizeof(struct sparc_stackf)))
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 21 return true;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 22
6f63e781eaf6a7 arch/sparc64/kernel/kstack.h David S. Miller 2008-08-13 @23 if (hardirq_stack[tp->cpu]) {
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 24 base = (unsigned long) hardirq_stack[tp->cpu];
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 25 if (sp >= base &&
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 26 sp <= (base + THREAD_SIZE - sizeof(struct sparc_stackf)))
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 27 return true;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 @28 base = (unsigned long) softirq_stack[tp->cpu];
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 29 if (sp >= base &&
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 30 sp <= (base + THREAD_SIZE - sizeof(struct sparc_stackf)))
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 31 return true;
6f63e781eaf6a7 arch/sparc64/kernel/kstack.h David S. Miller 2008-08-13 32 }
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 33 return false;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 34 }
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 35
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 36 /* Does "regs" point to a valid pt_regs trap frame? */
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 37 static inline bool kstack_is_trap_frame(struct thread_info *tp, struct pt_regs *regs)
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 38 {
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 39 unsigned long base = (unsigned long) tp;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 40 unsigned long addr = (unsigned long) regs;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 41
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 42 if (addr >= base &&
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 43 addr <= (base + THREAD_SIZE - sizeof(*regs)))
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 44 goto check_magic;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 45
6f63e781eaf6a7 arch/sparc64/kernel/kstack.h David S. Miller 2008-08-13 46 if (hardirq_stack[tp->cpu]) {
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 47 base = (unsigned long) hardirq_stack[tp->cpu];
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 48 if (addr >= base &&
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 49 addr <= (base + THREAD_SIZE - sizeof(*regs)))
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 50 goto check_magic;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 51 base = (unsigned long) softirq_stack[tp->cpu];
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 52 if (addr >= base &&
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 53 addr <= (base + THREAD_SIZE - sizeof(*regs)))
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 54 goto check_magic;
6f63e781eaf6a7 arch/sparc64/kernel/kstack.h David S. Miller 2008-08-13 55 }
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 56 return false;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 57
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 58 check_magic:
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 @59 if ((regs->magic & ~0x1ff) == PT_REGS_MAGIC)
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 60 return true;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 61 return false;
4f70f7a91bffdc arch/sparc64/kernel/kstack.h David S. Miller 2008-08-12 62
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Dec 01, 2021 at 07:21:20PM -0800, Eric Dumazet wrote:
From: Eric Dumazet <redacted>
Two first patches add a generic infrastructure, that will be used
to get tracking of refcount increments/decrements.
The general idea is to be able to precisely pair each decrement with
a corresponding prior increment. Both share a cookie, basically
a pointer to private data storing stack traces.
Then a series of 17 patches converts some dev_hold()/dev_put()
pairs to new hepers : dev_hold_track() and dev_put_track().
Hopefully this will be used by developpers and syzbot to
root cause bugs that cause netdevice dismantles freezes.
Hi Eric:
I really like the idea of adding debug informantion for debugging
refcnt problems, we have met some bugs of leaking netdev refcnt in
the past and debugging them is really HARD !!
Recently, when investigating a sk_refcnt double free bug in SMC,
I added some debug code a bit similar like this. I'm curious have
you considered expanding the ref tracker infrastructure into other
places like sock_hold/put() or even some hot path ?
AFAIU, ref tracker add a tracker inside each object who want to
hold a refcnt, and stored the callstack into the tracker.
I have 2 questions here:
1. If we want to use this in the hot path, looks like the overhead
is a bit heavy ?
2. Since we only store 1 callstack in 1 tracker, what if some object
want to hold and put refcnt in different places ?
Thanks.
With CONFIG_PCPU_DEV_REFCNT=n option, we were able to detect
some class of bugs, but too late (when too many dev_put()
were happening).
Eric Dumazet (19):
lib: add reference counting tracking infrastructure
lib: add tests for reference tracker
net: add dev_hold_track() and dev_put_track() helpers
net: add net device refcount tracker to struct netdev_rx_queue
net: add net device refcount tracker to struct netdev_queue
net: add net device refcount tracker to ethtool_phys_id()
net: add net device refcount tracker to dev_ifsioc()
drop_monitor: add net device refcount tracker
net: dst: add net device refcount tracking to dst_entry
ipv6: add net device refcount tracker to rt6_probe_deferred()
sit: add net device refcount tracking to ip_tunnel
ipv6: add net device refcount tracker to struct ip6_tnl
net: add net device refcount tracker to struct neighbour
net: add net device refcount tracker to struct pneigh_entry
net: add net device refcount tracker to struct neigh_parms
net: add net device refcount tracker to struct netdev_adjacent
ipv6: add net device refcount tracker to struct inet6_dev
ipv4: add net device refcount tracker to struct in_device
net/sched: add net device refcount tracker to struct Qdisc
include/linux/inetdevice.h | 2 +
include/linux/netdevice.h | 53 ++++++++++++++
include/linux/ref_tracker.h | 73 +++++++++++++++++++
include/net/devlink.h | 3 +
include/net/dst.h | 1 +
include/net/if_inet6.h | 1 +
include/net/ip6_tunnel.h | 1 +
include/net/ip_tunnels.h | 3 +
include/net/neighbour.h | 3 +
include/net/sch_generic.h | 2 +-
lib/Kconfig | 4 ++
lib/Kconfig.debug | 10 +++
lib/Makefile | 4 +-
lib/ref_tracker.c | 140 ++++++++++++++++++++++++++++++++++++
lib/test_ref_tracker.c | 116 ++++++++++++++++++++++++++++++
net/Kconfig | 8 +++
net/core/dev.c | 10 ++-
net/core/dev_ioctl.c | 5 +-
net/core/drop_monitor.c | 4 +-
net/core/dst.c | 8 +--
net/core/neighbour.c | 18 ++---
net/core/net-sysfs.c | 8 +--
net/ethtool/ioctl.c | 5 +-
net/ipv4/devinet.c | 4 +-
net/ipv4/route.c | 7 +-
net/ipv6/addrconf.c | 4 +-
net/ipv6/addrconf_core.c | 2 +-
net/ipv6/ip6_gre.c | 8 +--
net/ipv6/ip6_tunnel.c | 4 +-
net/ipv6/ip6_vti.c | 4 +-
net/ipv6/route.c | 10 +--
net/ipv6/sit.c | 4 +-
net/sched/sch_generic.c | 4 +-
33 files changed, 481 insertions(+), 52 deletions(-)
create mode 100644 include/linux/ref_tracker.h
create mode 100644 lib/ref_tracker.c
create mode 100644 lib/test_ref_tracker.c
--
2.34.0.rc2.393.gf8c9666880-goog
From: Eric Dumazet <hidden> Date: 2021-12-02 15:13:26
On Thu, Dec 2, 2021 at 4:49 AM dust.li [off-list ref] wrote:
On Wed, Dec 01, 2021 at 07:21:20PM -0800, Eric Dumazet wrote:
quoted
From: Eric Dumazet <redacted>
Two first patches add a generic infrastructure, that will be used
to get tracking of refcount increments/decrements.
The general idea is to be able to precisely pair each decrement with
a corresponding prior increment. Both share a cookie, basically
a pointer to private data storing stack traces.
Then a series of 17 patches converts some dev_hold()/dev_put()
pairs to new hepers : dev_hold_track() and dev_put_track().
Hopefully this will be used by developpers and syzbot to
root cause bugs that cause netdevice dismantles freezes.
Hi Eric:
I really like the idea of adding debug informantion for debugging
refcnt problems, we have met some bugs of leaking netdev refcnt in
the past and debugging them is really HARD !!
Recently, when investigating a sk_refcnt double free bug in SMC,
I added some debug code a bit similar like this. I'm curious have
you considered expanding the ref tracker infrastructure into other
places like sock_hold/put() or even some hot path ?
Sure, this should be absolutely doable with this generic infra.
AFAIU, ref tracker add a tracker inside each object who want to
hold a refcnt, and stored the callstack into the tracker.
I have 2 questions here:
1. If we want to use this in the hot path, looks like the overhead
is a bit heavy ?
Much less expensive than lockdep (we use one spinlock per struct
ref_tracker_dir), so I think this is something doable.
2. Since we only store 1 callstack in 1 tracker, what if some object
want to hold and put refcnt in different places ?
You can use a tracker on the stack (patch 6/19 net: add net device
refcount tracker to ethtool_phys_id())
For generic uses of dev_hold(), like in dev_get_by_index(), we will
probably have new helpers
so that callers can provide where the tracker is put.
Or simply use this sequence to convert a generic/untracked reference
to a tracked one.
dev = dev_get_by_index(),;
...
p->dev = dev;
dev_hold_track(dev, &p->dev_tracker, GFP...)
dev_put(dev);
Thanks.
quoted
With CONFIG_PCPU_DEV_REFCNT=n option, we were able to detect
some class of bugs, but too late (when too many dev_put()
were happening).
Eric Dumazet (19):
lib: add reference counting tracking infrastructure
lib: add tests for reference tracker
net: add dev_hold_track() and dev_put_track() helpers
net: add net device refcount tracker to struct netdev_rx_queue
net: add net device refcount tracker to struct netdev_queue
net: add net device refcount tracker to ethtool_phys_id()
net: add net device refcount tracker to dev_ifsioc()
drop_monitor: add net device refcount tracker
net: dst: add net device refcount tracking to dst_entry
ipv6: add net device refcount tracker to rt6_probe_deferred()
sit: add net device refcount tracking to ip_tunnel
ipv6: add net device refcount tracker to struct ip6_tnl
net: add net device refcount tracker to struct neighbour
net: add net device refcount tracker to struct pneigh_entry
net: add net device refcount tracker to struct neigh_parms
net: add net device refcount tracker to struct netdev_adjacent
ipv6: add net device refcount tracker to struct inet6_dev
ipv4: add net device refcount tracker to struct in_device
net/sched: add net device refcount tracker to struct Qdisc
include/linux/inetdevice.h | 2 +
include/linux/netdevice.h | 53 ++++++++++++++
include/linux/ref_tracker.h | 73 +++++++++++++++++++
include/net/devlink.h | 3 +
include/net/dst.h | 1 +
include/net/if_inet6.h | 1 +
include/net/ip6_tunnel.h | 1 +
include/net/ip_tunnels.h | 3 +
include/net/neighbour.h | 3 +
include/net/sch_generic.h | 2 +-
lib/Kconfig | 4 ++
lib/Kconfig.debug | 10 +++
lib/Makefile | 4 +-
lib/ref_tracker.c | 140 ++++++++++++++++++++++++++++++++++++
lib/test_ref_tracker.c | 116 ++++++++++++++++++++++++++++++
net/Kconfig | 8 +++
net/core/dev.c | 10 ++-
net/core/dev_ioctl.c | 5 +-
net/core/drop_monitor.c | 4 +-
net/core/dst.c | 8 +--
net/core/neighbour.c | 18 ++---
net/core/net-sysfs.c | 8 +--
net/ethtool/ioctl.c | 5 +-
net/ipv4/devinet.c | 4 +-
net/ipv4/route.c | 7 +-
net/ipv6/addrconf.c | 4 +-
net/ipv6/addrconf_core.c | 2 +-
net/ipv6/ip6_gre.c | 8 +--
net/ipv6/ip6_tunnel.c | 4 +-
net/ipv6/ip6_vti.c | 4 +-
net/ipv6/route.c | 10 +--
net/ipv6/sit.c | 4 +-
net/sched/sch_generic.c | 4 +-
33 files changed, 481 insertions(+), 52 deletions(-)
create mode 100644 include/linux/ref_tracker.h
create mode 100644 lib/ref_tracker.c
create mode 100644 lib/test_ref_tracker.c
--
2.34.0.rc2.393.gf8c9666880-goog
From: Eric Dumazet <hidden> Date: 2021-12-02 16:05:58
On Thu, Dec 2, 2021 at 12:13 AM Dmitry Vyukov [off-list ref] wrote:
On Thu, 2 Dec 2021 at 04:21, Eric Dumazet [off-list ref] wrote:
quoted
From: Eric Dumazet <redacted>
It can be hard to track where references are taken and released.
In networking, we have annoying issues at device or netns dismantles,
and we had various proposals to ease root causing them.
This patch adds new infrastructure pairing refcount increases
and decreases. This will self document code, because programmers
will have to associate increments/decrements.
This is controled by CONFIG_REF_TRACKER which can be selected
by users of this feature.
This adds both cpu and memory costs, and thus should probably be
used with care.
Signed-off-by: Eric Dumazet <redacted>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Thanks !
quoted
++ if (!tracker) {+ refcount_dec(&dir->untracked);
Nice approach.
Yes, I found that a boolean was weak, we can do a full refcounted way,
Transition from 1 to 0 will generate a warning/stacktrace
(.text+0x2e4): undefined reference to `save_stack_trace'
(.text+0x2e4): relocation truncated to fit: R_NIOS2_CALL26 against `save_stack_trace'
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for STACKTRACE
Depends on STACKTRACE_SUPPORT
Selected by
- STACKDEPOT
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
(.text+0x2e4): undefined reference to `save_stack_trace'
(.text+0x2e4): relocation truncated to fit: R_NIOS2_CALL26 against `save_stack_trace'
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for STACKTRACE
Depends on STACKTRACE_SUPPORT
Selected by
- STACKDEPOT
I am not sure I understand this.
Dmitry, do I need to add a depends on STACKTRACE_SUPPORT.
Thanks !
On Thu, Dec 02, 2021 at 07:13:12AM -0800, Eric Dumazet wrote:
On Thu, Dec 2, 2021 at 4:49 AM dust.li [off-list ref] wrote:
quoted
On Wed, Dec 01, 2021 at 07:21:20PM -0800, Eric Dumazet wrote:
quoted
From: Eric Dumazet <redacted>
Two first patches add a generic infrastructure, that will be used
to get tracking of refcount increments/decrements.
The general idea is to be able to precisely pair each decrement with
a corresponding prior increment. Both share a cookie, basically
a pointer to private data storing stack traces.
Then a series of 17 patches converts some dev_hold()/dev_put()
pairs to new hepers : dev_hold_track() and dev_put_track().
Hopefully this will be used by developpers and syzbot to
root cause bugs that cause netdevice dismantles freezes.
Hi Eric:
I really like the idea of adding debug informantion for debugging
refcnt problems, we have met some bugs of leaking netdev refcnt in
the past and debugging them is really HARD !!
Recently, when investigating a sk_refcnt double free bug in SMC,
I added some debug code a bit similar like this. I'm curious have
you considered expanding the ref tracker infrastructure into other
places like sock_hold/put() or even some hot path ?
Sure, this should be absolutely doable with this generic infra.
quoted
AFAIU, ref tracker add a tracker inside each object who want to
hold a refcnt, and stored the callstack into the tracker.
I have 2 questions here:
1. If we want to use this in the hot path, looks like the overhead
is a bit heavy ?
Much less expensive than lockdep (we use one spinlock per struct
ref_tracker_dir), so I think this is something doable.
Yeah, I'm thinking enable this feature by default in our kernel
so I care about this won't bring regression.
I did a simple test in case this might be helpful to other.
Testing was triggered by test_ref_tracker.ko run on a Intel Xeon server.
Added the following debug code:
2. Since we only store 1 callstack in 1 tracker, what if some object
want to hold and put refcnt in different places ?
You can use a tracker on the stack (patch 6/19 net: add net device
refcount tracker to ethtool_phys_id())
For generic uses of dev_hold(), like in dev_get_by_index(), we will
probably have new helpers
so that callers can provide where the tracker is put.
Or simply use this sequence to convert a generic/untracked reference
to a tracked one.
dev = dev_get_by_index(),;
...
p->dev = dev;
dev_hold_track(dev, &p->dev_tracker, GFP...)
dev_put(dev);
Yeah, I see. This looks good for netdev.
For sock_hold/put, I feel it's more complicate, I'm not sure if we
need add lots of tracker.
quoted
Thanks.
quoted
With CONFIG_PCPU_DEV_REFCNT=n option, we were able to detect
some class of bugs, but too late (when too many dev_put()
were happening).
Eric Dumazet (19):
lib: add reference counting tracking infrastructure
lib: add tests for reference tracker
net: add dev_hold_track() and dev_put_track() helpers
net: add net device refcount tracker to struct netdev_rx_queue
net: add net device refcount tracker to struct netdev_queue
net: add net device refcount tracker to ethtool_phys_id()
net: add net device refcount tracker to dev_ifsioc()
drop_monitor: add net device refcount tracker
net: dst: add net device refcount tracking to dst_entry
ipv6: add net device refcount tracker to rt6_probe_deferred()
sit: add net device refcount tracking to ip_tunnel
ipv6: add net device refcount tracker to struct ip6_tnl
net: add net device refcount tracker to struct neighbour
net: add net device refcount tracker to struct pneigh_entry
net: add net device refcount tracker to struct neigh_parms
net: add net device refcount tracker to struct netdev_adjacent
ipv6: add net device refcount tracker to struct inet6_dev
ipv4: add net device refcount tracker to struct in_device
net/sched: add net device refcount tracker to struct Qdisc
include/linux/inetdevice.h | 2 +
include/linux/netdevice.h | 53 ++++++++++++++
include/linux/ref_tracker.h | 73 +++++++++++++++++++
include/net/devlink.h | 3 +
include/net/dst.h | 1 +
include/net/if_inet6.h | 1 +
include/net/ip6_tunnel.h | 1 +
include/net/ip_tunnels.h | 3 +
include/net/neighbour.h | 3 +
include/net/sch_generic.h | 2 +-
lib/Kconfig | 4 ++
lib/Kconfig.debug | 10 +++
lib/Makefile | 4 +-
lib/ref_tracker.c | 140 ++++++++++++++++++++++++++++++++++++
lib/test_ref_tracker.c | 116 ++++++++++++++++++++++++++++++
net/Kconfig | 8 +++
net/core/dev.c | 10 ++-
net/core/dev_ioctl.c | 5 +-
net/core/drop_monitor.c | 4 +-
net/core/dst.c | 8 +--
net/core/neighbour.c | 18 ++---
net/core/net-sysfs.c | 8 +--
net/ethtool/ioctl.c | 5 +-
net/ipv4/devinet.c | 4 +-
net/ipv4/route.c | 7 +-
net/ipv6/addrconf.c | 4 +-
net/ipv6/addrconf_core.c | 2 +-
net/ipv6/ip6_gre.c | 8 +--
net/ipv6/ip6_tunnel.c | 4 +-
net/ipv6/ip6_vti.c | 4 +-
net/ipv6/route.c | 10 +--
net/ipv6/sit.c | 4 +-
net/sched/sch_generic.c | 4 +-
33 files changed, 481 insertions(+), 52 deletions(-)
create mode 100644 include/linux/ref_tracker.h
create mode 100644 lib/ref_tracker.c
create mode 100644 lib/test_ref_tracker.c
--
2.34.0.rc2.393.gf8c9666880-goog
From: Eric Dumazet <hidden> Date: 2021-12-03 04:06:43
On Thu, Dec 2, 2021 at 7:46 PM dust.li [off-list ref] wrote:
On Thu, Dec 02, 2021 at 07:13:12AM -0800, Eric Dumazet wrote:
quoted
On Thu, Dec 2, 2021 at 4:49 AM dust.li [off-list ref] wrote:
quoted
On Wed, Dec 01, 2021 at 07:21:20PM -0800, Eric Dumazet wrote:
quoted
From: Eric Dumazet <redacted>
Two first patches add a generic infrastructure, that will be used
to get tracking of refcount increments/decrements.
The general idea is to be able to precisely pair each decrement with
a corresponding prior increment. Both share a cookie, basically
a pointer to private data storing stack traces.
Then a series of 17 patches converts some dev_hold()/dev_put()
pairs to new hepers : dev_hold_track() and dev_put_track().
Hopefully this will be used by developpers and syzbot to
root cause bugs that cause netdevice dismantles freezes.
Hi Eric:
I really like the idea of adding debug informantion for debugging
refcnt problems, we have met some bugs of leaking netdev refcnt in
the past and debugging them is really HARD !!
Recently, when investigating a sk_refcnt double free bug in SMC,
I added some debug code a bit similar like this. I'm curious have
you considered expanding the ref tracker infrastructure into other
places like sock_hold/put() or even some hot path ?
Sure, this should be absolutely doable with this generic infra.
quoted
AFAIU, ref tracker add a tracker inside each object who want to
hold a refcnt, and stored the callstack into the tracker.
I have 2 questions here:
1. If we want to use this in the hot path, looks like the overhead
is a bit heavy ?
Much less expensive than lockdep (we use one spinlock per struct
ref_tracker_dir), so I think this is something doable.
Yeah, I'm thinking enable this feature by default in our kernel
so I care about this won't bring regression.
I doubt it, especially if some workload depended on device refcount being percpu
(CONFIG_PCPU_DEV_REFCNT=y)
quoted hunk
I did a simple test in case this might be helpful to other.
Testing was triggered by test_ref_tracker.ko run on a Intel Xeon server.
Added the following debug code:
Probably not.
Perhaps we can make the depth of the stack trace configurable,
but this seems premature right now.
quoted
quoted
2. Since we only store 1 callstack in 1 tracker, what if some object
want to hold and put refcnt in different places ?
You can use a tracker on the stack (patch 6/19 net: add net device
refcount tracker to ethtool_phys_id())
For generic uses of dev_hold(), like in dev_get_by_index(), we will
probably have new helpers
so that callers can provide where the tracker is put.
Or simply use this sequence to convert a generic/untracked reference
to a tracked one.
dev = dev_get_by_index(),;
...
p->dev = dev;
dev_hold_track(dev, &p->dev_tracker, GFP...)
dev_put(dev);
Yeah, I see. This looks good for netdev.
For sock_hold/put, I feel it's more complicate, I'm not sure if we
need add lots of tracker.
I fail to see why there would be a lot of trackers.
I am betting less than 50, if you only focus on the usual suspects.
quoted
quoted
Thanks.
quoted
With CONFIG_PCPU_DEV_REFCNT=n option, we were able to detect
some class of bugs, but too late (when too many dev_put()
were happening).
Eric Dumazet (19):
lib: add reference counting tracking infrastructure
lib: add tests for reference tracker
net: add dev_hold_track() and dev_put_track() helpers
net: add net device refcount tracker to struct netdev_rx_queue
net: add net device refcount tracker to struct netdev_queue
net: add net device refcount tracker to ethtool_phys_id()
net: add net device refcount tracker to dev_ifsioc()
drop_monitor: add net device refcount tracker
net: dst: add net device refcount tracking to dst_entry
ipv6: add net device refcount tracker to rt6_probe_deferred()
sit: add net device refcount tracking to ip_tunnel
ipv6: add net device refcount tracker to struct ip6_tnl
net: add net device refcount tracker to struct neighbour
net: add net device refcount tracker to struct pneigh_entry
net: add net device refcount tracker to struct neigh_parms
net: add net device refcount tracker to struct netdev_adjacent
ipv6: add net device refcount tracker to struct inet6_dev
ipv4: add net device refcount tracker to struct in_device
net/sched: add net device refcount tracker to struct Qdisc
include/linux/inetdevice.h | 2 +
include/linux/netdevice.h | 53 ++++++++++++++
include/linux/ref_tracker.h | 73 +++++++++++++++++++
include/net/devlink.h | 3 +
include/net/dst.h | 1 +
include/net/if_inet6.h | 1 +
include/net/ip6_tunnel.h | 1 +
include/net/ip_tunnels.h | 3 +
include/net/neighbour.h | 3 +
include/net/sch_generic.h | 2 +-
lib/Kconfig | 4 ++
lib/Kconfig.debug | 10 +++
lib/Makefile | 4 +-
lib/ref_tracker.c | 140 ++++++++++++++++++++++++++++++++++++
lib/test_ref_tracker.c | 116 ++++++++++++++++++++++++++++++
net/Kconfig | 8 +++
net/core/dev.c | 10 ++-
net/core/dev_ioctl.c | 5 +-
net/core/drop_monitor.c | 4 +-
net/core/dst.c | 8 +--
net/core/neighbour.c | 18 ++---
net/core/net-sysfs.c | 8 +--
net/ethtool/ioctl.c | 5 +-
net/ipv4/devinet.c | 4 +-
net/ipv4/route.c | 7 +-
net/ipv6/addrconf.c | 4 +-
net/ipv6/addrconf_core.c | 2 +-
net/ipv6/ip6_gre.c | 8 +--
net/ipv6/ip6_tunnel.c | 4 +-
net/ipv6/ip6_vti.c | 4 +-
net/ipv6/route.c | 10 +--
net/ipv6/sit.c | 4 +-
net/sched/sch_generic.c | 4 +-
33 files changed, 481 insertions(+), 52 deletions(-)
create mode 100644 include/linux/ref_tracker.h
create mode 100644 lib/ref_tracker.c
create mode 100644 lib/test_ref_tracker.c
--
2.34.0.rc2.393.gf8c9666880-goog
(.text+0x2e4): undefined reference to `save_stack_trace'
(.text+0x2e4): relocation truncated to fit: R_NIOS2_CALL26 against `save_stack_trace'
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for STACKTRACE
Depends on STACKTRACE_SUPPORT
Selected by
- STACKDEPOT
I am not sure I understand this.
Dmitry, do I need to add a depends on STACKTRACE_SUPPORT.
Humm... There is something strange about nios2 arch.
KASAN depends on ARCH_HAVE_KASAN which is not selected for nios2, so
it implicitly avoids this issue.
But I see PAGE_OWNER that also uses STACKDEPOT has "Depends on
STACKTRACE_SUPPORT".
So I guess yes.
I am not sure how Kconfig will reach if we make STACKDEPOT depend on
STACKTRACE_SUPPORT and your config says "select STACKDEPOT"...
Hi Eric, how bad would it be if we renamed dev_hold/put_track() to
netdev_hold/put()? IIUC we use the dev_ prefix for "historic reasons"
could this be an opportunity to stop doing that?
Hi Eric, how bad would it be if we renamed dev_hold/put_track() to
netdev_hold/put()? IIUC we use the dev_ prefix for "historic reasons"
could this be an opportunity to stop doing that?
From: Jakub Kicinski <kuba@kernel.org> Date: 2022-05-23 18:44:29
On Mon, 23 May 2022 11:14:27 -0700 Eric Dumazet wrote:
quoted
Hi Eric, how bad would it be if we renamed dev_hold/put_track() to
netdev_hold/put()? IIUC we use the dev_ prefix for "historic reasons"
could this be an opportunity to stop doing that?
Sure, we can do that, thanks.
Thanks! I'll send a rename after the merge window.