| Message ID | 20240403072204.2139712-1-william.xuanziyang@huawei.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [nft,v2] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get() | expand |
Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can > concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable(). > And thhere is not any protection when iterate over nf_tables_flowtables > list in __nft_flowtable_type_get(). Therefore, there is pertential > data-race of nf_tables_flowtables list entry. > > Use list_for_each_entry_rcu() to iterate over nf_tables_flowtables list > in __nft_flowtable_type_get(), and use rcu_read_lock() in the caller > nft_flowtable_type_get() to protect the entire type query process. Reviewed-by: Florian Westphal <fw@strlen.de> Would you be so kind to send followup patches for the other two types Pablo pointed out? static LIST_HEAD(nf_tables_expressions); static LIST_HEAD(nf_tables_objects); It looks like they have same issue. Thanks!
> Ziyang Xuan <william.xuanziyang@huawei.com> wrote: >> nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can >> concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable(). >> And thhere is not any protection when iterate over nf_tables_flowtables >> list in __nft_flowtable_type_get(). Therefore, there is pertential >> data-race of nf_tables_flowtables list entry. >> >> Use list_for_each_entry_rcu() to iterate over nf_tables_flowtables list >> in __nft_flowtable_type_get(), and use rcu_read_lock() in the caller >> nft_flowtable_type_get() to protect the entire type query process. > > Reviewed-by: Florian Westphal <fw@strlen.de> > > Would you be so kind to send followup patches for the other two types > Pablo pointed out? > > static LIST_HEAD(nf_tables_expressions); > static LIST_HEAD(nf_tables_objects); > > It looks like they have same issue. Yes, I am doing and testing. Best regards. > > Thanks! > > > . >
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index fd86f2720c9e..47e1a22e8fb1 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -8293,11 +8293,12 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx, return err; } +/* call under rcu_read_lock */ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family) { const struct nf_flowtable_type *type; - list_for_each_entry(type, &nf_tables_flowtables, list) { + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) { if (family == type->family) return type; } @@ -8309,9 +8310,13 @@ nft_flowtable_type_get(struct net *net, u8 family) { const struct nf_flowtable_type *type; + rcu_read_lock(); type = __nft_flowtable_type_get(family); - if (type != NULL && try_module_get(type->owner)) + if (type != NULL && try_module_get(type->owner)) { + rcu_read_unlock(); return type; + } + rcu_read_unlock(); lockdep_nfnl_nft_mutex_not_held(); #ifdef CONFIG_MODULES
nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable(). And thhere is not any protection when iterate over nf_tables_flowtables list in __nft_flowtable_type_get(). Therefore, there is pertential data-race of nf_tables_flowtables list entry. Use list_for_each_entry_rcu() to iterate over nf_tables_flowtables list in __nft_flowtable_type_get(), and use rcu_read_lock() in the caller nft_flowtable_type_get() to protect the entire type query process. Fixes: 3b49e2e94e6e ("netfilter: nf_tables: add flow table netlink frontend") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- v2: - Move rcu_read_lock() outside to caller. [Florian Westphal] - Add "Fixes" tag. --- net/netfilter/nf_tables_api.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)