| Message ID | 0edf21d2cb2a6cf22ba1065bf451b44ad5962cbd.1578957500.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: readdir optimization | expand |
On Mon, 13 Jan 2020 23:22:08 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > A good 9p client sends T_readdir with "count" parameter that's > sufficiently smaller than client's initially negotiated msize > (maximum message size). We perform a check for that though to > avoid the server to be interrupted with a "Failed to encode > VirtFS reply type 41" error message by bad clients. > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > --- > hw/9pfs/9p.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index a5fbe821d4..30da2fedf3 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -2426,6 +2426,7 @@ static void coroutine_fn v9fs_readdir(void *opaque) > int32_t count; > uint32_t max_count; > V9fsPDU *pdu = opaque; > + V9fsState *s = pdu->s; > > retval = pdu_unmarshal(pdu, offset, "dqd", &fid, > &initial_offset, &max_count); > @@ -2434,6 +2435,13 @@ static void coroutine_fn v9fs_readdir(void *opaque) > } > trace_v9fs_readdir(pdu->tag, pdu->id, fid, initial_offset, max_count); > > + if (max_count > s->msize - P9_IOHDRSZ) { P9_IOHDRSZ relates to Twrite. The Rreaddir message has a smaller header of size 11: size[4] Rreaddir tag[2] count[4] > + max_count = s->msize - P9_IOHDRSZ; > + warn_report_once( > + "9p: bad client: T_readdir with count > msize - P9_IOHDRSZ" > + ); > + } > + > fidp = get_fid(pdu, fid); > if (fidp == NULL) { > retval = -EINVAL;
On Donnerstag, 16. Januar 2020 14:33:42 CET Greg Kurz wrote: > On Mon, 13 Jan 2020 23:22:08 +0100 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > A good 9p client sends T_readdir with "count" parameter that's > > sufficiently smaller than client's initially negotiated msize > > (maximum message size). We perform a check for that though to > > avoid the server to be interrupted with a "Failed to encode > > VirtFS reply type 41" error message by bad clients. > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > --- > > > > hw/9pfs/9p.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index a5fbe821d4..30da2fedf3 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -2426,6 +2426,7 @@ static void coroutine_fn v9fs_readdir(void *opaque) > > > > int32_t count; > > uint32_t max_count; > > V9fsPDU *pdu = opaque; > > > > + V9fsState *s = pdu->s; > > > > retval = pdu_unmarshal(pdu, offset, "dqd", &fid, > > > > &initial_offset, &max_count); > > > > @@ -2434,6 +2435,13 @@ static void coroutine_fn v9fs_readdir(void *opaque) > > > > } > > trace_v9fs_readdir(pdu->tag, pdu->id, fid, initial_offset, > > max_count); > > > > + if (max_count > s->msize - P9_IOHDRSZ) { > > P9_IOHDRSZ relates to Twrite. The Rreaddir message has a smaller header > of size 11: > > size[4] Rreaddir tag[2] count[4] Right, looks like I have falsely picked P9_IOHDRSZ after looking at: static size_t v9fs_readdir_data_size(V9fsString *name) { /* * Size of each dirent on the wire: size of qid (13) + size of offset (8) * size of type (1) + size of name.size (2) + strlen(name.data) */ return 24 + v9fs_string_size(name); } I'll have to correct that in the test cases as well. So no need to comment on them for now. But if you have an idea about the issue mentioned in cover letter (patch 7), let me know. I have a feeling that there is some problem with the test environment, because I also get strange error messages when I just add some more e.g. noop 9pfs test cases (empty test cases doing nothing) or by copy pasting existing tests and then running tests/qos-test -l which obviously should just list the test cases, but not executing any of them. I'd end up with "cannot push stack" error messages for some reason. > > + max_count = s->msize - P9_IOHDRSZ; > > + warn_report_once( > > + "9p: bad client: T_readdir with count > msize - P9_IOHDRSZ" > > + ); > > + } > > + > > > > fidp = get_fid(pdu, fid); > > if (fidp == NULL) { > > > > retval = -EINVAL; Best regards, Christian Schoenebeck
On Thu, 16 Jan 2020 17:51:10 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Donnerstag, 16. Januar 2020 14:33:42 CET Greg Kurz wrote: > > On Mon, 13 Jan 2020 23:22:08 +0100 > > > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > > A good 9p client sends T_readdir with "count" parameter that's > > > sufficiently smaller than client's initially negotiated msize > > > (maximum message size). We perform a check for that though to > > > avoid the server to be interrupted with a "Failed to encode > > > VirtFS reply type 41" error message by bad clients. > > > > > > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > > > --- > > > > > > hw/9pfs/9p.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > > index a5fbe821d4..30da2fedf3 100644 > > > --- a/hw/9pfs/9p.c > > > +++ b/hw/9pfs/9p.c > > > @@ -2426,6 +2426,7 @@ static void coroutine_fn v9fs_readdir(void *opaque) > > > > > > int32_t count; > > > uint32_t max_count; > > > V9fsPDU *pdu = opaque; > > > > > > + V9fsState *s = pdu->s; > > > > > > retval = pdu_unmarshal(pdu, offset, "dqd", &fid, > > > > > > &initial_offset, &max_count); > > > > > > @@ -2434,6 +2435,13 @@ static void coroutine_fn v9fs_readdir(void *opaque) > > > > > > } > > > trace_v9fs_readdir(pdu->tag, pdu->id, fid, initial_offset, > > > max_count); > > > > > > + if (max_count > s->msize - P9_IOHDRSZ) { > > > > P9_IOHDRSZ relates to Twrite. The Rreaddir message has a smaller header > > of size 11: > > > > size[4] Rreaddir tag[2] count[4] > > Right, looks like I have falsely picked P9_IOHDRSZ after looking at: > > static size_t v9fs_readdir_data_size(V9fsString *name) > { > /* > * Size of each dirent on the wire: size of qid (13) + size of offset (8) > * size of type (1) + size of name.size (2) + strlen(name.data) > */ > return 24 + v9fs_string_size(name); > } > > I'll have to correct that in the test cases as well. So no need to comment on > them for now. > > But if you have an idea about the issue mentioned in cover letter (patch 7), > let me know. I have a feeling that there is some problem with the test > environment, because I also get strange error messages when I just add some > more e.g. noop 9pfs test cases (empty test cases doing nothing) or by copy > pasting existing tests and then running > > tests/qos-test -l > > which obviously should just list the test cases, but not executing any of > them. I'd end up with "cannot push stack" error messages for some reason. > No idea. I'll have to look more. > > > + max_count = s->msize - P9_IOHDRSZ; > > > + warn_report_once( > > > + "9p: bad client: T_readdir with count > msize - P9_IOHDRSZ" > > > + ); > > > + } > > > + > > > > > > fidp = get_fid(pdu, fid); > > > if (fidp == NULL) { > > > > > > retval = -EINVAL; > > Best regards, > Christian Schoenebeck > >
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index a5fbe821d4..30da2fedf3 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -2426,6 +2426,7 @@ static void coroutine_fn v9fs_readdir(void *opaque) int32_t count; uint32_t max_count; V9fsPDU *pdu = opaque; + V9fsState *s = pdu->s; retval = pdu_unmarshal(pdu, offset, "dqd", &fid, &initial_offset, &max_count); @@ -2434,6 +2435,13 @@ static void coroutine_fn v9fs_readdir(void *opaque) } trace_v9fs_readdir(pdu->tag, pdu->id, fid, initial_offset, max_count); + if (max_count > s->msize - P9_IOHDRSZ) { + max_count = s->msize - P9_IOHDRSZ; + warn_report_once( + "9p: bad client: T_readdir with count > msize - P9_IOHDRSZ" + ); + } + fidp = get_fid(pdu, fid); if (fidp == NULL) { retval = -EINVAL;
A good 9p client sends T_readdir with "count" parameter that's sufficiently smaller than client's initially negotiated msize (maximum message size). We perform a check for that though to avoid the server to be interrupted with a "Failed to encode VirtFS reply type 41" error message by bad clients. Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> --- hw/9pfs/9p.c | 8 ++++++++ 1 file changed, 8 insertions(+)