Re: [V9fs-developer] [PATCH 1/2] net/9p: embed fcall in req to round down buffer allocs

[piaojun]

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
>