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

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

[Simon Horman]

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));
+		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

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

[Leon Romanovsky]

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
> 

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

[Simon Horman]

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
> > 

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

[Leon Romanovsky]

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

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

[Jakub Kicinski]

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.

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

[Leon Romanovsky]

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

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html