[v2,2/2] 9p: Add refcount to p9_req_t

To avoid use-after-free(s), use a refcount to keep track of the
usable references to any instantiated struct p9_req_t.

This commit adds p9_req_put(), p9_req_get() and p9_req_try_get() as
wrappers to kref_put(), kref_get() and kref_get_unless_zero().
These are used by the client and the transports to keep track of
valid requests' references.

p9_free_req() is added back and used as callback by kref_put().

Add SLAB_TYPESAFE_BY_RCU as it ensures that the memory freed by
kmem_cache_free() will not be reused for another type until the rcu
synchronisation period is over, so an address gotten under rcu read
lock is safe to inc_ref() without corrupting random memory while
the lock is held.

Co-developed-by: Dominique Martinet <dominique.martinet@cea.fr>
Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com
Signed-off-by: Dominique Martinet <dominique.martinet@cea.fr>
---
 include/net/9p/client.h | 14 +++++++++++++
 net/9p/client.c         | 54 +++++++++++++++++++++++++++++++++++++++++++------
 net/9p/trans_fd.c       | 11 +++++++++-
 net/9p/trans_rdma.c     |  1 +
 4 files changed, 73 insertions(+), 7 deletions(-)

Tomas Bortoli wrote on Tue, Aug 14, 2018:
> To avoid use-after-free(s), use a refcount to keep track of the
> usable references to any instantiated struct p9_req_t.
> 
> This commit adds p9_req_put(), p9_req_get() and p9_req_try_get() as
> wrappers to kref_put(), kref_get() and kref_get_unless_zero().
> These are used by the client and the transports to keep track of
> valid requests' references.
> 
> p9_free_req() is added back and used as callback by kref_put().
> 
> Add SLAB_TYPESAFE_BY_RCU as it ensures that the memory freed by
> kmem_cache_free() will not be reused for another type until the rcu
> synchronisation period is over, so an address gotten under rcu read
> lock is safe to inc_ref() without corrupting random memory while
> the lock is held.


FWIW, since 4.19-rc1 has been tagged I was going to push this and all
the perrequesites to linux-next, but I've managed to leak some requests
by interrupting them in trans_virtio.
I think I've found why (see below), so I'll push a fixed version after
some more testing and another thorough read -- at some point today, but
this hasn't been 'approved' explicitely so please review! :)

