Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 17 Jul 2018 02:45:42 +0200
From:   Dominique Martinet <asmadeus@codewreck.org>
To:     Chirantan Ekbote <chirantan@chromium.org>
Cc:     groug@kaod.org, linux-kernel@vger.kernel.org,
        v9fs-developer@lists.sourceforge.net, dgreid@chromium.org,
        groeck@chromium.org
Subject: Re: [PATCH v2] Fix zero-copy path in the 9p virtio transport
Message-ID: <20180717004542.GA8102@nautica>
References: <20180717003529.114368-1-chirantan@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20180717003529.114368-1-chirantan@chromium.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Chirantan Ekbote wrote on Mon, Jul 16, 2018:
> The zero-copy optimization when reading or writing large chunks of data
> is quite useful.  However, the 9p messages created through the zero-copy
> write path have an incorrect message size: it should be the size of the
> header + size of the data being written but instead it's just the size
> of the header.
> 
> This only works if the server ignores the size field of the message and
> otherwise breaks the framing of the protocol. Fix this by re-writing the
> message size field with the correct value.
> 
> Tested by running `dd if=/dev/zero of=out bs=4k count=1` inside a
> virtio-9p mount.
> 
> Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Tested-by: Greg Kurz <groug@kaod.org>

Ack, I've added this to my queue for 4.19

Thanks for moving the memcpy and the updated comment, it makes it more
clear that these are different fields of the message.

> ---
>  net/9p/trans_virtio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 05006cbb3361..65761381c58f 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -406,6 +406,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
>  	p9_debug(P9_DEBUG_TRANS, "virtio request\n");
>  
>  	if (uodata) {
> +		__le32 sz;
>  		int n = p9_get_mapped_pages(chan, &out_pages, uodata,
>  					    outlen, &offs, &need_drop);
>  		if (n < 0)
> @@ -416,6 +417,12 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
>  			memcpy(&req->tc->sdata[req->tc->size - 4], &v, 4);
>  			outlen = n;
>  		}
> +		/* The size field of the message must include the length of the
> +		 * header and the length of the data.  We didn't actually know
> +		 * the length of the data until this point so add it in now.
> +		 */
> +		sz = cpu_to_le32(req->tc->size + outlen);
> +		memcpy(&req->tc->sdata[0], &sz, sizeof(sz));
>  	} else if (uidata) {
>  		int n = p9_get_mapped_pages(chan, &in_pages, uidata,
>  					    inlen, &offs, &need_drop);

-- 
Dominique Martinet