Re: [V9fs-developer] [PATCH 2/2] 9p: Add refcount to p9_req_t

[Dominique Martinet]

piaojun wrote on Mon, Aug 13, 2018:
> Could you help paste the reason of the crash bug to help others
> understand more clearly? And I have another question below.

The problem for tcp (but other transports have a similar problem) is
that with a malicious server like syzkaller they can try to submit
replies before the request came in.

This leads in the writer thread trying to write a buffer that has
already been freed, and if memory has been reused could potentially leak
some information.

Now, with the previous patches this is based on this would be a slab and
the likeliness of it being sensitive information is rather low (it would
likely be some other packet being sent twice, or a mix and match of two
packets that would have been sent anyway), but it would nevertheless be
a use after free.


There is a second advantage to this reference counting, that is now we
have this system we will be able to implement flush asynchronously.
This will remove the need for the 'goto again' in p9_client_rpc which
was making 9p threads unkillable in practice if the server would not
reply to the flush requests.
Even if the server replies I've always found myself needing to hit ^C
multiple times to exit a process doing I/Os and I think fixing that
behaviour will make 9p more comfortable to use.


> > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> > index 20f46f13fe83..686e24e355d0 100644
> > --- a/net/9p/trans_fd.c
> > +++ b/net/9p/trans_fd.c
> > @@ -132,6 +132,7 @@ struct p9_conn {
> >  	struct list_head req_list;
> >  	struct list_head unsent_req_list;
> >  	struct p9_req_t *req;
> > +	struct p9_req_t *wreq;
> 
> Why adding a wreq for write work? And I wonder we should rename req to
> rreq?

We need to store a pointer to the request for the write thread because
we need to put the reference to it when we're done writing its content.

Previously, the worker would only store the write buffer there but
that's not enough to figure what request to dereference.


I personally don't think renaming req to rreq would bring much but it
could be done in another patch if you think that'd be helpful; I think
it shouldn't be done here at least to make the patch more readable.

-- 
Dominique