netdev - Re: [PATCH net-next v3 4/4] net/mlx5e: Return -EOPNOTSUPP when attempting to offload an unsupported action

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMDZJNXzuR+eG26QL2UxhhtdjqOj+GoZwF1Mff-Q_NwLKMB=mQ@mail.gmail.com>
Date:   Thu, 28 Feb 2019 18:22:10 +0800
From:   Tonghao Zhang <xiangxia.m.yue@...il.com>
To:     Roi Dayan <roid@...lanox.com>
Cc:     Saeed Mahameed <saeedm@...lanox.com>,
        "gerlitz.or@...il.com" <gerlitz.or@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 4/4] net/mlx5e: Return -EOPNOTSUPP when
 attempting to offload an unsupported action

On Wed, Feb 27, 2019 at 10:01 PM Roi Dayan <roid@...lanox.com> wrote:
>
>
>
> On 26/02/2019 14:27, xiangxia.m.yue@...il.com wrote:
> > From: Tonghao Zhang <xiangxia.m.yue@...il.com>
> >
> > * Now the encapsulation is not supported for mlx5 VFs. When we try to
> > offload that action, the -EINVAL is returned, but not -EOPNOTSUPP.
> > This patch changes the returned value and ignore to confuse user.
> > The command is shown as below [1].
> >
> > * When max modify header action is zero, we return -EOPNOTSUPP
> > directly. In this way, we can ignore wrong message info (e.g.
> > "mlx5: parsed 0 pedit actions, can't do more"). This happens when
> > offloading pedit actions on mlx(cx4) VFs. The command is shown as below [2].
> >
> > For example: (p2p1_0 is VF net device)
> > [1]
> > $ tc filter add dev p2p1_0 protocol ip  parent ffff: prio 1 flower skip_sw \
> >     src_mac e4:11:22:33:44:01    \
> >     action tunnel_key set        \
> >     src_ip 1.1.1.100        \
> >     dst_ip 1.1.1.200        \
> >     dst_port 4789 id 100        \
> >     action mirred egress redirect dev vxlan0
> >
> > [2]
> > $ tc filter add dev p2p1_0 parent ffff: protocol ip prio 1 \
> >     flower skip_sw dst_mac 00:10:56:fb:64:e8 \
> >     dst_ip 1.1.1.100 src_ip 1.1.1.200 \
> >     action pedit ex munge eth src set 00:10:56:b4:5d:20
> >
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>
> > ---
> >  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index 56ac50d..3a02b22 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -2035,7 +2035,7 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
> >                                struct netlink_ext_ack *extack)
> >  {
> >       u8 cmd = (act->id == FLOW_ACTION_MANGLE) ? 0 : 1;
> > -     int err = -EOPNOTSUPP;
> > +     int max_actions, err = -EOPNOTSUPP;
> >       u32 mask, val, offset;
> >       u8 htype;
> >
> > @@ -2047,6 +2047,17 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
> >               goto out_err;
> >       }
> >
> > +        if (namespace == MLX5_FLOW_NAMESPACE_FDB) /* FDB offloading */
> > +                max_actions = MLX5_CAP_ESW_FLOWTABLE_FDB(priv->mdev, max_modify_header_actions);
> > +        else /* namespace is MLX5_FLOW_NAMESPACE_KERNEL - NIC offloading */
> > +                max_actions = MLX5_CAP_FLOWTABLE_NIC_RX(priv->mdev, max_modify_header_actions);
> > +
>
> we have the exact same block with comments in alloc_mod_hdr_actions() which
> is called later after we parse the action and now parsing the pedit fields.
> your check for max_actions was there in prev v. why move it?
I think we should check it as soon as possible.
> if like this I suggest we create a small static function to get max actions
> for a namespace and call it from both places to get max actions.
it's ok.
> > +     if (!max_actions) {
> > +             NL_SET_ERR_MSG_MOD(extack,
> > +                                "don't support pedit actions, can't offload");
>
> please rephrase the msg to be consistent
> i.e. The pedit offload action is not supported
>
> > +             goto out_err;
> > +     }
> > +
> >       mask = act->mangle.mask;
> >       val = act->mangle.val;
> >       offset = act->mangle.offset;
> > @@ -2294,7 +2305,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> >                       }
> >                       break;
> >               default:
> > -                     return -EINVAL;
> > +                     NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> > +                     return -EOPNOTSUPP;
> >               }
> >       }
> >
> > @@ -2616,7 +2628,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> >                       break;
> >                       }
> >               default:
> > -                     return -EINVAL;
> > +                     NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> > +                     return -EOPNOTSUPP;
> >               }
> >       }
> >
> >