Re: [PATCH] net: convert fib_treeref from int to refcount_t

[Marek Szyprowski]

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@xxxxxxxxx>

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