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
>
