| Message ID | 3990d3891e8ae2074709b56449e96ab4b4b93b7d.1579567020.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: readdir optimization | expand |
On Tue, 21 Jan 2020 00:50:33 +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" transport error message by > bad clients. This count value constraint uses msize - 11, because 11 is the > header size of R_readdir. > This would be worth a comment... > 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..18370183c4 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); > ... here. Something like: /* Enough space for a R_readdir header: size[4] Rreaddir tag[2] count[4] */ I can fix this in my tree, and actually done so since I've applied patches 1 to 3. Reviewed-by: Greg Kurz <groug@kaod.org> > + if (max_count > s->msize - 11) { > + max_count = s->msize - 11; > + warn_report_once( > + "9p: bad client: T_readdir with count > msize - 11" > + ); > + } > + > fidp = get_fid(pdu, fid); > if (fidp == NULL) { > retval = -EINVAL;
On Mittwoch, 22. Januar 2020 15:11:07 CET Greg Kurz wrote: > On Tue, 21 Jan 2020 00:50:33 +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" transport error message by > > bad clients. This count value constraint uses msize - 11, because 11 is > > the > > header size of R_readdir. > > This would be worth a comment... > > > 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..18370183c4 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); > > ... here. Something like: > > /* Enough space for a R_readdir header: size[4] Rreaddir tag[2] count[4] > */ > > I can fix this in my tree, and actually done so since I've > applied patches 1 to 3. Fine with me, thanks Greg! > Reviewed-by: Greg Kurz <groug@kaod.org> Best regards, Christian Schoenebeck
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index a5fbe821d4..18370183c4 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 - 11) { + max_count = s->msize - 11; + warn_report_once( + "9p: bad client: T_readdir with count > msize - 11" + ); + } + 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" transport error message by bad clients. This count value constraint uses msize - 11, because 11 is the header size of R_readdir. Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com> --- hw/9pfs/9p.c | 8 ++++++++ 1 file changed, 8 insertions(+)