Messages in this thread | | | Subject | Re: [V9fs-developer] [PATCH v2 2/2] 9p: Add refcount to p9_req_t | From | piaojun <> | Date | Tue, 28 Aug 2018 09:07:29 +0800 |
| |
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 >
| |