netdev - Re: [PATCH net-next V3 03/11] net/mlx5: fs, add counter object to flow destination

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: <015ad3e1-9526-4d40-af4e-ff852d9dd117@intel.com>
Date: Thu, 19 Dec 2024 10:00:15 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Tariq Toukan <tariqt@...dia.com>, Moshe Shemesh <moshe@...dia.com>
CC: <netdev@...r.kernel.org>, 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>, "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>
Subject: Re: [PATCH net-next V3 03/11] net/mlx5: fs, add counter object to
 flow destination

On 12/18/24 16:09, Tariq Toukan wrote:
> From: Moshe Shemesh <moshe@...dia.com>
> 
> Currently mlx5_flow_destination includes counter_id which is assigned in
> case we use flow counter on the flow steering rule. However, counter_id
> is not enough data in case of using HW Steering. Thus, have mlx5_fc
> object as part of mlx5_flow_destination instead of counter_id and assign
> it where needed.
> 
> In case counter_id is received from user space, create a local counter
> object to represent it.
> 
> 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>
> ---
>   drivers/infiniband/hw/mlx5/fs.c               | 37 +++++++++----
>   .../mellanox/mlx5/core/diag/fs_tracepoint.h   |  2 +-
>   .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 20 +++----
>   .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  2 +-
>   .../mellanox/mlx5/core/esw/acl/egress_lgcy.c  |  2 +-
>   .../mellanox/mlx5/core/esw/acl/ingress_lgcy.c |  2 +-
>   .../ethernet/mellanox/mlx5/core/esw/bridge.c  | 20 +++----
>   .../mellanox/mlx5/core/eswitch_offloads.c     |  2 +-
>   .../net/ethernet/mellanox/mlx5/core/fs_cmd.c  |  2 +-
>   .../net/ethernet/mellanox/mlx5/core/fs_core.c |  1 +
>   .../ethernet/mellanox/mlx5/core/fs_counters.c | 52 +++++++++++++++++++
>   .../mellanox/mlx5/core/lib/macsec_fs.c        |  8 +--
>   .../mellanox/mlx5/core/steering/sws/fs_dr.c   |  2 +-
>   drivers/vdpa/mlx5/net/mlx5_vnet.c             |  4 +-
>   include/linux/mlx5/fs.h                       |  4 +-
>   15 files changed, 116 insertions(+), 44 deletions(-)



> +/**
> + * mlx5_fc_local_create - Allocate mlx5_fc struct for a counter which
> + * was already acquired using its counter id and bulk data.
> + *
> + * @counter_id: counter acquired counter id
> + * @offset: counter offset from bulk base
> + * @bulk_size: counter's bulk size as was allocated
> + *
> + * Return: Pointer to mlx5_fc on success, ERR_PTR otherwise.
> + */
> +struct mlx5_fc *
> +mlx5_fc_local_create(u32 counter_id, u32 offset, u32 bulk_size)
> +{
> +	struct mlx5_fc_bulk *bulk;
> +	struct mlx5_fc *counter;
> +
> +	counter = kzalloc(sizeof(*counter), GFP_KERNEL);
> +	if (!counter)
> +		return ERR_PTR(-ENOMEM);
> +	bulk = kzalloc(sizeof(*bulk), GFP_KERNEL);
> +	if (!bulk) {
> +		kfree(counter);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +

I would expect to have following line added here:

	counter->bulk = bulk;

otherwise that is memleak?

> +	counter->type = MLX5_FC_TYPE_LOCAL;
> +	counter->id = counter_id;
> +	bulk->base_id = counter_id - offset;
> +	bulk->bulk_len = bulk_size;
> +	return counter;
> +}
> +EXPORT_SYMBOL(mlx5_fc_local_create);
> +
> +void mlx5_fc_local_destroy(struct mlx5_fc *counter)
> +{
> +	if (!counter || counter->type != MLX5_FC_TYPE_LOCAL)
> +		return;
> +
> +	kfree(counter->bulk);

in the whole patch there is no "->bulk ="
you didn't catched that as it's fine to kfree(NULL) of course

> +	kfree(counter);
> +}
> +EXPORT_SYMBOL(mlx5_fc_local_destroy);