lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 8 Apr 2020 19:51:22 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "marcelo.leitner@...il.com" <marcelo.leitner@...il.com>
CC:     "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "leon@...nel.org" <leon@...nel.org>, Roi Dayan <roid@...lanox.com>,
        Paul Blakey <paulb@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Vlad Buslov <vladbu@...lanox.com>,
        Oz Shlomo <ozsh@...lanox.com>
Subject: Re: [PATCH net] net/mlx5e: limit log messages due to (ovs) probing to
 _once

On Thu, 2020-04-02 at 23:48 -0300, marcelo.leitner@...il.com wrote:
> On Fri, Apr 03, 2020 at 02:27:08AM +0000, Saeed Mahameed wrote:
> > On Thu, 2020-04-02 at 21:11 -0300, Marcelo Ricardo Leitner wrote:
> > > OVS will keep adding such flows, no matter what. They will
> > > usually be
> > > handled by tc software (or ovs datapath, if skip_sw is used). But
> > > the
> > > driver is logging these messages for each and every attempt,
> > > despite
> > > the
> > > extack. Note that they weren't rate limited, and a broadcast
> > > storm
> > > could
> > > trigger system console flooding.
> > > 
> > > Switch these to be _once. It's enough to tell the sysadmin what
> > > is
> > > happenning, and if anything, the OVS log will have all the
> > > errors.
> > > 
> > 
> > ++ mlnx TC stake holders 
> > 
> > The fact that for all of the suppressed messages we will still have
> > NL
> > extack reporting, makes it easier for me to agree with this patch.
> > but
> > there is a loss of information since now we will stop printing the
> > attribute/params which caused the failure in most of the cases, and
> > it
> > will be harder for the user and the developer to understand why
> > these
> > attributes are not working .. 
> 
> I see.
> 
> > I understand it is for debug only but i strongly suggest to not
> > totally
> > suppress these messages and maybe just move them to tracepoints
> > buffer
> > ? for those who would want to really debug .. 
> > 
> > we already have some tracepoints implemented for en_tc.c 
> > mlx5/core/diag/en_tc_tracepoints.c, maybe we should define a
> > tracepoint
> > for error reporting .. 
> 
> That, or s/netdev_warn/netdev_dbg/, but both are more hidden to the
> user than the _once.
> 

i don't see any reason to pollute kernel log with debug messages when
we have tracepoint buffer for en_tc .. 


> > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com
> > > >
> > 
> > net patches must have a "Fixes:" tag
> 
> I know that it is strongly recommended, but there is nothing really
> broken here. It's a small cleanup to code already there. Now I'm
> confused, isn't that what net is meant for, and a Fixes tag would be
> an abuse of it here?

net is meant for bug fixes, cleanups and "next-like" content should go
to net-next. so this should be considered as an improvement and should
be submitted to net-next.

