Re: [syzbot] KASAN: use-after-free Read in rdma_close

On Wed, Sep 28, 2022 at 08:23:14PM +0900, asmadeus@codewreck.org wrote:

Leon Romanovsky wrote on Wed, Sep 28, 2022 at 01:49:19PM +0300:

quoted

quoted

But I agree I did get that wrong: trans_mod->close() wasn't called if
create failed.
We do want the idr_for_each_entry() that is in p9_client_destroy so
rather than revert the commit (fix a bug, create a new one..) I'd rather
split it out in an internal function that takes a 'bool close' or
something to not duplicate the rest.
(Bit of a nitpick, sure)

Please do proper unwind without extra variable.

Proper unwind means that you will call to symmetrical functions in
destroy as you used in create:
alloc -> free
create -> close
e.t.c

When you use some global function like you did, there is huge chance
to see unwind bugs.

No.

Let's agree to disagree.

Duplicating complicated cleanup code leads to leaks like we used to
have; that destroy function already frees up things in the right order.

It is pretty straightforward code, nothing complex there.

Just pause for a minute, and ask yourself how totally random guy who
looked on this syzbot bug just because RDMA name in it, found the issue
so quickly.

I will give a hint, I saw not symmetrical error unwind in call trace.

Thanks