Messages in this thread Patch in this message | | | Subject | Re: [PATCH] net: convert fib_treeref from int to refcount_t | From | Marek Szyprowski <> | Date | Tue, 3 Aug 2021 13:08:02 +0200 |
| |
Hi
On 29.07.2021 09:13, Yajun Deng wrote: > refcount_t type should be used instead of int when fib_treeref is used as > a reference counter,and avoid use-after-free risks. > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
This patch landed in linux-next 20210802 as commit 79976892f7ea ("net: convert fib_treeref from int to refcount_t"). It triggers the following warning on all my test systems (ARM32bit and ARM64bit based):
------------[ cut here ]------------ WARNING: CPU: 3 PID: 858 at lib/refcount.c:25 fib_create_info+0xbd8/0xc18 refcount_t: addition on 0; use-after-free. Modules linked in: s5p_csis s5p_mfc s5p_fimc exynos4_is_common s5p_jpeg v4l2_fwnode v4l2_async v4l2_mem2mem videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_common videodev mc s5p_cec CPU: 3 PID: 858 Comm: ip Not tainted 5.14.0-rc2-00636-g79976892f7ea #10620 Hardware name: Samsung Exynos (Flattened Device Tree) [<c0111900>] (unwind_backtrace) from [<c010d0b8>] (show_stack+0x10/0x14) [<c010d0b8>] (show_stack) from [<c0b827b0>] (dump_stack_lvl+0x58/0x70) [<c0b827b0>] (dump_stack_lvl) from [<c0127938>] (__warn+0x118/0x11c) [<c0127938>] (__warn) from [<c01279b4>] (warn_slowpath_fmt+0x78/0xbc) [<c01279b4>] (warn_slowpath_fmt) from [<c0a5b600>] (fib_create_info+0xbd8/0xc18) [<c0a5b600>] (fib_create_info) from [<c0a5fe20>] (fib_table_insert+0x90/0x650) [<c0a5fe20>] (fib_table_insert) from [<c0a54ea0>] (fib_magic+0x164/0x16c) [<c0a54ea0>] (fib_magic) from [<c0a580d0>] (fib_add_ifaddr+0x60/0x158) [<c0a580d0>] (fib_add_ifaddr) from [<c0a58e6c>] (fib_inetaddr_event+0x7c/0xc0) [<c0a58e6c>] (fib_inetaddr_event) from [<c0154ef0>] (blocking_notifier_call_chain+0x6c/0x94) [<c0154ef0>] (blocking_notifier_call_chain) from [<c0a448ec>] (__inet_insert_ifa+0x29c/0x3b8) [<c0a448ec>] (__inet_insert_ifa) from [<c0a4882c>] (inetdev_event+0x204/0x79c) [<c0a4882c>] (inetdev_event) from [<c0154c0c>] (raw_notifier_call_chain+0x34/0x6c) [<c0154c0c>] (raw_notifier_call_chain) from [<c0988900>] (__dev_notify_flags+0x5c/0xcc) [<c0988900>] (__dev_notify_flags) from [<c09890b0>] (dev_change_flags+0x3c/0x44) [<c09890b0>] (dev_change_flags) from [<c09993f8>] (do_setlink+0x338/0x9f0) [<c09993f8>] (do_setlink) from [<c099fc70>] (__rtnl_newlink+0x51c/0x804) [<c099fc70>] (__rtnl_newlink) from [<c099ff9c>] (rtnl_newlink+0x44/0x60) [<c099ff9c>] (rtnl_newlink) from [<c099ba74>] (rtnetlink_rcv_msg+0x154/0x4f4) [<c099ba74>] (rtnetlink_rcv_msg) from [<c09d44a4>] (netlink_rcv_skb+0xe4/0x118) [<c09d44a4>] (netlink_rcv_skb) from [<c09d3c0c>] (netlink_unicast+0x1ac/0x240) [<c09d3c0c>] (netlink_unicast) from [<c09d3f70>] (netlink_sendmsg+0x2d0/0x418) [<c09d3f70>] (netlink_sendmsg) from [<c0955a30>] (____sys_sendmsg+0x1d4/0x230) [<c0955a30>] (____sys_sendmsg) from [<c095755c>] (___sys_sendmsg+0x70/0x9c) [<c095755c>] (___sys_sendmsg) from [<c0957964>] (__sys_sendmsg+0x54/0x90) [<c0957964>] (__sys_sendmsg) from [<c0100060>] (ret_fast_syscall+0x0/0x2c) Exception stack(0xc346dfa8 to 0xc346dff0) dfa0: becb275c becaa6a4 00000003 becaa6b0 00000000 00000000 dfc0: becb275c becaa6a4 00000000 00000128 0050e304 61091e59 0050e000 becaa6b0 dfe0: 0000006c becaa660 004d7f80 b6e7fab8 irq event stamp: 5457 hardirqs last enabled at (5465): [<c01a53d0>] console_unlock+0x50c/0x650 hardirqs last disabled at (5484): [<c01a53b4>] console_unlock+0x4f0/0x650 softirqs last enabled at (5544): [<c0101768>] __do_softirq+0x500/0x63c softirqs last disabled at (5493): [<c0131578>] irq_exit+0x214/0x220 ---[ end trace dc2378f379f97dd0 ]---
This issue should be possible to trigger also with qemu. If you need any help in reproducing it, let me know.
> --- > include/net/dn_fib.h | 2 +- > include/net/ip_fib.h | 2 +- > net/decnet/dn_fib.c | 6 +++--- > net/ipv4/fib_semantics.c | 8 ++++---- > 4 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/include/net/dn_fib.h b/include/net/dn_fib.h > index ccc6e9df178b..ddd6565957b3 100644 > --- a/include/net/dn_fib.h > +++ b/include/net/dn_fib.h > @@ -29,7 +29,7 @@ struct dn_fib_nh { > struct dn_fib_info { > struct dn_fib_info *fib_next; > struct dn_fib_info *fib_prev; > - int fib_treeref; > + refcount_t fib_treeref; > refcount_t fib_clntref; > int fib_dead; > unsigned int fib_flags; > diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h > index 3ab2563b1a23..21c5386d4a6d 100644 > --- a/include/net/ip_fib.h > +++ b/include/net/ip_fib.h > @@ -133,7 +133,7 @@ struct fib_info { > struct hlist_node fib_lhash; > struct list_head nh_list; > struct net *fib_net; > - int fib_treeref; > + refcount_t fib_treeref; > refcount_t fib_clntref; > unsigned int fib_flags; > unsigned char fib_dead; > diff --git a/net/decnet/dn_fib.c b/net/decnet/dn_fib.c > index 77fbf8e9df4b..387a7e81dd00 100644 > --- a/net/decnet/dn_fib.c > +++ b/net/decnet/dn_fib.c > @@ -102,7 +102,7 @@ void dn_fib_free_info(struct dn_fib_info *fi) > void dn_fib_release_info(struct dn_fib_info *fi) > { > spin_lock(&dn_fib_info_lock); > - if (fi && --fi->fib_treeref == 0) { > + if (fi && refcount_dec_and_test(&fi->fib_treeref)) { > if (fi->fib_next) > fi->fib_next->fib_prev = fi->fib_prev; > if (fi->fib_prev) > @@ -385,11 +385,11 @@ struct dn_fib_info *dn_fib_create_info(const struct rtmsg *r, struct nlattr *att > if ((ofi = dn_fib_find_info(fi)) != NULL) { > fi->fib_dead = 1; > dn_fib_free_info(fi); > - ofi->fib_treeref++; > + refcount_inc(&ofi->fib_treeref); > return ofi; > } > > - fi->fib_treeref++; > + refcount_inc(&fi->fib_treeref); > refcount_set(&fi->fib_clntref, 1); > spin_lock(&dn_fib_info_lock); > fi->fib_next = dn_fib_info_list; > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c > index 4c0c33e4710d..fa19f4cdf3a4 100644 > --- a/net/ipv4/fib_semantics.c > +++ b/net/ipv4/fib_semantics.c > @@ -260,7 +260,7 @@ EXPORT_SYMBOL_GPL(free_fib_info); > void fib_release_info(struct fib_info *fi) > { > spin_lock_bh(&fib_info_lock); > - if (fi && --fi->fib_treeref == 0) { > + if (fi && refcount_dec_and_test(&fi->fib_treeref)) { > hlist_del(&fi->fib_hash); > if (fi->fib_prefsrc) > hlist_del(&fi->fib_lhash); > @@ -1373,7 +1373,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg, > if (!cfg->fc_mx) { > fi = fib_find_info_nh(net, cfg); > if (fi) { > - fi->fib_treeref++; > + refcount_inc(&fi->fib_treeref); > return fi; > } > } > @@ -1547,11 +1547,11 @@ struct fib_info *fib_create_info(struct fib_config *cfg, > if (ofi) { > fi->fib_dead = 1; > free_fib_info(fi); > - ofi->fib_treeref++; > + refcount_inc(&ofi->fib_treeref); > return ofi; > } > > - fi->fib_treeref++; > + refcount_inc(&fi->fib_treeref); > refcount_set(&fi->fib_clntref, 1); > spin_lock_bh(&fib_info_lock); > hlist_add_head(&fi->fib_hash,
Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
| |