netdev - Re: [PATCH net-next V2 09/15] net/mlx5: fs, add HWS fte API functions

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: <2fe1bbd5-834e-4142-b101-8c420c459472@intel.com>
Date: Mon, 13 Jan 2025 15:25:41 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Tariq Toukan <tariqt@...dia.com>, Moshe Shemesh <moshe@...dia.com>
CC: <netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>, "Jakub
 Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Eric Dumazet
	<edumazet@...gle.com>, Andrew Lunn <andrew+netdev@...n.ch>, Saeed Mahameed
	<saeedm@...dia.com>, Gal Pressman <gal@...dia.com>, Leon Romanovsky
	<leonro@...dia.com>, Mark Bloch <mbloch@...dia.com>, Yevgeny Kliteynik
	<kliteyn@...dia.com>
Subject: Re: [PATCH net-next V2 09/15] net/mlx5: fs, add HWS fte API functions

On 1/9/25 17:05, Tariq Toukan wrote:
> From: Moshe Shemesh <moshe@...dia.com>
> 
> Add create, destroy and update fte API functions for adding, removing
> and updating flow steering rules in HW Steering mode. Get HWS actions
> according to required rule, use actions from pool whenever possible.
> 
> Signed-off-by: Moshe Shemesh <moshe@...dia.com>
> Reviewed-by: Yevgeny Kliteynik <kliteyn@...dia.com>
> Reviewed-by: Mark Bloch <mbloch@...dia.com>
> Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> ---
>   .../net/ethernet/mellanox/mlx5/core/fs_core.h |   5 +-
>   .../mellanox/mlx5/core/steering/hws/fs_hws.c  | 549 ++++++++++++++++++
>   .../mellanox/mlx5/core/steering/hws/fs_hws.h  |  13 +
>   3 files changed, 566 insertions(+), 1 deletion(-)
> 

side note: there is an option for git to put .h files before .c files
in the diff (and thus in the send email), you can plug a git orderfile:
./scripts/git.orderFile

> +	if (fte_action->action & MLX5_FLOW_CONTEXT_ACTION_VLAN_POP) {
> +		tmp_action = mlx5_fs_get_action_pop_vlan(fs_ctx);
> +		if (!tmp_action) {
> +			err = -ENOMEM;
> +			goto free_actions;
> +		}
> +		(*ractions)[num_actions++].action = tmp_action;
> +	}
> +
> +	if (fte_action->action & MLX5_FLOW_CONTEXT_ACTION_VLAN_POP_2) {
> +		tmp_action = mlx5_fs_get_action_pop_vlan(fs_ctx);

It's not clear if this was a typo/ommision of second pop impl,
or it is expected to just work (this get_action_pop_vlan() helper
returns precomputed action command, that has no link to prev action, so
hard to tell).


> +		if (!tmp_action) {
> +			err = -ENOMEM;

I would use -E2BIG for cases like that

> +			goto free_actions;
> +		}
> +		(*ractions)[num_actions++].action = tmp_action;
> +	}
> +
> +	if (fte_action->action & MLX5_FLOW_CONTEXT_ACTION_MOD_HDR) {
> +		mh_data = fte_action->modify_hdr->fs_hws_action.mh_data;
> +		(*ractions)[num_actions].modify_header.offset = mh_data->offset;
> +		(*ractions)[num_actions].modify_header.data = mh_data->data;
> +		(*ractions)[num_actions++].action =
> +			fte_action->modify_hdr->fs_hws_action.hws_action;
> +	}
> +
> +	if (fte_action->action & MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH) {
> +		tmp_action = mlx5_fs_get_action_push_vlan(fs_ctx);
> +		if (!tmp_action) {
> +			err = -ENOMEM;
> +			goto free_actions;
> +		}
> +		(*ractions)[num_actions].push_vlan.vlan_hdr =
> +			htonl(mlx5_fs_calc_vlan_hdr(&fte_action->vlan[0]));
> +		(*ractions)[num_actions++].action = tmp_action;
> +	}
> +
> +	if (fte_action->action & MLX5_FLOW_CONTEXT_ACTION_VLAN_PUSH_2) {
> +		tmp_action = mlx5_fs_get_action_push_vlan(fs_ctx);
> +		if (!tmp_action) {
> +			err = -ENOMEM;
> +			goto free_actions;
> +		}
> +		(*ractions)[num_actions].push_vlan.vlan_hdr =
> +			htonl(mlx5_fs_calc_vlan_hdr(&fte_action->vlan[1]));
> +		(*ractions)[num_actions++].action = tmp_action;
> +	}
> +
> +	if (delay_encap_set) {
> +		pr_data = fte_action->pkt_reformat->fs_hws_action.pr_data;
> +		(*ractions)[num_actions].reformat.offset = pr_data->offset;
> +		(*ractions)[num_actions].reformat.data = pr_data->data;
> +		(*ractions)[num_actions++].action =
> +			fte_action->pkt_reformat->fs_hws_action.hws_action;
> +	}
> +
> +	if (fte_action->action & MLX5_FLOW_CONTEXT_ACTION_COUNT) {
> +		list_for_each_entry(dst, &fte->node.children, node.list) {
> +			struct mlx5_fc *counter;
> +
> +			if (dst->dest_attr.type !=
> +			    MLX5_FLOW_DESTINATION_TYPE_COUNTER)
> +				continue;
> +
> +			if (num_actions == MLX5_FLOW_CONTEXT_ACTION_MAX) {
> +				err = -EOPNOTSUPP;

I get that some combinations are not supported, but this looks rather
like "too much requested" (-E2BIG)

> +				goto free_actions;
> +			}
> +

In general, this function is over 300 lines, it would benefit from some
"rule action builder", that will keep count of actions, and would allow
you to check (most) erros at the very end

Should be fine to over-alloc *ractions, so it will be safe to fill it
without checking for if there is a space for each element, and just
decide it's too much, when it's too much for your liking at the very end