diff mbox series

net: convert fib_treeref from int to refcount_t

Message ID 20210729071350.28919-1-yajun.deng@linux.dev
State New
Headers show
Series net: convert fib_treeref from int to refcount_t | expand

Commit Message

Yajun Deng July 29, 2021, 7:13 a.m. UTC
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>
---
 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(-)

Comments

patchwork-bot+netdevbpf@kernel.org July 30, 2021, 3:30 p.m. UTC | #1
Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Thu, 29 Jul 2021 15:13:50 +0800 you 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>

> ---

>  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(-)


Here is the summary with links:
  - net: convert fib_treeref from int to refcount_t
    https://git.kernel.org/netdev/net-next/c/79976892f7ea

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Ioana Ciornei Aug. 2, 2021, 1:37 p.m. UTC | #2
On Thu, Jul 29, 2021 at 03:13:50PM +0800, 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>


Hi,

Unfortunately, with this patch applied I get into the following WARNINGs
when booting over NFS:


[    5.042532] ------------[ cut here ]------------
[    5.047184] refcount_t: addition on 0; use-after-free.
[    5.052324] WARNING: CPU: 7 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0xa4/0x150
[    5.060500] Modules linked in:
[    5.063544] CPU: 7 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3-00864-g10b91fc2a425 #957
[    5.071709] Hardware name: NXP Layerscape LX2160ARDB (DT)
[    5.077095] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
[    5.083090] pc : refcount_warn_saturate+0xa4/0x150
[    5.087869] lr : refcount_warn_saturate+0xa4/0x150
[    5.092649] sp : ffff80001009b880
[    5.095951] x29: ffff80001009b880 x28: 0000000000000000 x27: ffff56018121b800
[    5.103077] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
[    5.110202] x23: 000000000100007f x22: ffffb6084ff072d8 x21: ffffb6084ff07000
[    5.117327] x20: 0000000000000001 x19: ffff56018121b880 x18: 0000000000000001
[    5.124451] x17: 0000000000000001 x16: 0000000000000019 x15: 0000000000000030
[    5.131577] x14: 0000000000000000 x13: ffffb6084fbb34c8 x12: 000000000000068a
[    5.138701] x11: 000000000000022e x10: ffffb6084fc0b4c8 x9 : 00000000fffff000
[    5.145827] x8 : ffffb6084fbb34c8 x7 : ffffb6084fc0b4c8 x6 : 0000000000000000
[    5.152951] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000ffffffff
[    5.160076] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff560180248000
[    5.167201] Call trace:
[    5.169635]  refcount_warn_saturate+0xa4/0x150
[    5.174067]  fib_create_info+0xc00/0xc90
[    5.177982]  fib_table_insert+0x8c/0x620
[    5.181893]  fib_magic.isra.0+0x110/0x11c
[    5.185891]  fib_add_ifaddr+0xb8/0x190
[    5.189629]  fib_inetaddr_event+0x8c/0x140
[    5.193714]  blocking_notifier_call_chain+0x70/0xac
[    5.198582]  __inet_insert_ifa+0x224/0x310
[    5.202667]  inetdev_event+0x54c/0x75c
[    5.206404]  raw_notifier_call_chain+0x58/0x80
[    5.210836]  call_netdevice_notifiers_info+0x58/0xac
[    5.215789]  __dev_notify_flags+0x50/0xcc
[    5.219788]  dev_change_flags+0x50/0x6c
[    5.223612]  ip_auto_config+0x1a8/0xe84
[    5.227437]  do_one_initcall+0x50/0x1b0
[    5.231262]  kernel_init_freeable+0x218/0x29c
[    5.235609]  kernel_init+0x28/0x130
[    5.239088]  ret_from_fork+0x10/0x18
[    5.242651] ---[ end trace b5c781c0b33f84b6 ]---
[    5.247276] ------------[ cut here ]------------
[    5.251890] refcount_t: saturated; leaking memory.
[    5.256679] WARNING: CPU: 7 PID: 1 at lib/refcount.c:22 refcount_warn_saturate+0x78/0x150
[    5.264846] Modules linked in:
[    5.267889] CPU: 7 PID: 1 Comm: swapper/0 Tainted: G        W         5.14.0-rc3-00864-g10b91fc2a425 #957
[    5.277441] Hardware name: NXP Layerscape LX2160ARDB (DT)
[    5.282826] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
[    5.288820] pc : refcount_warn_saturate+0x78/0x150
[    5.293599] lr : refcount_warn_saturate+0x78/0x150
[    5.298378] sp : ffff80001009b880
[    5.301681] x29: ffff80001009b880 x28: 0000000000000000 x27: ffff56018121b900
[    5.308806] x26: 0000000000000000 x25: ffff56018121b800 x24: 0000000000000000
[    5.315930] x23: 000000000100007f x22: ffffb6084ff072d8 x21: ffffb6084ff07000
[    5.323055] x20: 0000000000000001 x19: ffffb6084fe657c0 x18: 0000000000000000
[    5.330180] x17: 0000000000000001 x16: 0000000000000019 x15: 0000000000000030
[    5.337304] x14: 0000000000000000 x13: ffffb6084fbb34c8 x12: 0000000000000702
[    5.344429] x11: 0000000000000256 x10: ffffb6084fc0b4c8 x9 : 00000000fffff000
[    5.351554] x8 : ffffb6084fbb34c8 x7 : ffffb6084fc0b4c8 x6 : 0000000000000000
[    5.358678] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000ffffffff
[    5.365802] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff560180248000
[    5.372927] Call trace:
[    5.375361]  refcount_warn_saturate+0x78/0x150
[    5.379793]  fib_create_info+0xb70/0xc90
[    5.383704]  fib_table_insert+0x8c/0x620
[    5.387616]  fib_magic.isra.0+0x110/0x11c
[    5.391614]  fib_add_ifaddr+0x114/0x190
[    5.395438]  fib_inetaddr_event+0x8c/0x140
[    5.399522]  blocking_notifier_call_chain+0x70/0xac
[    5.404389]  __inet_insert_ifa+0x224/0x310
[    5.408473]  inetdev_event+0x54c/0x75c
[    5.412210]  raw_notifier_call_chain+0x58/0x80
[    5.416642]  call_netdevice_notifiers_info+0x58/0xac
[    5.421594]  __dev_notify_flags+0x50/0xcc
[    5.425593]  dev_change_flags+0x50/0x6c
[    5.429417]  ip_auto_config+0x1a8/0xe84
[    5.433241]  do_one_initcall+0x50/0x1b0
[    5.437064]  kernel_init_freeable+0x218/0x29c
[    5.441410]  kernel_init+0x28/0x130
[    5.444887]  ret_from_fork+0x10/0x18
[    5.448450] ---[ end trace b5c781c0b33f84b7 ]---
[    5.453084] ------------[ cut here ]------------
[    5.457695] refcount_t: underflow; use-after-free.
[    5.462481] WARNING: CPU: 7 PID: 1 at lib/refcount.c:28 refcount_warn_saturate+0xf8/0x150
[    5.470648] Modules linked in:
[    5.473690] CPU: 7 PID: 1 Comm: swapper/0 Tainted: G        W         5.14.0-rc3-00864-g10b91fc2a425 #957
[    5.483243] Hardware name: NXP Layerscape LX2160ARDB (DT)
[    5.488628] pstate: 60000005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
[    5.494622] pc : refcount_warn_saturate+0xf8/0x150
[    5.499401] lr : refcount_warn_saturate+0xf8/0x150
[    5.504180] sp : ffff80001009b9e0
[    5.507481] x29: ffff80001009b9e0 x28: 00000000ffffffef x27: 000000007f000001
[    5.514607] x26: ffff560182d06020 x25: ffff80001009baf8 x24: ffff56018146f0b0
[    5.521731] x23: ffff56018121b800 x22: 0000000000000020 x21: ffffb6084ff07000
[    5.528856] x20: ffffb6084ff072d8 x19: ffff56018121b800 x18: 0000000000000000
[    5.535980] x17: 0000000000000001 x16: 0000000000000019 x15: 0000000000000030
[    5.543105] x14: 0000000000000000 x13: ffffb6084fbb34c8 x12: 000000000000077a
[    5.550230] x11: 000000000000027e x10: ffffb6084fc0b4c8 x9 : 00000000fffff000
[    5.557354] x8 : ffffb6084fbb34c8 x7 : ffffb6084fc0b4c8 x6 : 0000000000000000
[    5.564479] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 00000000ffffffff
[    5.571604] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff560180248000
[    5.578728] Call trace:
[    5.581162]  refcount_warn_saturate+0xf8/0x150
[    5.585594]  fib_release_info+0x190/0x1d0
[    5.589592]  fib_table_insert+0x108/0x620
[    5.593590]  fib_magic.isra.0+0x110/0x11c
[    5.597588]  fib_add_ifaddr+0xb8/0x190
[    5.601325]  fib_netdev_event+0xd4/0x1cc
[    5.605236]  raw_notifier_call_chain+0x58/0x80
[    5.609669]  call_netdevice_notifiers_info+0x58/0xac
[    5.614621]  __dev_notify_flags+0x50/0xcc
[    5.618619]  dev_change_flags+0x50/0x6c
[    5.622443]  ip_auto_config+0x1a8/0xe84
[    5.626267]  do_one_initcall+0x50/0x1b0
[    5.630091]  kernel_init_freeable+0x218/0x29c
[    5.634437]  kernel_init+0x28/0x130
[    5.637914]  ret_from_fork+0x10/0x18
[    5.641477] ---[ end trace b5c781c0b33f84b8 ]---


