| Message ID | 20220216173217.3792411-1-eric.dumazet@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9fcf986cc4bc6a3a39f23fbcbbc3a9e52d3c24fd |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] ipv4: fix data races in fib_alias_hw_flags_set | expand |
On Wed, Feb 16, 2022 at 09:32:16AM -0800, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > fib_alias_hw_flags_set() can be used by concurrent threads, > and is only RCU protected. > > We need to annotate accesses to following fields of struct fib_alias: > > offload, trap, offload_failed > > Because of READ_ONCE()WRITE_ONCE() limitations, make these > field u8. [...] > Fixes: 90b93f1b31f8 ("ipv4: Add "offload" and "trap" indications to routes") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Ido Schimmel <idosch@nvidia.com> > Reported-by: syzbot <syzkaller@googlegroups.com> Reviewed-by: Ido Schimmel <idosch@nvidia.com> Thanks, this is better than taking RTNL > --- > net/ipv4/fib_lookup.h | 7 +++---- > net/ipv4/fib_semantics.c | 6 +++--- > net/ipv4/fib_trie.c | 22 +++++++++++++--------- > net/ipv4/route.c | 4 ++-- > 4 files changed, 21 insertions(+), 18 deletions(-) > > diff --git a/net/ipv4/fib_lookup.h b/net/ipv4/fib_lookup.h > index e184bcb1994343c00914e09ab728ae16c4d23dc8..78e40ea42e58d930b3439d497de2b9e15fe45706 100644 > --- a/net/ipv4/fib_lookup.h > +++ b/net/ipv4/fib_lookup.h > @@ -16,10 +16,9 @@ struct fib_alias { > u8 fa_slen; > u32 tb_id; > s16 fa_default; > - u8 offload:1, > - trap:1, > - offload_failed:1, > - unused:5; > + u8 offload; > + u8 trap; > + u8 offload_failed; There is a 5 bytes hole here, so this shouldn't increase the struct and it should still fit in one cache line > struct rcu_head rcu; > };
Hello: This series was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Wed, 16 Feb 2022 09:32:16 -0800 you wrote: > From: Eric Dumazet <edumazet@google.com> > > fib_alias_hw_flags_set() can be used by concurrent threads, > and is only RCU protected. > > We need to annotate accesses to following fields of struct fib_alias: > > [...] Here is the summary with links: - [net,1/2] ipv4: fix data races in fib_alias_hw_flags_set https://git.kernel.org/netdev/net/c/9fcf986cc4bc - [net,2/2] ipv6: fix data-race in fib6_info_hw_flags_set / fib6_purge_rt https://git.kernel.org/netdev/net/c/d95d6320ba7a You are awesome, thank you!
diff --git a/net/ipv4/fib_lookup.h b/net/ipv4/fib_lookup.h index e184bcb1994343c00914e09ab728ae16c4d23dc8..78e40ea42e58d930b3439d497de2b9e15fe45706 100644 --- a/net/ipv4/fib_lookup.h +++ b/net/ipv4/fib_lookup.h @@ -16,10 +16,9 @@ struct fib_alias { u8 fa_slen; u32 tb_id; s16 fa_default; - u8 offload:1, - trap:1, - offload_failed:1, - unused:5; + u8 offload; + u8 trap; + u8 offload_failed; struct rcu_head rcu; }; diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c index b4589861b84c6bc4daa7149e078ad63749c7622f..2dd375f7407b68b0ddd914237185671550d19b72 100644 --- a/net/ipv4/fib_semantics.c +++ b/net/ipv4/fib_semantics.c @@ -525,9 +525,9 @@ void rtmsg_fib(int event, __be32 key, struct fib_alias *fa, fri.dst_len = dst_len; fri.tos = fa->fa_tos; fri.type = fa->fa_type; - fri.offload = fa->offload; - fri.trap = fa->trap; - fri.offload_failed = fa->offload_failed; + fri.offload = READ_ONCE(fa->offload); + fri.trap = READ_ONCE(fa->trap); + fri.offload_failed = READ_ONCE(fa->offload_failed); err = fib_dump_info(skb, info->portid, seq, event, &fri, nlm_flags); if (err < 0) { /* -EMSGSIZE implies BUG in fib_nlmsg_size() */ diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c index 8060524f425667d008470c2826f2ac835c8e25d2..f7f74d5c14da663e067f4d09a620b263947bd2da 100644 --- a/net/ipv4/fib_trie.c +++ b/net/ipv4/fib_trie.c @@ -1047,19 +1047,23 @@ void fib_alias_hw_flags_set(struct net *net, const struct fib_rt_info *fri) if (!fa_match) goto out; - if (fa_match->offload == fri->offload && fa_match->trap == fri->trap && - fa_match->offload_failed == fri->offload_failed) + /* These are paired with the WRITE_ONCE() happening in this function. + * The reason is that we are only protected by RCU at this point. + */ + if (READ_ONCE(fa_match->offload) == fri->offload && + READ_ONCE(fa_match->trap) == fri->trap && + READ_ONCE(fa_match->offload_failed) == fri->offload_failed) goto out; - fa_match->offload = fri->offload; - fa_match->trap = fri->trap; + WRITE_ONCE(fa_match->offload, fri->offload); + WRITE_ONCE(fa_match->trap, fri->trap); /* 2 means send notifications only if offload_failed was changed. */ if (net->ipv4.sysctl_fib_notify_on_flag_change == 2 && - fa_match->offload_failed == fri->offload_failed) + READ_ONCE(fa_match->offload_failed) == fri->offload_failed) goto out; - fa_match->offload_failed = fri->offload_failed; + WRITE_ONCE(fa_match->offload_failed, fri->offload_failed); if (!net->ipv4.sysctl_fib_notify_on_flag_change) goto out; @@ -2297,9 +2301,9 @@ static int fn_trie_dump_leaf(struct key_vector *l, struct fib_table *tb, fri.dst_len = KEYLENGTH - fa->fa_slen; fri.tos = fa->fa_tos; fri.type = fa->fa_type; - fri.offload = fa->offload; - fri.trap = fa->trap; - fri.offload_failed = fa->offload_failed; + fri.offload = READ_ONCE(fa->offload); + fri.trap = READ_ONCE(fa->trap); + fri.offload_failed = READ_ONCE(fa->offload_failed); err = fib_dump_info(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, diff --git a/net/ipv4/route.c b/net/ipv4/route.c index ff6f91cdb6c4db7a1d5d66e08c99acb2919df2ef..f33ad1f383b684ea4a64d74da0a9951651a4be22 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -3395,8 +3395,8 @@ static int inet_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh, fa->fa_tos == fri.tos && fa->fa_info == res.fi && fa->fa_type == fri.type) { - fri.offload = fa->offload; - fri.trap = fa->trap; + fri.offload = READ_ONCE(fa->offload); + fri.trap = READ_ONCE(fa->trap); break; } }