(Jun, I think you'll need to ask again to rename 'req' to 'rreq' if you
think it's important -- I think such a rename should go in a separate
patch anyway, there's plenty of time until the 4.20 merge window)


By "all the prerequesites" I mean this patch "serie":
* 9p: Use a slab for allocating requests
* 9p: Remove p9_idpool
* net/9p: embed fcall in req to round down buffer allocs
* net/9p: add a per-client fcall kmem_cache
* 9p: rename p9_free_req() function
* 9p: Add refcount to p9_req_t

All the other patchs have had some review though, I was just waiting for
the start of this cycle, but if someone has any issue with the above
patches now is a good time to say.


> diff --git a/net/9p/client.c b/net/9p/client.c
> index 7942c0bfcc5b..c9bb5d41afa4 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -716,6 +756,8 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
>  
>  	err = c->trans_mod->request(c, req);
>  	if (err < 0) {
> +		/* write won't happen */
> +		p9_req_put(req);
>  		if (err != -ERESTARTSYS && err != -EFAULT)
>  			c->status = Disconnected;
>  		goto recalc_sigpending;

p9_client_zc_rpc needs the same put if zc_request failed, I'm not sure
why it wasn't here in my draft

Hi Dominique,

On 2018/8/28 7:09, Dominique Martinet wrote:
> Tomas Bortoli wrote on Tue, Aug 14, 2018:
>> To avoid use-after-free(s), use a refcount to keep track of the
>> usable references to any instantiated struct p9_req_t.
>>
>> This commit adds p9_req_put(), p9_req_get() and p9_req_try_get() as
>> wrappers to kref_put(), kref_get() and kref_get_unless_zero().
>> These are used by the client and the transports to keep track of
>> valid requests' references.
>>
>> p9_free_req() is added back and used as callback by kref_put().
>>
>> Add SLAB_TYPESAFE_BY_RCU as it ensures that the memory freed by
>> kmem_cache_free() will not be reused for another type until the rcu
>> synchronisation period is over, so an address gotten under rcu read
>> lock is safe to inc_ref() without corrupting random memory while
>> the lock is held.
> 
> 
> FWIW, since 4.19-rc1 has been tagged I was going to push this and all
> the perrequesites to linux-next, but I've managed to leak some requests
> by interrupting them in trans_virtio.
> I think I've found why (see below), so I'll push a fixed version after
> some more testing and another thorough read -- at some point today, but
> this hasn't been 'approved' explicitely so please review! :)
> 
> (Jun, I think you'll need to ask again to rename 'req' to 'rreq' if you
> think it's important -- I think such a rename should go in a separate
> patch anyway, there's plenty of time until the 4.20 merge window)
> 

I still think such a rename is necessary, and as you said, it will be
better go in another patch.

Thanks,
Jun

> 
> By "all the prerequesites" I mean this patch "serie":
> * 9p: Use a slab for allocating requests
> * 9p: Remove p9_idpool
> * net/9p: embed fcall in req to round down buffer allocs
> * net/9p: add a per-client fcall kmem_cache
> * 9p: rename p9_free_req() function
> * 9p: Add refcount to p9_req_t
> 
> All the other patchs have had some review though, I was just waiting for
> the start of this cycle, but if someone has any issue with the above
> patches now is a good time to say.
> 
> 
>> diff --git a/net/9p/client.c b/net/9p/client.c
>> index 7942c0bfcc5b..c9bb5d41afa4 100644
>> --- a/net/9p/client.c
>> +++ b/net/9p/client.c
>> @@ -716,6 +756,8 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
>>  
>>  	err = c->trans_mod->request(c, req);
>>  	if (err < 0) {
>> +		/* write won't happen */
>> +		p9_req_put(req);
>>  		if (err != -ERESTARTSYS && err != -EFAULT)
>>  			c->status = Disconnected;
>>  		goto recalc_sigpending;
> 
> p9_client_zc_rpc needs the same put if zc_request failed, I'm not sure
> why it wasn't here in my draft
>

piaojun wrote on Tue, Aug 28, 2018:
> > (Jun, I think you'll need to ask again to rename 'req' to 'rreq' if you
> > think it's important -- I think such a rename should go in a separate
> > patch anyway, there's plenty of time until the 4.20 merge window)
> > 
> 
> I still think such a rename is necessary, and as you said, it will be
> better go in another patch.

Tomas can you send a patch for that please?
It's not very interesting, but might as well finish this properly :)

> >> diff --git a/net/9p/client.c b/net/9p/client.c
> >> index 7942c0bfcc5b..c9bb5d41afa4 100644
> >> --- a/net/9p/client.c
> >> +++ b/net/9p/client.c
> >> @@ -716,6 +756,8 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
> >>  
> >>  	err = c->trans_mod->request(c, req);
> >>  	if (err < 0) {
> >> +		/* write won't happen */
> >> +		p9_req_put(req);
> >>  		if (err != -ERESTARTSYS && err != -EFAULT)
> >>  			c->status = Disconnected;
> >>  		goto recalc_sigpending;
> > 
> > p9_client_zc_rpc needs the same put if zc_request failed, I'm not sure
> > why it wasn't here in my draft

Ah, I remember a bit better now, this is not as simple as adding the
same check after zc_request because the zc_request embeds the wait
itself to do its own cleanup after the reply came (unpin pages that were
sent to the server).

This brings in an interesting race condition that if the
wait_event_killable() in the zc_request is interrupted, the user data
pages that were pinned get unpinned and could potentially be moved
before the server replies... Even if they're not moved the user would be
told the read/write failed and could reuse the memory that would be
read/written later.
I'm not sure how this part works but it's probably not great.

Greg, do you have an opinion on this?


This is tricky, we cannot even rely on the refcounting for this as the
zc pages are likely user pages, so it'll be bad if we return from the
syscall and the memory gets accessed later.
On the other hand making that wait non-killable isn't a good solution
either, and we cannot use flush for virtio, so I don't have any idea for
this... Any magic virtio "take-back"?



Well, this would be for another patch anyway - for now I'll just do the
p9_req_put if it hasn't been kicked so that means something like the
following diff.. But my test bed is currently down so I'll wait for
tests to push:
-------8<----------------
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 7728b0acde09..36a1401c0722 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -404,6 +404,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	struct scatterlist *sgs[4];
 	size_t offs;
 	int need_drop = 0;
+	int kicked = 0;
 
 	p9_debug(P9_DEBUG_TRANS, "virtio request\n");
 
@@ -498,6 +499,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	}
 	virtqueue_kick(chan->vq);
 	spin_unlock_irqrestore(&chan->lock, flags);
+	kicked = 1;
 	p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
 	err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);
 	/*
@@ -518,6 +520,10 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	}
 	kvfree(in_pages);
 	kvfree(out_pages);
+	if (!kicked) {
+		/* reply won't come */
+		p9_req_put(req);
+	}
 	return err;
 }
 
-------8<----------------

Dominique Martinet wrote on Tue, Aug 28, 2018:
> I think I've found why (see below), so I'll push a fixed version after
> some more testing and another thorough read -- at some point today, but
> this hasn't been 'approved' explicitely so please review! :)

While the issue I pointed at was real, it wasn't what was causing the
refcount leak I was observing -- the problem is that we didn't drop a
ref when the request was successfully cancelled (e.g. the reply to the
flush came and the original request didn't get replied to)

The reason for this was that there were multiple versions of the patch
which alternated between doing the put in client.c after the cancelled
callback inconditionally, and doing the put in each transport's
cancelled() function, but virtio does not have this callback so that
didn't get added in the final version (codeveloping is hard); so I've
added an else() close to just issue a put if there is no callback.

(In the end, it felt better to have the req_put in the transport because
trans_fd is making refcounting difficult with its list handling, and
separating the put from the list removal would be more confusing than is
gained by sharing code)



Anyway, that's starting to be quite different from the v2 so I'll send a
v3 keeping Tomas as the author -- please check my edits are alright with
you, Tomas.

Meanwhile I'll keep running tests, I'm now confident about virtio but
want to spend more time on other transports again, so delaying the push
to linux-next for a few more days...