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;
Date:   Fri, 19 Aug 2022 05:23:36 +0900
From:   asmadeus@codewreck.org
To:     Christian Schoenebeck <linux_oss@crudebyte.com>
Cc:     syzbot <syzbot+de52531662ebb8823b26@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 p9_req_put
Message-ID: <Yv6fyMxHq1CI5iZf@codewreck.org>
References: <0000000000001c3efc05e6693f06@google.com>
 <YvyD053bdbGE9xoo@codewreck.org>
 <2207113.SgyDDyVIbp@silver>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <2207113.SgyDDyVIbp@silver>
Precedence: bulk

Christian Schoenebeck wrote on Thu, Aug 18, 2022 at 05:12:17PM +0200:
> > @@ -997,12 +997,8 @@ struct p9_client *p9_client_create(const char
> > *dev_name, char *options)
> > 
> >  	return clnt;
> > 
> > -close_trans:
> > -	clnt->trans_mod->close(clnt);
> > -put_trans:
> > -	v9fs_put_trans(clnt->trans_mod);
> > -free_client:
> > -	kfree(clnt);
> > +out:
> > +	p9_client_destroy(clnt);
> >  	return ERR_PTR(err);
> >  }
> >  EXPORT_SYMBOL(p9_client_create);
> 
> Looks like a nice reduction to me!
> 
> As p9_client_destroy() is doing a bit more than current code, I would probably 
> additionally do s/kmalloc/kzmalloc/ at the start of the function, which would 
> add more safety & reduction.

Good point, I checked the variables p9_client_destroy cares about get
initialized but kzalloc is safer, let's switch that as well.
> > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> > index e758978b44be..60fcc6b30b46 100644
> > --- a/net/9p/trans_fd.c
> > +++ b/net/9p/trans_fd.c
> > @@ -205,6 +205,8 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
> >  		list_move(&req->req_list, &cancel_list);
> >  	}
> > 
> > +	spin_unlock(&m->client->lock);
> > +
> >  	list_for_each_entry_safe(req, rtmp, &cancel_list, req_list) {
> >  		p9_debug(P9_DEBUG_ERROR, "call back req %p\n", req);
> >  		list_del(&req->req_list);
> > @@ -212,7 +214,6 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
> >  			req->t_err = err;
> >  		p9_client_cb(m->client, req, REQ_STATUS_ERROR);
> >  	}
> > -	spin_unlock(&m->client->lock);
> >  }
> 
> Are you sure that would resolve that (other) syzbot report? I just had a 
> glimpse at it yet, but I don't see this list iteration portion being involved 
> in the backtrace provided by the report, is it?

It won't fix the inconsistent locking ones, but should take care of
http://lkml.kernel.org/r/000000000000cad57405e5b5dbb7@google.com

ffff888026be2c18 (&clnt->lock){+.+.}-{2:2}, at: p9_conn_cancel+0xaa/0x970 net/9p/trans_fd.c:192
holding the lock in that function, calling
p9_client_cb itself calling p9_req_put and locking again when refcount
hits 0.

And that one has a reproducer, so syzbot will confirm if it helps when I
get around to pushing it (probably this weekend) :)

--
Dominique