linux-kernel - Re: [PATCH 5.15 049/230] net/mlx5e: Check action fwd/drop flag exists also for nic flows

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y8CEv90mCZkmuFAq@xps13.dannf>
Date:   Thu, 12 Jan 2023 15:07:59 -0700
From:   dann frazier <dann.frazier@...onical.com>
To:     Sasha Levin <sashal@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Roi Dayan <roid@...dia.com>, Maor Dickman <maord@...dia.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Ran Drori <rdrori@...dia.com>,
        Frode Nordahl <frode.nordahl@...onical.com>
Subject: Re: [PATCH 5.15 049/230] net/mlx5e: Check action fwd/drop flag
 exists also for nic flows

On Mon, Jul 11, 2022 at 11:05:05AM +0200, Greg Kroah-Hartman wrote:
> From: Roi Dayan <roid@...dia.com>
> 
> [ Upstream commit 6b50cf45b6a0e99f3cab848a72ecca8da56b7460 ]
> 
> The driver should add offloaded rules with either a fwd or drop action.
> The check existed in parsing fdb flows but not when parsing nic flows.
> Move the test into actions_match_supported() which is called for
> checking nic flows and fdb flows.
> 
> Signed-off-by: Roi Dayan <roid@...dia.com>
> Reviewed-by: Maor Dickman <maord@...dia.com>
> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> Signed-off-by: Sasha Levin <sashal@...nel.org>

hey Sasha,

  A contact at Nvidia tells me that this has caused a regression w/
OVN HW offload. To fix that, commit 7f8770c7 ("net/mlx5e: Set action
fwd flag when parsing tc action goto") is also required.

 I'm not really sure what flagged this patch for stable, so I don't
know whether to suggest it be reverted, or that additonal patch be
applied. Roi - what's your thought?

  -dann

> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 3aa8d0b83d10..fe52db591121 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -3305,6 +3305,12 @@ static bool actions_match_supported(struct mlx5e_priv *priv,
>  	ct_flow = flow_flag_test(flow, CT) && !ct_clear;
>  	actions = flow->attr->action;
>  
> +	if (!(actions &
> +	      (MLX5_FLOW_CONTEXT_ACTION_FWD_DEST | MLX5_FLOW_CONTEXT_ACTION_DROP))) {
> +		NL_SET_ERR_MSG_MOD(extack, "Rule must have at least one forward/drop action");
> +		return false;
> +	}
> +
>  	if (mlx5e_is_eswitch_flow(flow)) {
>  		if (flow->attr->esw_attr->split_count && ct_flow &&
>  		    !MLX5_CAP_GEN(flow->attr->esw_attr->in_mdev, reg_c_preserve)) {
> @@ -4207,13 +4213,6 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
>  		attr->action |= MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
>  	}
>  
> -	if (!(attr->action &
> -	      (MLX5_FLOW_CONTEXT_ACTION_FWD_DEST | MLX5_FLOW_CONTEXT_ACTION_DROP))) {
> -		NL_SET_ERR_MSG_MOD(extack,
> -				   "Rule must have at least one forward/drop action");
> -		return -EOPNOTSUPP;
> -	}
> -
>  	if (esw_attr->split_count > 0 && !mlx5_esw_has_fwd_fdb(priv->mdev)) {
>  		NL_SET_ERR_MSG_MOD(extack,
>  				   "current firmware doesn't support split rule for port mirroring");