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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Aug 2018 11:23:01 +0200
From:   Greg Kurz <groug@...d.org>
To:     Dominique Martinet <asmadeus@...ewreck.org>
Cc:     v9fs-developer@...ts.sourceforge.net,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Dominique Martinet <dominique.martinet@....fr>,
        Matthew Wilcox <willy@...radead.org>,
        Jun Piao <piaojun@...wei.com>
Subject: Re: [PATCH v2 1/2] net/9p: embed fcall in req to round down buffer
 allocs

On Thu,  2 Aug 2018 04:37:31 +0200
Dominique Martinet <asmadeus@...ewreck.org> wrote:

> From: Dominique Martinet <dominique.martinet@....fr>
> 
> 'msize' is often a power of two, or at least page-aligned, so avoiding
> an overhead of two dozen bytes for each allocation will help the
> allocator do its work and reduce memory fragmentation.
> 
> Suggested-by: Matthew Wilcox <willy@...radead.org>
> Signed-off-by: Dominique Martinet <dominique.martinet@....fr>
> Cc: Matthew Wilcox <willy@...radead.org>
> Cc: Greg Kurz <groug@...d.org>
> Cc: Jun Piao <piaojun@...wei.com>
> ---
> 
> v2:
>  - Add extra label to not free uninitialized memory on alloc failure
>  - Rename p9_fcall_alloc to 9p_fcall_init
>  - Add a p9_fcall_fini function to echo to init
> 
[...]
> diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
> index 2ab4574183c9..c5cac97df7f7 100644
> --- a/net/9p/trans_rdma.c
> +++ b/net/9p/trans_rdma.c
> @@ -122,7 +122,7 @@ struct p9_rdma_context {
>  	dma_addr_t busa;
>  	union {
>  		struct p9_req_t *req;
> -		struct p9_fcall *rc;
> +		struct p9_fcall rc;
>  	};
>  };
>  
> @@ -320,8 +320,8 @@ recv_done(struct ib_cq *cq, struct ib_wc *wc)
>  	if (wc->status != IB_WC_SUCCESS)
>  		goto err_out;
>  
> -	c->rc->size = wc->byte_len;
> -	err = p9_parse_header(c->rc, NULL, NULL, &tag, 1);
> +	c->rc.size = wc->byte_len;
> +	err = p9_parse_header(&c->rc, NULL, NULL, &tag, 1);
>  	if (err)
>  		goto err_out;
>  
> @@ -331,12 +331,13 @@ recv_done(struct ib_cq *cq, struct ib_wc *wc)
>  
>  	/* Check that we have not yet received a reply for this request.
>  	 */
> -	if (unlikely(req->rc)) {
> +	if (unlikely(req->rc.sdata)) {
>  		pr_err("Duplicate reply for request %d", tag);
>  		goto err_out;
>  	}
>  
> -	req->rc = c->rc;
> +	req->rc.size = c->rc.size;
> +	req->rc.sdata = c->rc.sdata;
>  	p9_client_cb(client, req, REQ_STATUS_RCVD);
>  
>   out:
> @@ -361,7 +362,7 @@ send_done(struct ib_cq *cq, struct ib_wc *wc)
>  		container_of(wc->wr_cqe, struct p9_rdma_context, cqe);
>  
>  	ib_dma_unmap_single(rdma->cm_id->device,
> -			    c->busa, c->req->tc->size,
> +			    c->busa, c->req->tc.size,
>  			    DMA_TO_DEVICE);
>  	up(&rdma->sq_sem);
>  	kfree(c);
> @@ -401,7 +402,7 @@ post_recv(struct p9_client *client, struct p9_rdma_context *c)
>  	struct ib_sge sge;
>  
>  	c->busa = ib_dma_map_single(rdma->cm_id->device,
> -				    c->rc->sdata, client->msize,
> +				    c->rc.sdata, client->msize,
>  				    DMA_FROM_DEVICE);
>  	if (ib_dma_mapping_error(rdma->cm_id->device, c->busa))
>  		goto error;
> @@ -443,9 +444,9 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
>  	 **/
>  	if (unlikely(atomic_read(&rdma->excess_rc) > 0)) {
>  		if ((atomic_sub_return(1, &rdma->excess_rc) >= 0)) {
> -			/* Got one ! */
> -			kfree(req->rc);
> -			req->rc = NULL;
> +			/* Got one! */
> +			kfree(req->rc.sdata);

Shouldn't this be p9_fcall_fini(&req->rc) ?

The rest looks good, so, with that fixed, you can add:

Reviewed-by: Greg Kurz <groug@...d.org>

> +			req->rc.sdata = NULL;
>  			goto dont_need_post_recv;
>  		} else {
>  			/* We raced and lost. */