-Ioana
David Ahern Aug. 2, 2021, 2:36 p.m. UTC | #3
On 8/2/21 7:37 AM, Ioana Ciornei wrote:
> Unfortunately, with this patch applied I get into the following WARNINGs

> when booting over NFS:


Can you test the attached?

Thanks,
From ec9d169eb33e6a65db641792821cc6a259ed9362 Mon Sep 17 00:00:00 2001
From: David Ahern <dsahern@kernel.org>
Date: Mon, 2 Aug 2021 08:29:26 -0600
Subject: [PATCH net-next] ipv4: Fix refcount warning for new fib_info

Ioana reported a refcount warning when booting over NFS:

[    5.042532] ------------[ cut here ]------------
[    5.047184] refcount_t: addition on 0; use-after-free.
[    5.052324] WARNING: CPU: 7 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0xa4/0x150
...
[    5.167201] Call trace:
[    5.169635]  refcount_warn_saturate+0xa4/0x150
[    5.174067]  fib_create_info+0xc00/0xc90
[    5.177982]  fib_table_insert+0x8c/0x620
[    5.181893]  fib_magic.isra.0+0x110/0x11c
[    5.185891]  fib_add_ifaddr+0xb8/0x190
[    5.189629]  fib_inetaddr_event+0x8c/0x140

