On 2018/7/31 9:12, Dominique Martinet wrote: > piaojun wrote on Tue, Jul 31, 2018: >> This is really a *big* patch, but the modification seems no harm. And I >> suggest running testcases to cover this. Please see my comments below. > > I'm always running tests, but more never hurt - please help ;) I'm glad to help testing, and actually I'm going to run some testcases in xfs-test for 9p. > > For reference I'm running a subset of cthon04[1], ltp[2] and some custom > tests like these[3][4] > > [1] https://fedorapeople.org/cgit/steved/public_git/cthon04.git/ > [2] https://github.com/linux-test-project/ltp > [3] https://github.com/phdeniel/sigmund/blob/master/modules/allfs.inc#L208 > [4] https://github.com/phdeniel/sigmund/blob/master/modules/allfs.inc#L251 > >>> [...] >>> @@ -263,13 +261,13 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) >>> if (!req) >>> return NULL; >>> >>> - req->tc = p9_fcall_alloc(alloc_msize); >>> - req->rc = p9_fcall_alloc(alloc_msize); >>> - if (!req->tc || !req->rc) >>> + if (p9_fcall_alloc(&req->tc, alloc_msize)) >>> + goto free; >>> + if (p9_fcall_alloc(&req->rc, alloc_msize)) >>> goto free; >>> >>> - p9pdu_reset(req->tc); >>> - p9pdu_reset(req->rc); >>> + p9pdu_reset(&req->tc); >>> + p9pdu_reset(&req->rc); >>> req->status = REQ_STATUS_ALLOC; >>> init_waitqueue_head(&req->wq); >>> INIT_LIST_HEAD(&req->req_list); >>> @@ -281,7 +279,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) >>> GFP_NOWAIT); >>> else >>> tag = idr_alloc(&c->reqs, req, 0, P9_NOTAG, GFP_NOWAIT); >>> - req->tc->tag = tag; >>> + req->tc.tag = tag; >>> spin_unlock_irq(&c->lock); >>> idr_preload_end(); >>> if (tag < 0) >>> @@ -290,8 +288,8 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) >>> return req; >>> >>> free: >>> - kfree(req->tc); >>> - kfree(req->rc); >>> + kfree(req->tc.sdata); >>> + kfree(req->rc.sdata); >> >> I wonder if we will free a wild pointer as 'sdata' has not been initialized NULL. > > Good point, it's possible to jump here if the first fcall_alloc failed > since this declustered the two allocations. > > Please consider this added to the previous patch (I'll send a v2 after > this has had more time for review, you can find the amended commit in my > 9p-test tree meanwhile): > -----8<----------------------------- > diff --git a/net/9p/client.c b/net/9p/client.c > index ba99a94a12c9..fe030ef1c076 100644 > --- a/net/9p/client.c > +++ b/net/9p/client.c > @@ -262,7 +262,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) > return NULL; > > if (p9_fcall_alloc(&req->tc, alloc_msize)) > - goto free; > + goto free_req; > if (p9_fcall_alloc(&req->rc, alloc_msize)) > goto free; > > @@ -290,6 +290,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size) > free: > kfree(req->tc.sdata); > kfree(req->rc.sdata); > +free_req: > kmem_cache_free(p9_req_cache, req); > return ERR_PTR(-ENOMEM); > } > -----8<----------------------------- > > The second goto doesn't need changing because rc.sdata will be set to > NULL if the allocation failed >
