diff mbox series

[net] nfp: flower: avoid taking mutex in atomic context

Message ID 20230131080313.2076060-1-simon.horman@corigine.com (mailing list archive)
State Accepted
Commit 9c6b9cbafdc010b38f4077c8252654381eb46028
Delegated to: Netdev Maintainers
Headers show
Series [net] nfp: flower: avoid taking mutex in atomic context | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 669 this patch: 669
netdev/cc_maintainers fail 1 blamed authors not CCed: louis.peens@corigine.com; 3 maintainers not CCed: louis.peens@corigine.com edumazet@google.com yinjun.zhang@corigine.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Simon Horman Jan. 31, 2023, 8:03 a.m. UTC 
From: Yanguo Li <yanguo.li@corigine.com>

A mutex may sleep, which is not permitted in atomic context.
Avoid a case where this may arise by moving the to
nfp_flower_lag_get_info_from_netdev() in nfp_tun_write_neigh() spinlock.

Fixes: abc210952af7 ("nfp: flower: tunnel neigh support bond offload")
Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Yanguo Li <yanguo.li@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Leon Romanovsky Jan. 31, 2023, 11:45 a.m. UTC | #1 
On Tue, Jan 31, 2023 at 09:03:13AM +0100, Simon Horman wrote:
> From: Yanguo Li <yanguo.li@corigine.com>
> 
> A mutex may sleep, which is not permitted in atomic context.
> Avoid a case where this may arise by moving the to
> nfp_flower_lag_get_info_from_netdev() in nfp_tun_write_neigh() spinlock.
> 
> Fixes: abc210952af7 ("nfp: flower: tunnel neigh support bond offload")
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Yanguo Li <yanguo.li@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> ---
>  drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> index a8678d5612ee..060a77f2265d 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> @@ -460,6 +460,7 @@ nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
>  			    sizeof(struct nfp_tun_neigh_v4);
>  	unsigned long cookie = (unsigned long)neigh;
>  	struct nfp_flower_priv *priv = app->priv;
> +	struct nfp_tun_neigh_lag lag_info;
>  	struct nfp_neigh_entry *nn_entry;
>  	u32 port_id;
>  	u8 mtype;
> @@ -468,6 +469,11 @@ nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
>  	if (!port_id)
>  		return;
>  
> +	if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT) {
> +		memset(&lag_info, 0, sizeof(struct nfp_tun_neigh_lag));

This memset can be removed if you initialize lag_info to zero.
struct nfp_tun_neigh_lag lag_info = {};

Thanks

> +		nfp_flower_lag_get_info_from_netdev(app, netdev, &lag_info);
> +	}
> +
>  	spin_lock_bh(&priv->predt_lock);
>  	nn_entry = rhashtable_lookup_fast(&priv->neigh_table, &cookie,
>  					  neigh_table_params);
> @@ -515,7 +521,7 @@ nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
>  		neigh_ha_snapshot(common->dst_addr, neigh, netdev);
>  
>  		if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT)
> -			nfp_flower_lag_get_info_from_netdev(app, netdev, lag);
> +			memcpy(lag, &lag_info, sizeof(struct nfp_tun_neigh_lag));
>  		common->port_id = cpu_to_be32(port_id);
>  
>  		if (rhashtable_insert_fast(&priv->neigh_table,
> -- 
> 2.30.2
>
Simon Horman Jan. 31, 2023, 12:15 p.m. UTC | #2 
On Tue, Jan 31, 2023 at 01:45:10PM +0200, Leon Romanovsky wrote:
> On Tue, Jan 31, 2023 at 09:03:13AM +0100, Simon Horman wrote:
> > From: Yanguo Li <yanguo.li@corigine.com>
> > 
> > A mutex may sleep, which is not permitted in atomic context.
> > Avoid a case where this may arise by moving the to
> > nfp_flower_lag_get_info_from_netdev() in nfp_tun_write_neigh() spinlock.
> > 
> > Fixes: abc210952af7 ("nfp: flower: tunnel neigh support bond offload")
> > Reported-by: Dan Carpenter <error27@gmail.com>
> > Signed-off-by: Yanguo Li <yanguo.li@corigine.com>
> > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > ---
> >  drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> > index a8678d5612ee..060a77f2265d 100644
> > --- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> > +++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> > @@ -460,6 +460,7 @@ nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
> >  			    sizeof(struct nfp_tun_neigh_v4);
> >  	unsigned long cookie = (unsigned long)neigh;
> >  	struct nfp_flower_priv *priv = app->priv;
> > +	struct nfp_tun_neigh_lag lag_info;
> >  	struct nfp_neigh_entry *nn_entry;
> >  	u32 port_id;
> >  	u8 mtype;
> > @@ -468,6 +469,11 @@ nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
> >  	if (!port_id)
> >  		return;
> >  
> > +	if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT) {
> > +		memset(&lag_info, 0, sizeof(struct nfp_tun_neigh_lag));
> 
> This memset can be removed if you initialize lag_info to zero.
> struct nfp_tun_neigh_lag lag_info = {};

Happy to change if that is preferred.
Is it preferred?

> Thanks
> 
> > +		nfp_flower_lag_get_info_from_netdev(app, netdev, &lag_info);
> > +	}
> > +
> >  	spin_lock_bh(&priv->predt_lock);
> >  	nn_entry = rhashtable_lookup_fast(&priv->neigh_table, &cookie,
> >  					  neigh_table_params);
> > @@ -515,7 +521,7 @@ nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
> >  		neigh_ha_snapshot(common->dst_addr, neigh, netdev);
> >  
> >  		if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT)
> > -			nfp_flower_lag_get_info_from_netdev(app, netdev, lag);
> > +			memcpy(lag, &lag_info, sizeof(struct nfp_tun_neigh_lag));
> >  		common->port_id = cpu_to_be32(port_id);
> >  
> >  		if (rhashtable_insert_fast(&priv->neigh_table,
> > -- 
> > 2.30.2
> >
Leon Romanovsky Jan. 31, 2023, 1:27 p.m. UTC | #3 
On Tue, Jan 31, 2023 at 01:15:46PM +0100, Simon Horman wrote:
> On Tue, Jan 31, 2023 at 01:45:10PM +0200, Leon Romanovsky wrote:
> > On Tue, Jan 31, 2023 at 09:03:13AM +0100, Simon Horman wrote:
> > > From: Yanguo Li <yanguo.li@corigine.com>
> > > 
> > > A mutex may sleep, which is not permitted in atomic context.
> > > Avoid a case where this may arise by moving the to
> > > nfp_flower_lag_get_info_from_netdev() in nfp_tun_write_neigh() spinlock.
> > > 
> > > Fixes: abc210952af7 ("nfp: flower: tunnel neigh support bond offload")
> > > Reported-by: Dan Carpenter <error27@gmail.com>
> > > Signed-off-by: Yanguo Li <yanguo.li@corigine.com>
> > > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> > > ---
> > >  drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> > > index a8678d5612ee..060a77f2265d 100644
> > > --- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> > > +++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
> > > @@ -460,6 +460,7 @@ nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
> > >  			    sizeof(struct nfp_tun_neigh_v4);
> > >  	unsigned long cookie = (unsigned long)neigh;
> > >  	struct nfp_flower_priv *priv = app->priv;
> > > +	struct nfp_tun_neigh_lag lag_info;
> > >  	struct nfp_neigh_entry *nn_entry;
> > >  	u32 port_id;
> > >  	u8 mtype;
> > > @@ -468,6 +469,11 @@ nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
> > >  	if (!port_id)
> > >  		return;
> > >  
> > > +	if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT) {
> > > +		memset(&lag_info, 0, sizeof(struct nfp_tun_neigh_lag));
> > 
> > This memset can be removed if you initialize lag_info to zero.
> > struct nfp_tun_neigh_lag lag_info = {};
> 
> Happy to change if that is preferred.
> Is it preferred?

I don't see why it can't be preferred.

Thanks
Jakub Kicinski Jan. 31, 2023, 9:21 p.m. UTC | #4 
On Tue, 31 Jan 2023 15:27:51 +0200 Leon Romanovsky wrote:
> > > > +	if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT) {
> > > > +		memset(&lag_info, 0, sizeof(struct nfp_tun_neigh_lag));  
> > > 
> > > This memset can be removed if you initialize lag_info to zero.
> > > struct nfp_tun_neigh_lag lag_info = {};  
> > 
> > Happy to change if that is preferred.
> > Is it preferred?  
> 
> I don't see why it can't be preferred.

It's too subjective to make Simon respin, IMO.
Leon Romanovsky Feb. 1, 2023, 8:21 a.m. UTC | #5 
On Tue, Jan 31, 2023 at 01:21:29PM -0800, Jakub Kicinski wrote:
> On Tue, 31 Jan 2023 15:27:51 +0200 Leon Romanovsky wrote:
> > > > > +	if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT) {
> > > > > +		memset(&lag_info, 0, sizeof(struct nfp_tun_neigh_lag));  
> > > > 
> > > > This memset can be removed if you initialize lag_info to zero.
> > > > struct nfp_tun_neigh_lag lag_info = {};  
> > > 
> > > Happy to change if that is preferred.
> > > Is it preferred?  
> > 
> > I don't see why it can't be preferred.
> 
> It's too subjective to make Simon respin, IMO.

I'm not insisting on respin, but would like to hear why writing compact
code with cleared variable on stack, which anyway needs to be cleared is
not preferred.

Thanks
patchwork-bot+netdevbpf@kernel.org Feb. 2, 2023, 4 a.m. UTC | #6 
Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 31 Jan 2023 09:03:13 +0100 you wrote:
> From: Yanguo Li <yanguo.li@corigine.com>
> 
> A mutex may sleep, which is not permitted in atomic context.
> Avoid a case where this may arise by moving the to
> nfp_flower_lag_get_info_from_netdev() in nfp_tun_write_neigh() spinlock.
> 
> Fixes: abc210952af7 ("nfp: flower: tunnel neigh support bond offload")
> Reported-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Yanguo Li <yanguo.li@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> 
> [...]

Here is the summary with links:
  - [net] nfp: flower: avoid taking mutex in atomic context
    https://git.kernel.org/netdev/net/c/9c6b9cbafdc0

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
index a8678d5612ee..060a77f2265d 100644
--- a/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
+++ b/drivers/net/ethernet/netronome/nfp/flower/tunnel_conf.c
@@ -460,6 +460,7 @@  nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
 			    sizeof(struct nfp_tun_neigh_v4);
 	unsigned long cookie = (unsigned long)neigh;
 	struct nfp_flower_priv *priv = app->priv;
+	struct nfp_tun_neigh_lag lag_info;
 	struct nfp_neigh_entry *nn_entry;
 	u32 port_id;
 	u8 mtype;
@@ -468,6 +469,11 @@  nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
 	if (!port_id)
 		return;
 
+	if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT) {
+		memset(&lag_info, 0, sizeof(struct nfp_tun_neigh_lag));
+		nfp_flower_lag_get_info_from_netdev(app, netdev, &lag_info);
+	}
+
 	spin_lock_bh(&priv->predt_lock);
 	nn_entry = rhashtable_lookup_fast(&priv->neigh_table, &cookie,
 					  neigh_table_params);
@@ -515,7 +521,7 @@  nfp_tun_write_neigh(struct net_device *netdev, struct nfp_app *app,
 		neigh_ha_snapshot(common->dst_addr, neigh, netdev);
 
 		if ((port_id & NFP_FL_LAG_OUT) == NFP_FL_LAG_OUT)
-			nfp_flower_lag_get_info_from_netdev(app, netdev, lag);
+			memcpy(lag, &lag_info, sizeof(struct nfp_tun_neigh_lag));
 		common->port_id = cpu_to_be32(port_id);
 
 		if (rhashtable_insert_fast(&priv->neigh_table,