Messages in this thread | | | Date | Tue, 28 Aug 2018 01:09:54 +0200 | From | Dominique Martinet <> | Subject | Re: [PATCH v2 2/2] 9p: Add refcount to p9_req_t |
| |
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
-- Dominique
| |