| 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 linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200403024835.GA3547@localhost.localdomain>
Date: Thu, 2 Apr 2020 23:48:35 -0300
From: "marcelo.leitner@...il.com" <marcelo.leitner@...il.com>
To: Saeed Mahameed <saeedm@...lanox.com>
Cc: 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>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
"leon@...nel.org" <leon@...nel.org>
Subject: Re: [PATCH net] net/mlx5e: limit log messages due to (ovs) probing
to _once
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.
>
> > 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/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..1182fba3edbb8cf7bd59557b7ec
> > 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