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

On Mon, Aug 13, 2018 at 3:48 AM, Dominique Martinet
[off-list ref] wrote:

piaojun wrote on Mon, Aug 13, 2018:

quoted

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.

Fixing unkillalble task would be nice. Don't know how much they are of
a problem in real life, but fixing them would allow fuzzer to find
other, potentially more critical bugs in 9p. These "task hung" crashes
are quite unpleasant for the fuzzer.

Thanks for all recent 9p work, Tomas!

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.

quoted

quoted

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

--
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.