> Ziyang Xuan <william.xuanziyang@xxxxxxxxxx> 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() with rcu_read_lock() to Iterate over
>> nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it.
>
> I don't think this resolves the described race.
>
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx>
>> ---
>> net/netfilter/nf_tables_api.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index fd86f2720c9e..fbf38e32f11d 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -8297,10 +8297,14 @@ 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) {
>> - if (family == type->family)
>> + rcu_read_lock()
>> + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) {
>> + if (family == type->family) {
>> + rcu_read_unlock();
>> return type;
>
> This means 'type' can be non-null while module is being unloaded,
> before refcount increment.
>
> You need acquire rcu_read_lock() in the caller, nft_flowtable_type_get,
> and release it after refcount on module owner failed or succeeded.
> .
In fact, I just want to resolve the potential tear-down problem about list entry here.
As following:
void nft_unregister_flowtable_type(struct nf_flowtable_type *type)
{
nfnl_lock(NFNL_SUBSYS_NFTABLES);
/* just use WRITE_ONCE() inside, but not rcu_assign_pointer().
* It can not form competition protection with rcu_read_lock().
*/
list_del_rcu(&type->list);
nfnl_unlock(NFNL_SUBSYS_NFTABLES);
}
static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family)
{
const struct nf_flowtable_type *type;
/* Get list entry use READ_ONCE() inside. And I think rcu_read_lock() maybe unnecessary. */
list_for_each_entry_rcu(type, &nf_tables_flowtables, list) {
if (family == type->family)
return type;
}
return NULL;
}
static const struct nf_flowtable_type *
nft_flowtable_type_get(struct net *net, u8 family)
{
const struct nf_flowtable_type *type;
type = __nft_flowtable_type_get(family);
/* If type is non-NULL, try to get module. If the module
* state is not ok, it will fail here.
*/
if (type != NULL && try_module_get(type->owner))
return type;
lockdep_nfnl_nft_mutex_not_held();
#ifdef CONFIG_MODULES
if (type == NULL) {
if (nft_request_module(net, "nf-flowtable-%u", family) == -EAGAIN)
return ERR_PTR(-EAGAIN);
}
#endif
return ERR_PTR(-ENOENT);
}
So I think replace with list_for_each_entry_rcu() can resolve the tear-down problem now.
William Xuan
Best regards.
