netdev - Re: [net-next 05/15] net/mlx5: fs, move hardware fte deletion function reset

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: <ec5bbfc5-84d3-4eda-977f-64483a7449d2@intel.com>
Date: Thu, 12 Sep 2024 12:01:00 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Saeed Mahameed <saeed@...nel.org>, "David S. Miller"
	<davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>
CC: Saeed Mahameed <saeedm@...dia.com>, <netdev@...r.kernel.org>, Tariq Toukan
	<tariqt@...dia.com>, Gal Pressman <gal@...dia.com>, Leon Romanovsky
	<leonro@...dia.com>, Mark Bloch <mbloch@...dia.com>
Subject: Re: [net-next 05/15] net/mlx5: fs, move hardware fte deletion
 function reset



On 9/11/2024 1:17 PM, Saeed Mahameed wrote:
> From: Mark Bloch <mbloch@...dia.com>
> 
> Downstream patches will need this as we might not want to reset
> it when a pending rule is connected to the FTE.
> 

At first I didn't quite understand the motivation here but...

> Signed-off-by: Mark Bloch <mbloch@...dia.com>
> Signed-off-by: Saeed Mahameed <saeedm@...dia.com>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/fs_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> index e32725487702..899d91577a54 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> @@ -683,6 +683,8 @@ static void del_hw_fte(struct fs_node *node)
>  				       fte->index, fg->id);
>  		node->active = false;
>  	}
> +	/* Avoid double call to del_hw_fte */
> +	fte->node.del_hw_func = NULL;
>  }
>  
>  static void del_sw_fte(struct fs_node *node)
> @@ -2265,8 +2267,6 @@ void mlx5_del_flow_rules(struct mlx5_flow_handle *handle)
>  		tree_remove_node(&handle->rule[i]->node, true);
>  	if (list_empty(&fte->node.children)) {
>  		fte->node.del_hw_func(&fte->node);
> -		/* Avoid double call to del_hw_fte */
> -		fte->node.del_hw_func = NULL;

I see. You were previously clearing del_hw_func after calling the
function. Now, it gets cleared by the del_hw_func implementation?

That does feel slightly brittle to me when thinking about this as an
API. On the other hand, it also seems reasonable to ensure that the
function gets cleared whenever you call it since it should only be
called once?

Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>

>  		up_write_ref_node(&fte->node, false);
>  		tree_put_node(&fte->node, false);
>  	} else if (fte->dests_size) {