[PATCH nf-next] netfilter: nf_table_offload: Fix the incorrect rcu usage in nft_indr_block_get_and_ing_cmd

[PATCH nf-next] netfilter: nf_table_offload: Fix the incorrect rcu usage in nft_indr_block_get_and_ing_cmd

[wenxu]

From: wenxu <wenxu@ucloud.cn>

The nft_indr_block_get_and_ing_cmd is called in netdevice notify
It is the incorrect rcu case, To fix it just traverse the list under
the commit mutex.

Fixes: 9a32669fecfb ("netfilter: nf_tables_offload: support indr block call")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/netfilter/nf_tables_offload.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index b95e27b..bcaafc8 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -359,15 +359,18 @@ void nft_indr_block_get_and_ing_cmd(struct net_device *dev,
 				    void *cb_priv,
 				    enum flow_block_command command)
 {
-	struct net *net = dev_net(dev);
+	const struct nft_chain *chain, *nr;
 	const struct nft_table *table;
-	const struct nft_chain *chain;
+	struct nft_ctx ctx = {
+		.net	= dev_net(dev),
+	};
 
-	list_for_each_entry_rcu(table, &net->nft.tables, list) {
+	mutex_lock(&ctx.net->nft.commit_mutex);
+	list_for_each_entry(table, &ctx.net->nft.tables, list) {
 		if (table->family != NFPROTO_NETDEV)
 			continue;
 
-		list_for_each_entry_rcu(chain, &table->chains, list) {
+		list_for_each_entry_safe(chain, nr, &table->chains, list) {
 			if (nft_is_base_chain(chain)) {
 				struct nft_base_chain *basechain;
 
@@ -382,4 +385,5 @@ void nft_indr_block_get_and_ing_cmd(struct net_device *dev,
 			}
 		}
 	}
+	mutex_unlock(&ctx.net->nft.commit_mutex);
 }
-- 
1.8.3.1

Re: [PATCH nf-next] netfilter: nf_table_offload: Fix the incorrect rcu usage in nft_indr_block_get_and_ing_cmd

[Florian Westphal]

wenxu@ucloud.cn <wenxu@ucloud.cn> wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> The nft_indr_block_get_and_ing_cmd is called in netdevice notify
> It is the incorrect rcu case, To fix it just traverse the list under
> the commit mutex.

What is an 'incorrect rcu case'?

Please clarify, e.g. by including rcu warning/splat backtrace here.

> +	struct nft_ctx ctx = {
> +		.net	= dev_net(dev),
> +	};

Why is this ctx needed?

> +	mutex_lock(&ctx.net->nft.commit_mutex);

net->nft.commit_mutex?

> -		list_for_each_entry_rcu(chain, &table->chains, list) {
> +		list_for_each_entry_safe(chain, nr, &table->chains, list) {

Why is _safe needed rather than list_for_each_entry()?

Re: [PATCH nf-next] netfilter: nf_table_offload: Fix the incorrect rcu usage in nft_indr_block_get_and_ing_cmd

[wenxu]

On 8/19/2019 6:21 PM, Florian Westphal wrote:
> wenxu@ucloud.cn <wenxu@ucloud.cn> wrote:
>> From: wenxu <wenxu@ucloud.cn>
>>
>> The nft_indr_block_get_and_ing_cmd is called in netdevice notify
>> It is the incorrect rcu case, To fix it just traverse the list under
>> the commit mutex.
> What is an 'incorrect rcu case'?
>
> Please clarify, e.g. by including rcu warning/splat backtrace here.

according to http://patchwork.ozlabs.org/patch/1148283/

flow_block_ing_cmd() needs to call blocking functions while iterating block_ing_cb_list,

nft_indr_block_get_and_ing_cmd is in the cb_list, So it should also not in rcu for blocking

cases.

>
>> +	struct nft_ctx ctx = {
>> +		.net	= dev_net(dev),
>> +	};
> Why is this ctx needed?
>
>> +	mutex_lock(&ctx.net->nft.commit_mutex);
> net->nft.commit_mutex?

When traverse the list, the list is protected under commit_mutex like nf_tables_netdev_event

do in the netdevice notify callback

>
>> -		list_for_each_entry_rcu(chain, &table->chains, list) {
>> +		list_for_each_entry_safe(chain, nr, &table->chains, list) {
> Why is _safe needed rather than list_for_each_entry()?
yes list_for_each_entry() is better
>

Re: [PATCH nf-next] netfilter: nf_table_offload: Fix the incorrect rcu usage in nft_indr_block_get_and_ing_cmd

[Florian Westphal]

wenxu <wenxu@ucloud.cn> wrote:
> >> The nft_indr_block_get_and_ing_cmd is called in netdevice notify
> >> It is the incorrect rcu case, To fix it just traverse the list under
> >> the commit mutex.
> > What is an 'incorrect rcu case'?
> >
> > Please clarify, e.g. by including rcu warning/splat backtrace here.

[..]

> flow_block_ing_cmd() needs to call blocking functions while iterating block_ing_cb_list,
> nft_indr_block_get_and_ing_cmd is in the cb_list, So it should also not in rcu for blocking
> cases.

Please submit a v2 that includes this explanation in the commit message.

> >> +	struct nft_ctx ctx = {
> >> +		.net	= dev_net(dev),
> >> +	};
> > Why is this ctx needed?
> >
> >> +	mutex_lock(&ctx.net->nft.commit_mutex);
> > net->nft.commit_mutex?
> 
> When traverse the list, the list is protected under commit_mutex like nf_tables_netdev_event
> do in the netdevice notify callback

Yes, I see that, but why do you need nft_ctx ctx?  Its confusing.

Just use

mutex_lock(&net->nft.commit_mutex);

without adding this 'ctx'.