linux-kernel - Re: [PATCH net-next 11/12] net/mlx5: HWS, Free unused action STE tables

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z_gL6fvuYUmD4oPS@nvidia.com>
Date: Thu, 10 Apr 2025 20:20:25 +0200
From: Vlad Dogaru <vdogaru@...dia.com>
To: Michal Kubiak <michal.kubiak@...el.com>
Cc: Tariq Toukan <tariqt@...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>, Gal Pressman <gal@...dia.com>,
	Leon Romanovsky <leonro@...dia.com>,
	Saeed Mahameed <saeedm@...dia.com>,
	Leon Romanovsky <leon@...nel.org>, netdev@...r.kernel.org,
	linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
	Moshe Shemesh <moshe@...dia.com>, Mark Bloch <mbloch@...dia.com>,
	Yevgeny Kliteynik <kliteyn@...dia.com>
Subject: Re: [PATCH net-next 11/12] net/mlx5: HWS, Free unused action STE
 tables

On Thu, Apr 10, 2025 at 07:28:18PM +0200, Michal Kubiak wrote:
> On Tue, Apr 08, 2025 at 05:00:55PM +0300, Tariq Toukan wrote:
> > From: Vlad Dogaru <vdogaru@...dia.com>
> > 
> > Periodically check for unused action STE tables and free their
> > associated resources. In order to do this safely, add a per-queue lock
> > to synchronize the garbage collect work with regular operations on
> > steering rules.
> > 
> > Signed-off-by: Vlad Dogaru <vdogaru@...dia.com>
> > Reviewed-by: Yevgeny Kliteynik <kliteyn@...dia.com>
> > Reviewed-by: Mark Bloch <mbloch@...dia.com>
> > Signed-off-by: Tariq Toukan <tariqt@...dia.com>
> > ---
> >  .../mlx5/core/steering/hws/action_ste_pool.c  | 88 ++++++++++++++++++-
> >  .../mlx5/core/steering/hws/action_ste_pool.h  | 11 +++
> >  .../mellanox/mlx5/core/steering/hws/context.h |  1 +
> >  3 files changed, 96 insertions(+), 4 deletions(-)
> 
> [...]
> 
> > +
> > +static void hws_action_ste_pool_cleanup(struct work_struct *work)
> > +{
> > +	enum mlx5hws_pool_optimize opt;
> > +	struct mlx5hws_context *ctx;
> > +	LIST_HEAD(cleanup);
> > +	int i;
> > +
> > +	ctx = container_of(work, struct mlx5hws_context,
> > +			   action_ste_cleanup.work);
> > +
> > +	for (i = 0; i < ctx->queues; i++) {
> > +		struct mlx5hws_action_ste_pool *p = &ctx->action_ste_pool[i];
> > +
> > +		mutex_lock(&p->lock);
> > +		for (opt = MLX5HWS_POOL_OPTIMIZE_NONE;
> > +		     opt < MLX5HWS_POOL_OPTIMIZE_MAX; opt++)
> > +			hws_action_ste_pool_element_collect_stale(
> > +				&p->elems[opt], &cleanup);
> > +		mutex_unlock(&p->lock);
> > +	}
> 
> As I understand, in the loop above all unused items are being collected
> to remove them at the end of the function, using `hws_action_ste_table_cleanup_list()`.
> 
> I noticed that only the collecting of elements is protected with the mutex.
> So I have a question: is it possible that in a very short period of time
> (between `mutex_unlock()` and `hws_action_ste_table_cleanup_list()` calls),
> the cleanup list can somehow be invalidated (by changing the STE state
> in another thread)?

An action_ste_table is either:
(a) in an action_ste_pool (indirectly, via a pool element); or
(b) in a cleanup list.

In situation (a), both the table's last_used timestamp and its offsets
are protected by the parent pool's mutex. The table can only be accessed
through its parent pool.

In situation (b), the table can only be accessed by its parent list, and
the only thing we do with it is free it.

There is only one transition, from state (a) to state (b), never the
other way around. This transition is done under the parent pool's mutex.

We only move tables to the cleanup list when all of their elements are
available, so there is no risk of a `chunk_free` call accessing a table
that's on a cleanup list: there are no chunks left to free.

I think this is airtight, but I'm happy to explore possible scenarios if
you have any in mind.

Thank you for the detailed review,
Vlad