| Message ID | 20220622041552.737754-40-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/44] 9p: handling Rerror without copy_from_iter_full() | expand |
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@codewreck.org> 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@zeniv.linux.org.uk> > --- > 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; > } > }
On Mittwoch, 22. Juni 2022 06:15:48 CEST Al Viro wrote: > that one is somewhat clumsier than usual and needs serious testing. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Hi Al, it took me a bit to find the patch that introduces iov_iter_get_pages_alloc2(), but this patch itself looks fine: Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> Please give me some days for thorough testing. We recently had 9p broken (with cache=loose) for half a year, so I would like to avoid repetition. Best regards, Christian Schoenebeck > --- > 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); > > 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); > > 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; > } > }
On Freitag, 1. Juli 2022 15:47:24 CEST Christian Schoenebeck wrote: > On Mittwoch, 22. Juni 2022 06:15:48 CEST Al Viro wrote: > > that one is somewhat clumsier than usual and needs serious testing. > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > Hi Al, > > it took me a bit to find the patch that introduces > iov_iter_get_pages_alloc2(), but this patch itself looks fine: > > Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com> Tested-by: Christian Schoenebeck <linux_oss@crudebyte.com>
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); 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); 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; } }
that one is somewhat clumsier than usual and needs serious testing. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- net/9p/client.c | 39 +++++++++++++++++++++++---------------- net/9p/protocol.c | 3 +-- net/9p/trans_virtio.c | 3 ++- 3 files changed, 26 insertions(+), 19 deletions(-)