diff mbox series

[nf-next,3/7] netfilter: nft_table_offload: Add rtnl for chain and rule operations

Message ID 1563886364-11164-4-git-send-email-wenxu@ucloud.cn
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series netfilter: nf_tables_offload: support more actions | expand

Commit Message

wenxu July 23, 2019, 12:52 p.m. UTC 
From: wenxu <wenxu@ucloud.cn>

The nft_setup_cb_call and ndo_setup_tc callback should be under rtnl lock

or it will report:
kernel: RTNL: assertion failed at
drivers/net/ethernet/mellanox/mlx5/core/en_rep.c (635)

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/netfilter/nf_tables_offload.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Pablo Neira Ayuso July 25, 2019, 9:48 a.m. UTC | #1 
On Tue, Jul 23, 2019 at 08:52:40PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> The nft_setup_cb_call and ndo_setup_tc callback should be under rtnl lock
> 
> or it will report:
> kernel: RTNL: assertion failed at
> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c (635)
> 
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  net/netfilter/nf_tables_offload.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
> index 33543f5..3e1a1a8 100644
> --- a/net/netfilter/nf_tables_offload.c
> +++ b/net/netfilter/nf_tables_offload.c
> @@ -115,14 +115,18 @@ static int nft_setup_cb_call(struct nft_base_chain *basechain,
>  			     enum tc_setup_type type, void *type_data)
>  {
>  	struct flow_block_cb *block_cb;
> -	int err;
> +	int err = 0;
>  
> +	rtnl_lock();

Please, have a look at 90d2723c6d4cb2ace50fc3b932a2bcc77710450b and
review if this assumption is correct. Probably nfnl_lock() is missing
from __nft_release_basechain().

I'd like to avoid grabbing the rtnl_lock().
Florian Westphal July 25, 2019, 10:14 a.m. UTC | #2 
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jul 23, 2019 at 08:52:40PM +0800, wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.cn>
> > 
> > The nft_setup_cb_call and ndo_setup_tc callback should be under rtnl lock
> > 
> > or it will report:
> > kernel: RTNL: assertion failed at
> > drivers/net/ethernet/mellanox/mlx5/core/en_rep.c (635)
> > 
> > Signed-off-by: wenxu <wenxu@ucloud.cn>
> > ---
> >  net/netfilter/nf_tables_offload.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
> > index 33543f5..3e1a1a8 100644
> > --- a/net/netfilter/nf_tables_offload.c
> > +++ b/net/netfilter/nf_tables_offload.c
> > @@ -115,14 +115,18 @@ static int nft_setup_cb_call(struct nft_base_chain *basechain,
> >  			     enum tc_setup_type type, void *type_data)
> >  {
> >  	struct flow_block_cb *block_cb;
> > -	int err;
> > +	int err = 0;
> >  
> > +	rtnl_lock();
> 
> Please, have a look at 90d2723c6d4cb2ace50fc3b932a2bcc77710450b and
> review if this assumption is correct. Probably nfnl_lock() is missing
> from __nft_release_basechain().

The mlx driver has a ASSERT_RTNL() in the mlx5e_rep_indr_setup_tc_block()
callpath.  Or are you proposing to remove that assertion?  If so, what
lock should protect the callback lists?
wenxu July 25, 2019, 10:42 a.m. UTC | #3 
On 7/25/2019 6:14 PM, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> On Tue, Jul 23, 2019 at 08:52:40PM +0800, wenxu@ucloud.cn wrote:
>>> From: wenxu <wenxu@ucloud.cn>
>>>
>>> The nft_setup_cb_call and ndo_setup_tc callback should be under rtnl lock
>>>
>>> or it will report:
>>> kernel: RTNL: assertion failed at
>>> drivers/net/ethernet/mellanox/mlx5/core/en_rep.c (635)
>>>
>>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>>> ---
>>>  net/netfilter/nf_tables_offload.c | 16 ++++++++++++----
>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
>>> index 33543f5..3e1a1a8 100644
>>> --- a/net/netfilter/nf_tables_offload.c
>>> +++ b/net/netfilter/nf_tables_offload.c
>>> @@ -115,14 +115,18 @@ static int nft_setup_cb_call(struct nft_base_chain *basechain,
>>>  			     enum tc_setup_type type, void *type_data)
>>>  {
>>>  	struct flow_block_cb *block_cb;
>>> -	int err;
>>> +	int err = 0;
>>>  
>>> +	rtnl_lock();
>> Please, have a look at 90d2723c6d4cb2ace50fc3b932a2bcc77710450b and
>> review if this assumption is correct. Probably nfnl_lock() is missing
>> from __nft_release_basechain().
> The mlx driver has a ASSERT_RTNL() in the mlx5e_rep_indr_setup_tc_block()
> callpath.  Or are you proposing to remove that assertion?  If so, what
> lock should protect the callback lists?
yes, most of the setup_tc callback in mlx driver has a 

ASSERT_RTNL() directly or indirectly. Maybe remove this is a good idear

>
>
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 33543f5..3e1a1a8 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -115,14 +115,18 @@  static int nft_setup_cb_call(struct nft_base_chain *basechain,
 			     enum tc_setup_type type, void *type_data)
 {
 	struct flow_block_cb *block_cb;
-	int err;
+	int err = 0;
 
+	rtnl_lock();
 	list_for_each_entry(block_cb, &basechain->flow_block.cb_list, list) {
 		err = block_cb->cb(type, type_data, block_cb->cb_priv);
 		if (err < 0)
-			return err;
+			goto out;
 	}
-	return 0;
+
+out:
+	rtnl_unlock();
+	return err;
 }
 
 static int nft_flow_offload_rule(struct nft_trans *trans,
@@ -204,9 +208,11 @@  static int nft_flow_offload_chain(struct nft_trans *trans,
 	bo.extack = &extack;
 	INIT_LIST_HEAD(&bo.cb_list);
 
+	rtnl_lock();
+
 	err = dev->netdev_ops->ndo_setup_tc(dev, FLOW_SETUP_BLOCK, &bo);
 	if (err < 0)
-		return err;
+		goto out;
 
 	switch (cmd) {
 	case FLOW_BLOCK_BIND:
@@ -217,6 +223,8 @@  static int nft_flow_offload_chain(struct nft_trans *trans,
 		break;
 	}
 
+out:
+	rtnl_unlock();
 	return err;
 }