Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Christian Schoenebeck <linux_oss@crudebyte.com>
To:     asmadeus@codewreck.org, Leon Romanovsky <leon@kernel.org>
Cc:     syzbot <syzbot+67d13108d855f451cafc@syzkaller.appspotmail.com>,
        davem@davemloft.net, edumazet@google.com, ericvh@gmail.com,
        kuba@kernel.org, linux-kernel@vger.kernel.org, lucho@ionkov.net,
        netdev@vger.kernel.org, pabeni@redhat.com,
        syzkaller-bugs@googlegroups.com,
        v9fs-developer@lists.sourceforge.net
Subject: Re: [syzbot] KASAN: use-after-free Read in rdma_close
Date:   Wed, 28 Sep 2022 14:57:07 +0200
Message-ID: <1783490.kFEjeSjHVE@silver>
In-Reply-To: <YzQ12+jtARpwS5bw@unreal>
References: <00000000000015ac7905e97ebaed@google.com> <YzQuoqyGsooyDfId@codewreck.org>
 <YzQ12+jtARpwS5bw@unreal>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Mittwoch, 28. September 2022 13:54:03 CEST Leon Romanovsky wrote:
> 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:
> > > > 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.

OK, maybe it's just me, but ask yourself Leon, if you were the only guy left 
(i.e. Dominique) still actively taking care for 9p, would those exactly be 
motivating phrases for your efforts? Just saying.

From technical perspective, yes, destruction in reverse order is usually the 
better way to go. Whether I would carve that in stone, without any exception, 
probably not.

Best regards,
Christian Schoenebeck