netdev - Re: [PATCH mlx5-next 1/3] net/mlx5: Nullify eq->dbg and qp->dbg pointers post destruction

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]

Date:   Wed, 6 Apr 2022 10:55:31 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     Jason Gunthorpe <jgg@...dia.com>,
        Patrisious Haddad <phaddad@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>, linux-rdma@...r.kernel.org,
        netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Yishai Hadas <yishaih@...dia.com>
Subject: Re: [PATCH mlx5-next 1/3] net/mlx5: Nullify eq->dbg and qp->dbg
 pointers post destruction

On Tue, Apr 05, 2022 at 12:48:45PM -0700, Saeed Mahameed wrote:
> On 05 Apr 11:12, Leon Romanovsky wrote:
> > From: Patrisious Haddad <phaddad@...dia.com>
> > 
> > Prior to this patch in the case that destroy_unmap_eq()
> > failed and was called again, it triggered an additional call of
> 
> Where is it being failed and called again ? this shouldn't even be an
> option, we try to keep mlx5 symmetrical, constructors and destructors are
> supposed to be called only once in their respective positions.
> the callers must be fixed to avoid re-entry, or change destructors to clear
> up all resources even on failures, no matter what do not invent a reentry
> protocols to mlx5 destructors.

It can happen when QP is exposed through DEVX interface. In that flow,
only FW knows about it and reference count all users. This means that
attempt to destroy such QP will fail, but mlx5_core is structured in
such way that all cleanup was done before calling to FW to get
success/fail response.

For more detailed information, see this cover letter:
https://lore.kernel.org/all/20200907120921.476363-1-leon@kernel.org/

<...>

> > int mlx5_eq_destroy_generic(struct mlx5_core_dev *dev, struct mlx5_eq *eq)
> > {
> > +	struct mlx5_eq_table *eq_table = dev->priv.eq_table;
> > 	int err;
> > 
> > 	if (IS_ERR(eq))
> > 		return -EINVAL;
> > 
> > -	err = destroy_async_eq(dev, eq);
> > +	mutex_lock(&eq_table->lock);
> 
> Here you are inventing the re-entry. Please drop this and fix properly. And
> avoid boolean parameters to mlx5 core
> functions as much as possible, let's keep mlx5_core simple.

If after reading the link above, you were not convinced, let's take it offline.

Thanks