Al Viro wrote on Wed, Jun 22, 2022 at 05:15:48AM +0100:
> that one is somewhat clumsier than usual and needs serious testing.
code inspection looks good to me, we revert everywhere I think we need
to revert for read/write and readdir doesn't need any special treatment.
I had a couple of nitpicks on debug messages, but that aside you can add
my R-b:
Reviewed-by: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
Now for tests though I'm not quite sure what I'm supposed to test to
stress the error cases, that'd actually let me detect a failure... Basic
stuff seems to work but I don't think I ever got into an error path
where that matters -- forcing short reads perhaps?
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> net/9p/client.c | 39 +++++++++++++++++++++++----------------
> net/9p/protocol.c | 3 +--
> net/9p/trans_virtio.c | 3 ++-
> 3 files changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/net/9p/client.c b/net/9p/client.c
> index d403085b9ef5..cb4324211561 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -1491,7 +1491,7 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
> struct p9_client *clnt = fid->clnt;
> struct p9_req_t *req;
> int count = iov_iter_count(to);
> - int rsize, non_zc = 0;
> + int rsize, received, non_zc = 0;
> char *dataptr;
>
> *err = 0;
> @@ -1520,36 +1520,40 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
> }
> if (IS_ERR(req)) {
> *err = PTR_ERR(req);
> + if (!non_zc)
> + iov_iter_revert(to, count - iov_iter_count(to));
> return 0;
> }
>
> *err = p9pdu_readf(&req->rc, clnt->proto_version,
> - "D", &count, &dataptr);
> + "D", &received, &dataptr);
> if (*err) {
> + if (!non_zc)
> + iov_iter_revert(to, count - iov_iter_count(to));
> trace_9p_protocol_dump(clnt, &req->rc);
> p9_tag_remove(clnt, req);
> return 0;
> }
> - if (rsize < count) {
> - pr_err("bogus RREAD count (%d > %d)\n", count, rsize);
> - count = rsize;
> + if (rsize < received) {
> + pr_err("bogus RREAD count (%d > %d)\n", received, rsize);
> + received = rsize;
> }
>
> p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", count);
This probably should be updated to received; we know how much we asked
to read already what we want to see here is what the server replied.
>
> if (non_zc) {
> - int n = copy_to_iter(dataptr, count, to);
> + int n = copy_to_iter(dataptr, received, to);
>
> - if (n != count) {
> + if (n != received) {
> *err = -EFAULT;
> p9_tag_remove(clnt, req);
> return n;
> }
> } else {
> - iov_iter_advance(to, count);
> + iov_iter_revert(to, count - received - iov_iter_count(to));
> }
> p9_tag_remove(clnt, req);
> - return count;
> + return received;
> }
> EXPORT_SYMBOL(p9_client_read_once);
>
> @@ -1567,6 +1571,7 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
> while (iov_iter_count(from)) {
> int count = iov_iter_count(from);
> int rsize = fid->iounit;
> + int written;
>
> if (!rsize || rsize > clnt->msize - P9_IOHDRSZ)
> rsize = clnt->msize - P9_IOHDRSZ;
> @@ -1584,27 +1589,29 @@ p9_client_write(struct p9_fid *fid, u64 offset, struct iov_iter *from, int *err)
> offset, rsize, from);
> }
> if (IS_ERR(req)) {
> + iov_iter_revert(from, count - iov_iter_count(from));
> *err = PTR_ERR(req);
> break;
> }
>
> - *err = p9pdu_readf(&req->rc, clnt->proto_version, "d", &count);
> + *err = p9pdu_readf(&req->rc, clnt->proto_version, "d", &written);
> if (*err) {
> + iov_iter_revert(from, count - iov_iter_count(from));
> trace_9p_protocol_dump(clnt, &req->rc);
> p9_tag_remove(clnt, req);
> break;
> }
> - if (rsize < count) {
> - pr_err("bogus RWRITE count (%d > %d)\n", count, rsize);
> - count = rsize;
> + if (rsize < written) {
> + pr_err("bogus RWRITE count (%d > %d)\n", written, rsize);
> + written = rsize;
> }
>
> p9_debug(P9_DEBUG_9P, "<<< RWRITE count %d\n", count);
likewise, please make it dump written.
--
Dominique
>
> p9_tag_remove(clnt, req);
> - iov_iter_advance(from, count);
> - total += count;
> - offset += count;
> + iov_iter_revert(from, count - written - iov_iter_count(from));
> + total += written;
> + offset += written;
> }
> return total;
> }
> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
> index 3754c33e2974..83694c631989 100644
> --- a/net/9p/protocol.c
> +++ b/net/9p/protocol.c
> @@ -63,9 +63,8 @@ static size_t
> pdu_write_u(struct p9_fcall *pdu, struct iov_iter *from, size_t size)
> {
> size_t len = min(pdu->capacity - pdu->size, size);
> - struct iov_iter i = *from;
>
> - if (!copy_from_iter_full(&pdu->sdata[pdu->size], len, &i))
> + if (!copy_from_iter_full(&pdu->sdata[pdu->size], len, from))
> len = 0;
>
> pdu->size += len;
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 2a210c2f8e40..1977d33475fe 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -331,7 +331,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
> if (err == -ERESTARTSYS)
> return err;
> }
> - n = iov_iter_get_pages_alloc(data, pages, count, offs);
> + n = iov_iter_get_pages_alloc2(data, pages, count, offs);
> if (n < 0)
> return n;
> *need_drop = 1;
> @@ -373,6 +373,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
> (*pages)[index] = kmap_to_page(p);
> p += PAGE_SIZE;
> }
> + iov_iter_advance(data, len);
> return len;
> }
> }
