| Message ID | 20191122200034.122564-1-dschatzberg@fb.com |
|---|---|
| State | New |
| Headers | show |
| Series | 9pfs: Fix divide by zero bug | expand |
On Freitag, 22. November 2019 21:00:34 CET Dan Schatzberg wrote: > Some filesystems may return 0s in statfs (trivially, a FUSE filesystem > can do so). QEMU should handle this gracefully and just behave the > same as if statfs failed. Is that actually legal in non-error cases? Shouldn't a driver without a block size concept return 512 according to POSIX? > Signed-off-by: Dan Schatzberg <dschatzberg@fb.com> > --- > hw/9pfs/9p.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index 37abcdb71e..520177f40c 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1834,8 +1834,10 @@ static int32_t coroutine_fn get_iounit(V9fsPDU *pdu, > V9fsPath *path) * and as well as less than (client msize - P9_IOHDRSZ)) > */ > if (!v9fs_co_statfs(pdu, path, &stbuf)) { > - iounit = stbuf.f_bsize; > - iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize; > + if (stbuf.f_bsize) { > + iounit = stbuf.f_bsize; > + iounit *= (s->msize - P9_IOHDRSZ) / stbuf.f_bsize; > + } > } > if (!iounit) { > iounit = s->msize - P9_IOHDRSZ; Nevertheless, since that will leave iounit initialized with zero and since there is already an !ionunit case handling there ... Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com> Best regards, Christian Schoenebeck
On Fri, 22 Nov 2019 22:53:09 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Freitag, 22. November 2019 21:00:34 CET Dan Schatzberg wrote: > > Some filesystems may return 0s in statfs (trivially, a FUSE filesystem > > can do so). QEMU should handle this gracefully and just behave the > > same as if statfs failed. > > Is that actually legal in non-error cases? Shouldn't a driver without a block > size concept return 512 according to POSIX? > The first problem I see is that statfs() isn't POSIX. It is a linux-specific implementation inspired by 4.4BSD. The equivalent system call in POSIX would be statvfs(): https://pubs.opengroup.org/onlinepubs/9699919799/functions/statvfs.html And even there, no details are provided about what f_bsize should contain, apart from: "It is unspecified whether all members of the statvfs structure have meaningful values on all file systems." A filesystem isn't necessarily backed by a block device, so I guess it is legal to get a zero block size and we should definitely cope with it. > > Signed-off-by: Dan Schatzberg <dschatzberg@fb.com> > > --- > > hw/9pfs/9p.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index 37abcdb71e..520177f40c 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -1834,8 +1834,10 @@ static int32_t coroutine_fn get_iounit(V9fsPDU *pdu, > > V9fsPath *path) * and as well as less than (client msize - P9_IOHDRSZ)) > > */ > > if (!v9fs_co_statfs(pdu, path, &stbuf)) { > > - iounit = stbuf.f_bsize; > > - iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize; > > + if (stbuf.f_bsize) { > > + iounit = stbuf.f_bsize; > > + iounit *= (s->msize - P9_IOHDRSZ) / stbuf.f_bsize; > > + } > > } > > if (!iounit) { > > iounit = s->msize - P9_IOHDRSZ; > > Nevertheless, since that will leave iounit initialized with zero and since > there is already an !ionunit case handling there ... > This fix looks like the only sensible thing to do. > Acked-by: Christian Schoenebeck <qemu_oss@crudebyte.com> > Thanks for the Cc otherwise I would have certainly missed this one :) I've pushed it to my 9p-fix branch and I'll send a pull request ASAP in order to have this fixed in rc3. > Best regards, > Christian Schoenebeck > >
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 37abcdb71e..520177f40c 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1834,8 +1834,10 @@ static int32_t coroutine_fn get_iounit(V9fsPDU *pdu, V9fsPath *path) * and as well as less than (client msize - P9_IOHDRSZ)) */ if (!v9fs_co_statfs(pdu, path, &stbuf)) { - iounit = stbuf.f_bsize; - iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize; + if (stbuf.f_bsize) { + iounit = stbuf.f_bsize; + iounit *= (s->msize - P9_IOHDRSZ) / stbuf.f_bsize; + } } if (!iounit) { iounit = s->msize - P9_IOHDRSZ;
Some filesystems may return 0s in statfs (trivially, a FUSE filesystem can do so). QEMU should handle this gracefully and just behave the same as if statfs failed. Signed-off-by: Dan Schatzberg <dschatzberg@fb.com> --- hw/9pfs/9p.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)