linux-kernel - RE: [PATCH rdma-next 1/1] RDMA/mana

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] [day] [month] [year] [list]

Date: Fri, 7 Jun 2024 07:44:35 +0000
From: Konstantin Taranov <kotaranov@...rosoft.com>
To: Long Li <longli@...rosoft.com>, Konstantin Taranov
	<kotaranov@...ux.microsoft.com>, Wei Hu <weh@...rosoft.com>,
	"sharmaajay@...rosoft.com" <sharmaajay@...rosoft.com>, "jgg@...pe.ca"
	<jgg@...pe.ca>, "leon@...nel.org" <leon@...nel.org>
CC: "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH rdma-next 1/1] RDMA/mana_ib: process QP error events



> -----Original Message-----
> From: Long Li <longli@...rosoft.com>
> Sent: Friday, 7 June 2024 04:45
> To: Konstantin Taranov <kotaranov@...rosoft.com>; Konstantin Taranov
> <kotaranov@...ux.microsoft.com>; Wei Hu <weh@...rosoft.com>;
> sharmaajay@...rosoft.com; jgg@...pe.ca; leon@...nel.org
> Cc: linux-rdma@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: RE: [PATCH rdma-next 1/1] RDMA/mana_ib: process QP error events
> 
> > > Strange logic. Why not do:
> > > if (!refcount_dec_and_test(&qp->refcount))
> > > 	wait_for_completion(&qp->free);
> > >
> >
> > It might work, but the logic will be even stranger and it will prevent
> > some debugging.
> > With the proposed change, qp->free may not be completed even though
> > the counter is 0.
> 
> Why this is a problem? mana_ib_destroy_rc_qp() is the only one waiting on
> it?

Sure, it is not a problem if you do not have a bug. The code is subject to change and bugs
could appear.

> 
> > As a result, the change makes an incorrect state to be an expected
> > state, thereby making bugs with that side effect undetectable.
> > E.g., we have a bug "use after free" and then we try to trace whether
> > qp was in use.
> 
> I don't get it. Can you explain why?

Please re-read my explanation again. Also please check the kernel code of other
drivers that use wait_for_completion. Many of them do the same three lines as I do
in this patch.

> 
> > Plus, it is a good practice deinit everything that was inited. With
> > the proposed change it is violated.
> 
> You shouldn't call wait_for_completion if it's not needed. This is not a
> "deinit".

See your message, you proposed to remove complete as well.