* [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup
2019-04-23 1:35 [PATCH net-next 0/3] ipv6: fib6_ref conversion to refcount_t Eric Dumazet
@ 2019-04-23 1:35 ` Eric Dumazet
2019-04-23 16:31 ` Wei Wang
2019-04-23 16:33 ` David Ahern
2019-04-23 1:35 ` [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper Eric Dumazet
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ messages in thread
From: Eric Dumazet @ 2019-04-23 1:35 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Wei Wang
We do not need to clear f6i->rt6i_exception_bucket right before
freeing f6i.
Note that f6i->rt6i_exception_bucket is properly protected by
f6i->exception_bucket_flushed being set to one in rt6_flush_exceptions()
under the protection of rt6_exception_lock.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
net/ipv6/ip6_fib.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index b47e15df9769ccb2919e6ec0de5edd93f2ae518f..55193859152969794dab3df02637217a7f21016f 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -175,10 +175,7 @@ void fib6_info_destroy_rcu(struct rcu_head *head)
WARN_ON(f6i->fib6_node);
bucket = rcu_dereference_protected(f6i->rt6i_exception_bucket, 1);
- if (bucket) {
- f6i->rt6i_exception_bucket = NULL;
- kfree(bucket);
- }
+ kfree(bucket);
if (f6i->rt6i_pcpu) {
int cpu;
--
2.21.0.593.g511ec345e18-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup
2019-04-23 1:35 ` [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup Eric Dumazet
@ 2019-04-23 16:31 ` Wei Wang
2019-04-23 16:33 ` David Ahern
1 sibling, 0 replies; 13+ messages in thread
From: Wei Wang @ 2019-04-23 16:31 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet
On Mon, Apr 22, 2019 at 6:35 PM Eric Dumazet <edumazet@google.com> wrote:
>
> We do not need to clear f6i->rt6i_exception_bucket right before
> freeing f6i.
>
> Note that f6i->rt6i_exception_bucket is properly protected by
> f6i->exception_bucket_flushed being set to one in rt6_flush_exceptions()
> under the protection of rt6_exception_lock.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
Acked-by: Wei Wang <weiwan@google.com>
> net/ipv6/ip6_fib.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index b47e15df9769ccb2919e6ec0de5edd93f2ae518f..55193859152969794dab3df02637217a7f21016f 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -175,10 +175,7 @@ void fib6_info_destroy_rcu(struct rcu_head *head)
> WARN_ON(f6i->fib6_node);
>
> bucket = rcu_dereference_protected(f6i->rt6i_exception_bucket, 1);
> - if (bucket) {
> - f6i->rt6i_exception_bucket = NULL;
> - kfree(bucket);
> - }
> + kfree(bucket);
>
> if (f6i->rt6i_pcpu) {
> int cpu;
> --
> 2.21.0.593.g511ec345e18-goog
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup
2019-04-23 1:35 ` [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup Eric Dumazet
2019-04-23 16:31 ` Wei Wang
@ 2019-04-23 16:33 ` David Ahern
2019-04-23 17:40 ` Eric Dumazet
1 sibling, 1 reply; 13+ messages in thread
From: David Ahern @ 2019-04-23 16:33 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet, Wei Wang
On 4/22/19 7:35 PM, Eric Dumazet wrote:
> We do not need to clear f6i->rt6i_exception_bucket right before
> freeing f6i.
>
> Note that f6i->rt6i_exception_bucket is properly protected by
> f6i->exception_bucket_flushed being set to one in rt6_flush_exceptions()
> under the protection of rt6_exception_lock.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
> net/ipv6/ip6_fib.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup
2019-04-23 16:33 ` David Ahern
@ 2019-04-23 17:40 ` Eric Dumazet
2019-04-23 19:46 ` David Ahern
0 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2019-04-23 17:40 UTC (permalink / raw)
To: David Ahern; +Cc: David S . Miller, netdev, Eric Dumazet, Wei Wang
On Tue, Apr 23, 2019 at 9:33 AM David Ahern <dsahern@gmail.com> wrote:
>
> On 4/22/19 7:35 PM, Eric Dumazet wrote:
> > We do not need to clear f6i->rt6i_exception_bucket right before
> > freeing f6i.
> >
> > Note that f6i->rt6i_exception_bucket is properly protected by
> > f6i->exception_bucket_flushed being set to one in rt6_flush_exceptions()
> > under the protection of rt6_exception_lock.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Wei Wang <weiwan@google.com>
> > ---
> > net/ipv6/ip6_fib.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
>
> Reviewed-by: David Ahern <dsahern@gmail.com>
Thanks David
I believe we have a problem with f6i->rt6i_pcpu rules :
It seems concurrent lookups can happily use it while
the call_rcu() is ongoing before fib6_info_destroy_rcu() is finally called.
We lack proper RCU rules here. Concurrent lookups should be warned
that the destroy operation is ongoing.
One solution could look at fib6_ref value : If it is 0, then we are
not allowed to mess with rt6i_pcpu.
This is why I was looking at this function :/
BUG: KASAN: user-memory-access in atomic_read
include/asm-generic/atomic-instrumented.h:26 [inline]
BUG: KASAN: user-memory-access in atomic_fetch_add_unless
include/linux/atomic-fallback.h:1086 [inline]
BUG: KASAN: user-memory-access in atomic_add_unless
include/linux/atomic-fallback.h:1111 [inline]
BUG: KASAN: user-memory-access in atomic_inc_not_zero
include/linux/atomic-fallback.h:1127 [inline]
BUG: KASAN: user-memory-access in dst_hold_safe include/net/dst.h:308 [inline]
BUG: KASAN: user-memory-access in ip6_hold_safe+0xb3/0x3a0 net/ipv6/route.c:1023
Read of size 4 at addr 00000000000191e0 by task syz-executor.0/28086
CPU: 0 PID: 28086 Comm: syz-executor.0 Not tainted 5.1.0-rc5+ #176
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
kasan_report.cold+0x5/0x40 mm/kasan/report.c:321
check_memory_region_inline mm/kasan/generic.c:185 [inline]
check_memory_region+0x123/0x190 mm/kasan/generic.c:191
kasan_check_read+0x11/0x20 mm/kasan/common.c:102
atomic_read include/asm-generic/atomic-instrumented.h:26 [inline]
atomic_fetch_add_unless include/linux/atomic-fallback.h:1086 [inline]
atomic_add_unless include/linux/atomic-fallback.h:1111 [inline]
atomic_inc_not_zero include/linux/atomic-fallback.h:1127 [inline]
dst_hold_safe include/net/dst.h:308 [inline]
ip6_hold_safe+0xb3/0x3a0 net/ipv6/route.c:1023
rt6_get_pcpu_route net/ipv6/route.c:1245 [inline]
ip6_pol_route+0x348/0x1040 net/ipv6/route.c:1906
ip6_pol_route_output+0x54/0x70 net/ipv6/route.c:2082
fib6_rule_lookup+0x269/0x560 net/ipv6/fib6_rules.c:122
ip6_route_output_flags+0x2c4/0x350 net/ipv6/route.c:2111
ip6_route_output include/net/ip6_route.h:88 [inline]
icmpv6_xrlim_allow net/ipv6/icmp.c:213 [inline]
icmp6_send+0x1150/0x21f0 net/ipv6/icmp.c:541
icmpv6_send+0xee/0x230 net/ipv6/ip6_icmp.c:43
__udp6_lib_rcv+0x1ad1/0x2cc0 net/ipv6/udp.c:939
udpv6_rcv+0x22/0x30 net/ipv6/udp.c:1028
ip6_protocol_deliver_rcu+0x303/0x16c0 net/ipv6/ip6_input.c:394
ip6_input_finish+0x84/0x170 net/ipv6/ip6_input.c:434
NF_HOOK include/linux/netfilter.h:289 [inline]
NF_HOOK include/linux/netfilter.h:283 [inline]
ip6_input+0xe4/0x3f0 net/ipv6/ip6_input.c:443
dst_input include/net/dst.h:450 [inline]
ip6_rcv_finish+0x1e7/0x320 net/ipv6/ip6_input.c:76
NF_HOOK include/linux/netfilter.h:289 [inline]
NF_HOOK include/linux/netfilter.h:283 [inline]
ipv6_rcv+0x10e/0x420 net/ipv6/ip6_input.c:272
__netif_receive_skb_one_core+0x115/0x1a0 net/core/dev.c:4987
__netif_receive_skb+0x2c/0x1c0 net/core/dev.c:5099
netif_receive_skb_internal+0x117/0x660 net/core/dev.c:5202
napi_frags_finish net/core/dev.c:5769 [inline]
napi_gro_frags+0xade/0xd10 net/core/dev.c:5843
tun_get_user+0x2f24/0x3fb0 drivers/net/tun.c:1981
tun_chr_write_iter+0xbd/0x156 drivers/net/tun.c:2027
call_write_iter include/linux/fs.h:1866 [inline]
do_iter_readv_writev+0x5e1/0x8e0 fs/read_write.c:681
do_iter_write fs/read_write.c:957 [inline]
do_iter_write+0x184/0x610 fs/read_write.c:938
vfs_writev+0x1b3/0x2f0 fs/read_write.c:1002
do_writev+0x15e/0x370 fs/read_write.c:1037
__do_sys_writev fs/read_write.c:1110 [inline]
__se_sys_writev fs/read_write.c:1107 [inline]
__x64_sys_writev+0x75/0xb0 fs/read_write.c:1107
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup
2019-04-23 17:40 ` Eric Dumazet
@ 2019-04-23 19:46 ` David Ahern
0 siblings, 0 replies; 13+ messages in thread
From: David Ahern @ 2019-04-23 19:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, Wei Wang
On 4/23/19 11:40 AM, Eric Dumazet wrote:
> I believe we have a problem with f6i->rt6i_pcpu rules :
>
> It seems concurrent lookups can happily use it while
> the call_rcu() is ongoing before fib6_info_destroy_rcu() is finally called.
>
> We lack proper RCU rules here. Concurrent lookups should be warned
> that the destroy operation is ongoing.
>
> One solution could look at fib6_ref value : If it is 0, then we are
> not allowed to mess with rt6i_pcpu.
ok. I'll take a look as part of the testing I am doing for other changes.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper
2019-04-23 1:35 [PATCH net-next 0/3] ipv6: fib6_ref conversion to refcount_t Eric Dumazet
2019-04-23 1:35 ` [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup Eric Dumazet
@ 2019-04-23 1:35 ` Eric Dumazet
2019-04-23 16:31 ` Wei Wang
2019-04-23 16:36 ` David Ahern
2019-04-23 1:35 ` [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t Eric Dumazet
2019-04-24 0:23 ` [PATCH net-next 0/3] ipv6: fib6_ref conversion " David Miller
3 siblings, 2 replies; 13+ messages in thread
From: Eric Dumazet @ 2019-04-23 1:35 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Wei Wang
Instead of using atomic_inc(), prefer fib6_info_hold()
so that upcoming refcount_t conversion is simpler.
Only fib6_info_alloc() is using atomic_set() since we
just allocated a new object.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
net/ipv6/ip6_fib.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 55193859152969794dab3df02637217a7f21016f..a5e83593e0e45c2762eca85d04757f9d8e118e0f 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -162,7 +162,7 @@ struct fib6_info *fib6_info_alloc(gfp_t gfp_flags)
}
INIT_LIST_HEAD(&f6i->fib6_siblings);
- atomic_inc(&f6i->fib6_ref);
+ atomic_set(&f6i->fib6_ref, 1);
return f6i;
}
@@ -846,8 +846,8 @@ static struct fib6_node *fib6_add_1(struct net *net,
RCU_INIT_POINTER(in->parent, pn);
in->leaf = fn->leaf;
- atomic_inc(&rcu_dereference_protected(in->leaf,
- lockdep_is_held(&table->tb6_lock))->fib6_ref);
+ fib6_info_hold(rcu_dereference_protected(in->leaf,
+ lockdep_is_held(&table->tb6_lock)));
/* update parent pointer */
if (dir)
@@ -942,7 +942,7 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
struct fib6_info *new_leaf;
if (!(fn->fn_flags & RTN_RTINFO) && leaf == rt) {
new_leaf = fib6_find_prefix(net, table, fn);
- atomic_inc(&new_leaf->fib6_ref);
+ fib6_info_hold(new_leaf);
rcu_assign_pointer(fn->leaf, new_leaf);
fib6_info_release(rt);
@@ -1108,7 +1108,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
return err;
rcu_assign_pointer(rt->fib6_next, iter);
- atomic_inc(&rt->fib6_ref);
+ fib6_info_hold(rt);
rcu_assign_pointer(rt->fib6_node, fn);
rcu_assign_pointer(*ins, rt);
if (!info->skip_notify)
@@ -1136,7 +1136,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
if (err)
return err;
- atomic_inc(&rt->fib6_ref);
+ fib6_info_hold(rt);
rcu_assign_pointer(rt->fib6_node, fn);
rt->fib6_next = iter->fib6_next;
rcu_assign_pointer(*ins, rt);
@@ -1278,7 +1278,7 @@ int fib6_add(struct fib6_node *root, struct fib6_info *rt,
if (!sfn)
goto failure;
- atomic_inc(&info->nl_net->ipv6.fib6_null_entry->fib6_ref);
+ fib6_info_hold(info->nl_net->ipv6.fib6_null_entry);
rcu_assign_pointer(sfn->leaf,
info->nl_net->ipv6.fib6_null_entry);
sfn->fn_flags = RTN_ROOT;
@@ -1321,7 +1321,7 @@ int fib6_add(struct fib6_node *root, struct fib6_info *rt,
rcu_assign_pointer(fn->leaf,
info->nl_net->ipv6.fib6_null_entry);
} else {
- atomic_inc(&rt->fib6_ref);
+ fib6_info_hold(rt);
rcu_assign_pointer(fn->leaf, rt);
}
}
--
2.21.0.593.g511ec345e18-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper
2019-04-23 1:35 ` [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper Eric Dumazet
@ 2019-04-23 16:31 ` Wei Wang
2019-04-23 16:36 ` David Ahern
1 sibling, 0 replies; 13+ messages in thread
From: Wei Wang @ 2019-04-23 16:31 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet
On Mon, Apr 22, 2019 at 6:35 PM Eric Dumazet <edumazet@google.com> wrote:
>
> Instead of using atomic_inc(), prefer fib6_info_hold()
> so that upcoming refcount_t conversion is simpler.
>
> Only fib6_info_alloc() is using atomic_set() since we
> just allocated a new object.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
Acked-by: Wei Wang <weiwan@google.com>
> net/ipv6/ip6_fib.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 55193859152969794dab3df02637217a7f21016f..a5e83593e0e45c2762eca85d04757f9d8e118e0f 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -162,7 +162,7 @@ struct fib6_info *fib6_info_alloc(gfp_t gfp_flags)
> }
>
> INIT_LIST_HEAD(&f6i->fib6_siblings);
> - atomic_inc(&f6i->fib6_ref);
> + atomic_set(&f6i->fib6_ref, 1);
>
> return f6i;
> }
> @@ -846,8 +846,8 @@ static struct fib6_node *fib6_add_1(struct net *net,
>
> RCU_INIT_POINTER(in->parent, pn);
> in->leaf = fn->leaf;
> - atomic_inc(&rcu_dereference_protected(in->leaf,
> - lockdep_is_held(&table->tb6_lock))->fib6_ref);
> + fib6_info_hold(rcu_dereference_protected(in->leaf,
> + lockdep_is_held(&table->tb6_lock)));
>
> /* update parent pointer */
> if (dir)
> @@ -942,7 +942,7 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
> struct fib6_info *new_leaf;
> if (!(fn->fn_flags & RTN_RTINFO) && leaf == rt) {
> new_leaf = fib6_find_prefix(net, table, fn);
> - atomic_inc(&new_leaf->fib6_ref);
> + fib6_info_hold(new_leaf);
>
> rcu_assign_pointer(fn->leaf, new_leaf);
> fib6_info_release(rt);
> @@ -1108,7 +1108,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
> return err;
>
> rcu_assign_pointer(rt->fib6_next, iter);
> - atomic_inc(&rt->fib6_ref);
> + fib6_info_hold(rt);
> rcu_assign_pointer(rt->fib6_node, fn);
> rcu_assign_pointer(*ins, rt);
> if (!info->skip_notify)
> @@ -1136,7 +1136,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
> if (err)
> return err;
>
> - atomic_inc(&rt->fib6_ref);
> + fib6_info_hold(rt);
> rcu_assign_pointer(rt->fib6_node, fn);
> rt->fib6_next = iter->fib6_next;
> rcu_assign_pointer(*ins, rt);
> @@ -1278,7 +1278,7 @@ int fib6_add(struct fib6_node *root, struct fib6_info *rt,
> if (!sfn)
> goto failure;
>
> - atomic_inc(&info->nl_net->ipv6.fib6_null_entry->fib6_ref);
> + fib6_info_hold(info->nl_net->ipv6.fib6_null_entry);
> rcu_assign_pointer(sfn->leaf,
> info->nl_net->ipv6.fib6_null_entry);
> sfn->fn_flags = RTN_ROOT;
> @@ -1321,7 +1321,7 @@ int fib6_add(struct fib6_node *root, struct fib6_info *rt,
> rcu_assign_pointer(fn->leaf,
> info->nl_net->ipv6.fib6_null_entry);
> } else {
> - atomic_inc(&rt->fib6_ref);
> + fib6_info_hold(rt);
> rcu_assign_pointer(fn->leaf, rt);
> }
> }
> --
> 2.21.0.593.g511ec345e18-goog
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper
2019-04-23 1:35 ` [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper Eric Dumazet
2019-04-23 16:31 ` Wei Wang
@ 2019-04-23 16:36 ` David Ahern
1 sibling, 0 replies; 13+ messages in thread
From: David Ahern @ 2019-04-23 16:36 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet, Wei Wang
On 4/22/19 7:35 PM, Eric Dumazet wrote:
> Instead of using atomic_inc(), prefer fib6_info_hold()
> so that upcoming refcount_t conversion is simpler.
>
> Only fib6_info_alloc() is using atomic_set() since we
> just allocated a new object.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
> net/ipv6/ip6_fib.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t
2019-04-23 1:35 [PATCH net-next 0/3] ipv6: fib6_ref conversion to refcount_t Eric Dumazet
2019-04-23 1:35 ` [PATCH net-next 1/3] ipv6: fib6_info_destroy_rcu() cleanup Eric Dumazet
2019-04-23 1:35 ` [PATCH net-next 2/3] ipv6: broadly use fib6_info_hold() helper Eric Dumazet
@ 2019-04-23 1:35 ` Eric Dumazet
2019-04-23 16:32 ` Wei Wang
2019-04-23 16:37 ` David Ahern
2019-04-24 0:23 ` [PATCH net-next 0/3] ipv6: fib6_ref conversion " David Miller
3 siblings, 2 replies; 13+ messages in thread
From: Eric Dumazet @ 2019-04-23 1:35 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Wei Wang
We suspect some issues involving fib6_ref 0 -> 1 transitions might
cause strange syzbot reports.
Lets convert fib6_ref to refcount_t to catch them earlier.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <weiwan@google.com>
---
include/net/ip6_fib.h | 8 ++++----
net/ipv6/ip6_fib.c | 6 +++---
net/ipv6/route.c | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 6b7557b71c8caef7785dd0c8c52dfb92cf4e8c52..355a47bfc452aa150f537522dfb77ffa50bcb9ec 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -146,7 +146,7 @@ struct fib6_info {
struct list_head fib6_siblings;
unsigned int fib6_nsiblings;
- atomic_t fib6_ref;
+ refcount_t fib6_ref;
unsigned long expires;
struct dst_metrics *fib6_metrics;
#define fib6_pmtu fib6_metrics->metrics[RTAX_MTU-1]
@@ -284,17 +284,17 @@ void fib6_info_destroy_rcu(struct rcu_head *head);
static inline void fib6_info_hold(struct fib6_info *f6i)
{
- atomic_inc(&f6i->fib6_ref);
+ refcount_inc(&f6i->fib6_ref);
}
static inline bool fib6_info_hold_safe(struct fib6_info *f6i)
{
- return atomic_inc_not_zero(&f6i->fib6_ref);
+ return refcount_inc_not_zero(&f6i->fib6_ref);
}
static inline void fib6_info_release(struct fib6_info *f6i)
{
- if (f6i && atomic_dec_and_test(&f6i->fib6_ref))
+ if (f6i && refcount_dec_and_test(&f6i->fib6_ref))
call_rcu(&f6i->rcu, fib6_info_destroy_rcu);
}
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index a5e83593e0e45c2762eca85d04757f9d8e118e0f..a8919c217cc214821e039786492ed284552cb0b4 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -162,7 +162,7 @@ struct fib6_info *fib6_info_alloc(gfp_t gfp_flags)
}
INIT_LIST_HEAD(&f6i->fib6_siblings);
- atomic_set(&f6i->fib6_ref, 1);
+ refcount_set(&f6i->fib6_ref, 1);
return f6i;
}
@@ -929,7 +929,7 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
{
struct fib6_table *table = rt->fib6_table;
- if (atomic_read(&rt->fib6_ref) != 1) {
+ if (refcount_read(&rt->fib6_ref) != 1) {
/* This route is used as dummy address holder in some split
* nodes. It is not leaked, but it still holds other resources,
* which must be released in time. So, scan ascendant nodes
@@ -2311,7 +2311,7 @@ static int ipv6_route_seq_show(struct seq_file *seq, void *v)
dev = rt->fib6_nh.fib_nh_dev;
seq_printf(seq, " %08x %08x %08x %08x %8s\n",
- rt->fib6_metric, atomic_read(&rt->fib6_ref), 0,
+ rt->fib6_metric, refcount_read(&rt->fib6_ref), 0,
flags, dev ? dev->name : "");
iter->w.leaf = NULL;
return 0;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index e8c73b7782cdc5a2c4ca5ef50aca4c8d8556c7be..7ce14a4c3d89955d6e6468bb3cdac5efa394839b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -296,7 +296,7 @@ static const struct fib6_info fib6_null_entry_template = {
.fib6_flags = (RTF_REJECT | RTF_NONEXTHOP),
.fib6_protocol = RTPROT_KERNEL,
.fib6_metric = ~(u32)0,
- .fib6_ref = ATOMIC_INIT(1),
+ .fib6_ref = REFCOUNT_INIT(1),
.fib6_type = RTN_UNREACHABLE,
.fib6_metrics = (struct dst_metrics *)&dst_default_metrics,
};
--
2.21.0.593.g511ec345e18-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t
2019-04-23 1:35 ` [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t Eric Dumazet
@ 2019-04-23 16:32 ` Wei Wang
2019-04-23 16:37 ` David Ahern
1 sibling, 0 replies; 13+ messages in thread
From: Wei Wang @ 2019-04-23 16:32 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet
On Mon, Apr 22, 2019 at 6:35 PM Eric Dumazet <edumazet@google.com> wrote:
>
> We suspect some issues involving fib6_ref 0 -> 1 transitions might
> cause strange syzbot reports.
>
> Lets convert fib6_ref to refcount_t to catch them earlier.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
Acked-by: Wei Wang <weiwan@google.com>
Thanks Eric.
> include/net/ip6_fib.h | 8 ++++----
> net/ipv6/ip6_fib.c | 6 +++---
> net/ipv6/route.c | 2 +-
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index 6b7557b71c8caef7785dd0c8c52dfb92cf4e8c52..355a47bfc452aa150f537522dfb77ffa50bcb9ec 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -146,7 +146,7 @@ struct fib6_info {
> struct list_head fib6_siblings;
> unsigned int fib6_nsiblings;
>
> - atomic_t fib6_ref;
> + refcount_t fib6_ref;
> unsigned long expires;
> struct dst_metrics *fib6_metrics;
> #define fib6_pmtu fib6_metrics->metrics[RTAX_MTU-1]
> @@ -284,17 +284,17 @@ void fib6_info_destroy_rcu(struct rcu_head *head);
>
> static inline void fib6_info_hold(struct fib6_info *f6i)
> {
> - atomic_inc(&f6i->fib6_ref);
> + refcount_inc(&f6i->fib6_ref);
> }
>
> static inline bool fib6_info_hold_safe(struct fib6_info *f6i)
> {
> - return atomic_inc_not_zero(&f6i->fib6_ref);
> + return refcount_inc_not_zero(&f6i->fib6_ref);
> }
>
> static inline void fib6_info_release(struct fib6_info *f6i)
> {
> - if (f6i && atomic_dec_and_test(&f6i->fib6_ref))
> + if (f6i && refcount_dec_and_test(&f6i->fib6_ref))
> call_rcu(&f6i->rcu, fib6_info_destroy_rcu);
> }
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index a5e83593e0e45c2762eca85d04757f9d8e118e0f..a8919c217cc214821e039786492ed284552cb0b4 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -162,7 +162,7 @@ struct fib6_info *fib6_info_alloc(gfp_t gfp_flags)
> }
>
> INIT_LIST_HEAD(&f6i->fib6_siblings);
> - atomic_set(&f6i->fib6_ref, 1);
> + refcount_set(&f6i->fib6_ref, 1);
>
> return f6i;
> }
> @@ -929,7 +929,7 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
> {
> struct fib6_table *table = rt->fib6_table;
>
> - if (atomic_read(&rt->fib6_ref) != 1) {
> + if (refcount_read(&rt->fib6_ref) != 1) {
> /* This route is used as dummy address holder in some split
> * nodes. It is not leaked, but it still holds other resources,
> * which must be released in time. So, scan ascendant nodes
> @@ -2311,7 +2311,7 @@ static int ipv6_route_seq_show(struct seq_file *seq, void *v)
>
> dev = rt->fib6_nh.fib_nh_dev;
> seq_printf(seq, " %08x %08x %08x %08x %8s\n",
> - rt->fib6_metric, atomic_read(&rt->fib6_ref), 0,
> + rt->fib6_metric, refcount_read(&rt->fib6_ref), 0,
> flags, dev ? dev->name : "");
> iter->w.leaf = NULL;
> return 0;
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index e8c73b7782cdc5a2c4ca5ef50aca4c8d8556c7be..7ce14a4c3d89955d6e6468bb3cdac5efa394839b 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -296,7 +296,7 @@ static const struct fib6_info fib6_null_entry_template = {
> .fib6_flags = (RTF_REJECT | RTF_NONEXTHOP),
> .fib6_protocol = RTPROT_KERNEL,
> .fib6_metric = ~(u32)0,
> - .fib6_ref = ATOMIC_INIT(1),
> + .fib6_ref = REFCOUNT_INIT(1),
> .fib6_type = RTN_UNREACHABLE,
> .fib6_metrics = (struct dst_metrics *)&dst_default_metrics,
> };
> --
> 2.21.0.593.g511ec345e18-goog
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t
2019-04-23 1:35 ` [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t Eric Dumazet
2019-04-23 16:32 ` Wei Wang
@ 2019-04-23 16:37 ` David Ahern
1 sibling, 0 replies; 13+ messages in thread
From: David Ahern @ 2019-04-23 16:37 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet, Wei Wang
On 4/22/19 7:35 PM, Eric Dumazet wrote:
> We suspect some issues involving fib6_ref 0 -> 1 transitions might
> cause strange syzbot reports.
>
> Lets convert fib6_ref to refcount_t to catch them earlier.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Wang <weiwan@google.com>
> ---
> include/net/ip6_fib.h | 8 ++++----
> net/ipv6/ip6_fib.c | 6 +++---
> net/ipv6/route.c | 2 +-
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 0/3] ipv6: fib6_ref conversion to refcount_t
2019-04-23 1:35 [PATCH net-next 0/3] ipv6: fib6_ref conversion to refcount_t Eric Dumazet
` (2 preceding siblings ...)
2019-04-23 1:35 ` [PATCH net-next 3/3] ipv6: convert fib6_ref to refcount_t Eric Dumazet
@ 2019-04-24 0:23 ` David Miller
3 siblings, 0 replies; 13+ messages in thread
From: David Miller @ 2019-04-24 0:23 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet
From: Eric Dumazet <edumazet@google.com>
Date: Mon, 22 Apr 2019 18:35:00 -0700
> We are chasing use-after-free in IPv6 that could have their origin
> in fib6_ref 0 -> 1 transitions.
>
> This patch series should help finding the root causes if these
> illegal transitions ever happen.
Series applied, thanks Eric.
^ permalink raw reply [flat|nested] 13+ messages in thread