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 ;) 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 -- Dominique