> 
> > > ---
> > >  .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 61 ++++++++++---
> > > ----
> > > --
> > >  1 file changed, 32 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > index
> > > 438128dde187d7ec58892c2879c6037f807f576f..1182fba3edbb8cf7bd59557
> > > b7ec
> > > e18765c704186 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > > @@ -1828,8 +1828,8 @@ enc_opts_is_dont_care_or_full_match(struct
> > > mlx5e_priv *priv,
> > >  				       opt->length * 4)) {
> > >  				NL_SET_ERR_MSG(extack,
> > >  					       "Partial match of tunnel
> > > options in chain > 0 isn't supported");
> > > -				netdev_warn(priv->netdev,
> > > -					    "Partial match of tunnel
> > > options in chain > 0 isn't supported");
> > > +				netdev_warn_once(priv->netdev,
> > > +						 "Partial match of
> > > tunnel options in chain > 0 isn't supported");
> > >  				return -EOPNOTSUPP;
> > >  			}
> > >  		}
> > > @@ -1988,8 +1988,8 @@ static int parse_tunnel_attr(struct
> > > mlx5e_priv
> > > *priv,
> > >  	    !mlx5_eswitch_reg_c1_loopback_enabled(esw)) {
> > >  		NL_SET_ERR_MSG(extack,
> > >  			       "Chains on tunnel devices isn't
> > > supported without register loopback support");
> > > -		netdev_warn(priv->netdev,
> > > -			    "Chains on tunnel devices isn't supported
> > > without register loopback support");
> > > +		netdev_warn_once(priv->netdev,
> > > +				 "Chains on tunnel devices isn't
> > > supported without register loopback support");
> > >  		return -EOPNOTSUPP;
> > >  	}
> > >  
> > > @@ -2133,8 +2133,8 @@ static int __parse_cls_flower(struct
> > > mlx5e_priv
> > > *priv,
> > >  	      BIT(FLOW_DISSECTOR_KEY_ENC_IP) |
> > >  	      BIT(FLOW_DISSECTOR_KEY_ENC_OPTS))) {
> > >  		NL_SET_ERR_MSG_MOD(extack, "Unsupported key");
> > > -		netdev_warn(priv->netdev, "Unsupported key used:
> > > 0x%x\n",
> > > -			    dissector->used_keys);
> > > +		netdev_warn_once(priv->netdev, "Unsupported key used:
> > > 0x%x\n",
> > > +				 dissector->used_keys);
> > >  		return -EOPNOTSUPP;
> > >  	}
> > >  
> > > @@ -2484,8 +2484,8 @@ static int parse_cls_flower(struct
> > > mlx5e_priv
> > > *priv,
> > >  		    esw->offloads.inline_mode <
> > > non_tunnel_match_level)) {
> > >  			NL_SET_ERR_MSG_MOD(extack,
> > >  					   "Flow is not offloaded due
> > > to min inline setting");
> > > -			netdev_warn(priv->netdev,
> > > -				    "Flow is not offloaded due to min
> > > inline setting, required %d actual %d\n",
> > > +			netdev_warn_once(priv->netdev,
> > > +					 "Flow is not offloaded due to
> > > min inline setting, required %d actual %d\n",
> > >  				    non_tunnel_match_level, esw-
> > > > offloads.inline_mode);
> > >  			return -EOPNOTSUPP;
> > >  		}
> > > @@ -2885,7 +2885,9 @@ static int alloc_tc_pedit_action(struct
> > > mlx5e_priv *priv, int namespace,
> > >  		if (memcmp(cmd_masks, &zero_masks, sizeof(zero_masks)))
> > > {
> > >  			NL_SET_ERR_MSG_MOD(extack,
> > >  					   "attempt to offload an
> > > unsupported field");
> > > -			netdev_warn(priv->netdev, "attempt to offload
> > > an unsupported field (cmd %d)\n", cmd);
> > > +			netdev_warn_once(priv->netdev,
> > > +					 "attempt to offload an
> > > unsupported field (cmd %d)\n",
> > > +					 cmd);
> > >  			print_hex_dump(KERN_WARNING, "mask: ",
> > > DUMP_PREFIX_ADDRESS,
> > >  				       16, 1, cmd_masks,
> > > sizeof(zero_masks), true);
> > >  			err = -EOPNOTSUPP;
> > > @@ -2912,17 +2914,17 @@ static bool csum_offload_supported(struct
> > > mlx5e_priv *priv,
> > >  	if (!(action & MLX5_FLOW_CONTEXT_ACTION_MOD_HDR)) {
> > >  		NL_SET_ERR_MSG_MOD(extack,
> > >  				   "TC csum action is only offloaded
> > > with pedit");
> > > -		netdev_warn(priv->netdev,
> > > -			    "TC csum action is only offloaded with
> > > pedit\n");
> > > +		netdev_warn_once(priv->netdev,
> > > +				 "TC csum action is only offloaded with
> > > pedit\n");
> > >  		return false;
> > >  	}
> > >  
> > >  	if (update_flags & ~prot_flags) {
> > >  		NL_SET_ERR_MSG_MOD(extack,
> > >  				   "can't offload TC csum action for
> > > some header/s");
> > > -		netdev_warn(priv->netdev,
> > > -			    "can't offload TC csum action for some
> > > header/s - flags %#x\n",
> > > -			    update_flags);
> > > +		netdev_warn_once(priv->netdev,
> > > +				 "can't offload TC csum action for some
> > > header/s - flags %#x\n",
> > > +				 update_flags);
> > >  		return false;
> > >  	}
> > >  
> > > @@ -3224,8 +3226,9 @@ static int parse_tc_nic_actions(struct
> > > mlx5e_priv *priv,
> > >  			} else {
> > >  				NL_SET_ERR_MSG_MOD(extack,
> > >  						   "device is not on
> > > same HW, can't offload");
> > > -				netdev_warn(priv->netdev, "device %s
> > > not on same HW, can't offload\n",
> > > -					    peer_dev->name);
> > > +				netdev_warn_once(priv->netdev,
> > > +						 "device %s not on same
> > > HW, can't offload\n",
> > > +						 peer_dev->name);
> > >  				return -EINVAL;
> > >  			}
> > >  			}
> > > @@ -3754,9 +3757,9 @@ static int parse_tc_fdb_actions(struct
> > > mlx5e_priv *priv,
> > >  			if (attr->out_count >=
> > > MLX5_MAX_FLOW_FWD_VPORTS) {
> > >  				NL_SET_ERR_MSG_MOD(extack,
> > >  						   "can't support more
> > > output ports, can't offload forwarding");
> > > -				netdev_warn(priv->netdev,
> > > -					    "can't support more than %d
> > > output ports, can't offload forwarding\n",
> > > -					    attr->out_count);
> > > +				netdev_warn_once(priv->netdev,
> > > +						 "can't support more
> > > than %d output ports, can't offload forwarding\n",
> > > +						 attr->out_count);
> > >  				return -EOPNOTSUPP;
> > >  			}
> > >  
> > > @@ -3821,10 +3824,10 @@ static int parse_tc_fdb_actions(struct
> > > mlx5e_priv *priv,
> > >  				if
> > > (!mlx5e_is_valid_eswitch_fwd_dev(priv, out_dev)) {
> > >  					NL_SET_ERR_MSG_MOD(extack,
> > >  							   "devices are
> > > not on same switch HW, can't offload forwarding");
> > > -					netdev_warn(priv->netdev,
> > > -						    "devices %s %s not
> > > on same switch HW, can't offload forwarding\n",
> > > -						    priv->netdev->name,
> > > -						    out_dev->name);
> > > +					netdev_warn_once(priv->netdev,
> > > +							 "devices %s %s
> > > not on same switch HW, can't offload forwarding\n",
> > > +							 priv->netdev-
> > > > name,
> > > +							 out_dev-
> > > > name);
> > >  					return -EOPNOTSUPP;
> > >  				}
> > >  
> > > @@ -3843,10 +3846,10 @@ static int parse_tc_fdb_actions(struct
> > > mlx5e_priv *priv,
> > >  			} else {
> > >  				NL_SET_ERR_MSG_MOD(extack,
> > >  						   "devices are not on
> > > same switch HW, can't offload forwarding");
> > > -				netdev_warn(priv->netdev,
> > > -					    "devices %s %s not on same
> > > switch HW, can't offload forwarding\n",
> > > -					    priv->netdev->name,
> > > -					    out_dev->name);
> > > +				netdev_warn_once(priv->netdev,
> > > +						 "devices %s %s not on
> > > same switch HW, can't offload forwarding\n",
> > > +						 priv->netdev->name,
> > > +						 out_dev->name);
> > >  				return -EINVAL;
> > >  			}
> > >  			}
> > > @@ -3959,8 +3962,8 @@ static int parse_tc_fdb_actions(struct
> > > mlx5e_priv *priv,
> > >  
> > >  			NL_SET_ERR_MSG(extack,
> > >  				       "Decap with goto isn't
> > > supported");
> > > -			netdev_warn(priv->netdev,
> > > -				    "Decap with goto isn't supported");
> > > +			netdev_warn_once(priv->netdev,
> > > +					 "Decap with goto isn't
> > > supported");
> > >  			return -EOPNOTSUPP;
> > >  		}
> > >  

Powered by blists - more mailing lists