Re: Re: [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs

From: "Bernard Metzler" <BMT@zurich.ibm.com>
To: "Stefan Metzmacher" <metze@samba.org>
Cc: "linux-rdma" <linux-rdma@vger.kernel.org>
Subject: Re: Re: [PATCH 00/31] rdma/siw: fix a lot of deadlocks and use after free bugs
Date: Fri, 28 May 2021 14:35:07 +0000	[thread overview]
Message-ID: <OF3E623A7B.5CEABB75-ON002586E3.004D659D-002586E3.00501E8D@notes.na.collabserv.com> (raw)
In-Reply-To: <731bc8e0-4bbc-7006-23ef-2e0008f5e415@samba.org>

-----"Stefan Metzmacher" <metze@samba.org> wrote: -----

>To: "Bernard Metzler" <BMT@zurich.ibm.com>
>From: "Stefan Metzmacher" <metze@samba.org>
>Date: 05/26/2021 05:44PM
>Cc: "linux-rdma" <linux-rdma@vger.kernel.org>
>Subject: [EXTERNAL] Re: [PATCH 00/31] rdma/siw: fix a lot of
>deadlocks and use after free bugs
>
>Hi Bernard,
>
>> Much appreciated!
>> These are quite some patches, and I will need some time
>> to go through. Would bee nice if those would be broken
>> down into smaller bundles (introduce non-blocking connect,
>> _siw_cep_close() subroutine, fixing cep reference counting,
>> smp_mb() after STag invalidation, ..). 
>
>They mostly fall out naturally getting one step further
>with each commit. So most of them depend on each other.
>I'll see if I can reorder some of them, but I'm not sure it's really
>worth the effort.

Why not having just a few patches - one fixing the obvious
object management bug(s), one on a more concise error handling,
one on using exported kernel functions instead of calling
socket method directly, one introducing asynchronous connect..?

I understand you collected problems over time and fixed those,
but it would be much easier to digest if separated logically.

>
>> Anyway, many thanks for the effort,
>
>Thanks a lot for the review.
>
>> it will improve the driver!
>
>Yes!
>
>On top I have some code to support MPA rev1 in peer_to_peer mode
>in order to interoperate with a Chelcio T404-BT card running
>under Windows.
>
>In preparation I've code that moves the currently hardcoded values
>(which where module params before) into a per device structure,
>some like 'sdev->options.crc_strict'. With that we only need to
>find a good way to pass these parameters from userspace to the
>device. I guess that should be done somehow via the 'rdma link add'
>command, or via files similar to /proc/sys/net/ipv4/conf/*.
>
yes with dropping the module parameters we lost that flexibility...

I'd prefer protocol specific extensions to the rdma tools.

In fact, we could allow different CRC and MPA settings per
QP (which would make sense if we have connections from a
local siw device to multiple remote devices with different
capabilities etc.). But we do not have endpoint/QP object
specific settings in rdma netlink currently. 
Having link specific settings might be sufficient though.

>Here's my branch with all (partly incomplete) siw changes:
>INVALID URI REMOVED
>Fp-3Dmetze_linux_wip.git-3Ba-3Dshortlog-3Bh-3Drefs_heads_rdma-2Dnext-
>2Dsiw&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=2TaYXQ0T-r8ZO1PP1alNwU_QJcR
>RLfmYTAgd3QCvqSc&m=bcCk65hNAmUVFgsBxIE9Y6S1cnxdE1otmHllxAlO-Ko&s=Rgu7
>GuEAeI9MyUx7m03KEMLH2qA7Y3065X8LCBo3EBY&e= 
>

Thanks, I'll have a look.

>> First comments:
>> 
>> A non blocking connect does really makes sense as you
>> are pointing out. I hope it doesn't complicate the CM
>> code even further.
>> 
>> I think we agreed upon not using BUG() and BUG_ON(),
>> so please don't introduce it.
>
>Ok.
>
>> 'I hit a lot of bugs' is not very helpful, but just
>> a statement.
>
>More details are in the individual commit messages,
>should I double them in the cover letter next time?
>
>Currently I'm quite busy with other stuff...
>I hope to find some time in the next weeks to
>comment more detailed and post a new revision.
>
>metze
>
Thanks again!
Bernard.