fib_treeref needs to be set after kzalloc. The old code had a ++ which
led to the confusion when the int was replaced by a refcount_t.

Fixes: 79976892f7ea ("net: convert fib_treeref from int to refcount_t")
Signed-off-by: David Ahern <dsahern@kernel.org>
Reported-by: Ioana Ciornei <ciorneiioana@gmail.com>
Cc: Yajun Deng <yajun.deng@linux.dev>
---
 net/ipv4/fib_semantics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index fa19f4cdf3a4..f29feb7772da 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1551,7 +1551,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 		return ofi;
 	}
 
-	refcount_inc(&fi->fib_treeref);
+	refcount_set(&fi->fib_treeref, 1);
 	refcount_set(&fi->fib_clntref, 1);
 	spin_lock_bh(&fib_info_lock);
 	hlist_add_head(&fi->fib_hash,
Ioana Ciornei Aug. 2, 2021, 2:59 p.m. UTC | #4
On Mon, Aug 02, 2021 at 08:36:59AM -0600, David Ahern wrote:
> On 8/2/21 7:37 AM, Ioana Ciornei wrote:

> > Unfortunately, with this patch applied I get into the following WARNINGs

> > when booting over NFS:

> 

> Can you test the attached?

> 


Yep, it fixes the problem.

Thanks!

-Ioana
Marek Szyprowski Aug. 3, 2021, 11:08 a.m. UTC | #5
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
Yajun Deng Aug. 3, 2021, 11:17 a.m. UTC | #6
This patch from David Ahern was applied in the newest net-next.

-------- Forwarded message -------
From: "David Ahern" <dsahern@gmail.com>
To: "Ioana Ciornei" <ciorneiioana@gmail.com>, "Yajun Deng" <yajun.deng@linux.dev>
CC: davem@davemloft.net, kuba@kernel.org, yoshfuji@linux-ipv6.org, dsahern@kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-decnet-user@lists.sourceforge.net
Sent: August 2, 2021 10:36 PM
Subject: Re: [PATCH] net: convert fib_treeref from int to refcount_t
On 8/2/21 7:37 AM, Ioana Ciornei wrote:

> Unfortunately, with this patch applied I get into the following WARNINGs
> when booting over NFS:

Can you test the attached?

Thanks,
Marek Szyprowski Aug. 3, 2021, 11:24 a.m. UTC | #7
On 03.08.2021 13:17, yajun.deng@linux.dev wrote:

> This patch from David Ahern was applied in the newest net-next.

>

> -------- Forwarded message -------

> From: "David Ahern" <dsahern@gmail.com>

> To: "Ioana Ciornei" <ciorneiioana@gmail.com>, "Yajun Deng" <yajun.deng@linux.dev>

> CC: davem@davemloft.net, kuba@kernel.org, yoshfuji@linux-ipv6.org, dsahern@kernel.org,

> netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-decnet-user@lists.sourceforge.net

> Sent: August 2, 2021 10:36 PM

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

> On 8/2/21 7:37 AM, Ioana Ciornei wrote:

>

>> Unfortunately, with this patch applied I get into the following WARNINGs

>> when booting over NFS:

> Can you test the attached?


Yes, it fixes the issue on my test systems. Feel free to add:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
David Ahern Aug. 3, 2021, 2:43 p.m. UTC | #8
On 8/3/21 5:08 AM, Marek Szyprowski wrote:
> 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):

> 


fixed in net-next.
diff mbox series

Patch

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,