Messages in this thread | | | Date | Mon, 13 Aug 2018 03:48:15 +0200 | From | Dominique Martinet <> | Subject | Re: [V9fs-developer] [PATCH 2/2] 9p: Add refcount to p9_req_t |
| |
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
| |