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

[Tomas Bortoli <tomasbortoli@gmail.com>]

On 08/13/2018 03:04 PM, Dmitry Vyukov wrote:
> On Mon, Aug 13, 2018 at 3:48 AM, Dominique Martinet
> <asmadeus@codewreck.org> wrote:
>> 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.
> 
> 
> 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!
> 

You are welcome, I have to thank Dominique that helped me a lot, I like
to help here, it's educative.

> 
>> 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
>>
>> --
>> 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.