| Message ID | 20210802160221.27263-1-dsahern@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ipv4: Fix refcount warning for new fib_info | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: yoshfuji@linux-ipv6.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
Hi David, On 02/08/2021 18:02, David Ahern wrote: > 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. Thank you for the patch! My CI was also complaining of not being able to run kernel selftests [1]. Your patch fixes the issue, thanks! Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net> Cheers, Matt [1] https://cirrus-ci.com/task/5688032394215424
On 8/2/21 11:58 AM, Matthieu Baerts wrote: > Hi David, > > On 02/08/2021 18:02, David Ahern wrote: >> 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. > > Thank you for the patch! > > My CI was also complaining of not being able to run kernel selftests [1]. > Your patch fixes the issue, thanks! > > Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net> > Given how easily it is to trigger the warning, I get the impression the original was an untested patch.
On Mon, 2 Aug 2021 12:04:01 -0600 David Ahern wrote: > On 8/2/21 11:58 AM, Matthieu Baerts wrote: > > Hi David, > > > > On 02/08/2021 18:02, David Ahern wrote: > >> 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. > > > > Thank you for the patch! > > > > My CI was also complaining of not being able to run kernel selftests [1]. > > Your patch fixes the issue, thanks! > > > > Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net> > > > > Given how easily it is to trigger the warning, I get the impression the > original was an untested patch. Yeah :( In hindsight any refcount patch which doesn't contain a refcount_set() is suspicious. Thanks for the quick fix, applied!
August 3, 2021 4:20 AM, "Jakub Kicinski" <kuba@kernel.org> wrote: > On Mon, 2 Aug 2021 12:04:01 -0600 David Ahern wrote: > >> On 8/2/21 11:58 AM, Matthieu Baerts wrote: >> Hi David, >> >> On 02/08/2021 18:02, David Ahern wrote: >> 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. >> >> Thank you for the patch! >> >> My CI was also complaining of not being able to run kernel selftests [1]. >> Your patch fixes the issue, thanks! >> >> Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net> >> >> Given how easily it is to trigger the warning, I get the impression the >> original was an untested patch. > > Yeah :( In hindsight any refcount patch which doesn't contain a > refcount_set() is suspicious. Thanks for the quick fix, applied! Sorry for that, there is another patch needed to apply for the same reason. I just submitted it.
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 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(-)