Re: [PATCH] net: convert fib_treeref from int to refcount_t
From: Marek Szyprowski <m.szyprowski@samsung.com>
Date: 2021-08-03 11:08:08
Also in:
lkml
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 <redacted>
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.
quoted hunk ↗ jump to hunk
--- 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