From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomas Bortoli Subject: Re: [V9fs-developer] [PATCH 2/2] 9p: Add refcount to p9_req_t Date: Mon, 13 Aug 2018 20:14:54 +0200 Message-ID: References: <20180811144254.23665-1-tomasbortoli@gmail.com> <20180811144254.23665-2-tomasbortoli@gmail.com> <5B70E0D1.2080300@huawei.com> <20180813014815.GB6777@nautica> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: piaojun , Eric Van Hensbergen , Ron Minnich , Latchesar Ionkov , Dominique Martinet , netdev , LKML , syzkaller , v9fs-developer@lists.sourceforge.net, David Miller To: Dmitry Vyukov , Dominique Martinet Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On 08/13/2018 03:04 PM, Dmitry Vyukov wrote: > On Mon, Aug 13, 2018 at 3:48 AM, Dominique Martinet > 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.