On Thu, 20 Mar 2025 10:48:11 +0100
Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> On Wednesday, March 19, 2025 7:52:51 PM CET Greg Kurz wrote:
> > On Wed, 19 Mar 2025 13:14:27 +0100
> > Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> >
> > > On Wednesday, March 19, 2025 11:08:58 AM CET Christian Schoenebeck wrote:
> > > > According to 'man 2 close' errors returned by close() should only be
> > > > used
> > > > for either diagnostic purposes or for catching data loss due to a
> > > > previous
> > > > write error, as an error result of close() usually indicates a deferred
> > > > error of a previous write operation.
> > > >
> > > > Therefore not decrementing 'total_open_fd' on a close() error is wrong
> > > > and would yield in a higher open file descriptor count than actually the
> > > > case, leading to 9p server reclaiming open file descriptors too soon.
> > > >
> > > > Based-on: <20250312152933.383967-7-gr...@kaod.org>
> > > > Signed-off-by: Christian Schoenebeck <qemu_...@crudebyte.com>
> > > > ---
> > > > hw/9pfs/9p.c | 14 ++++++++------
> > > > hw/9pfs/codir.c | 3 ++-
> > > > hw/9pfs/cofile.c | 3 ++-
> > > > 3 files changed, 12 insertions(+), 8 deletions(-)
> [...]
> > > > diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
> > > > index 2068a4779d..f1fd97c8a7 100644
> > > > --- a/hw/9pfs/codir.c
> > > > +++ b/hw/9pfs/codir.c
> > > > @@ -353,7 +353,8 @@ int coroutine_fn v9fs_co_closedir(V9fsPDU *pdu,
> > > > V9fsFidOpenState *fs)
> > > > err = -errno;
> > > > }
> > > > });
> > > > - if (!err) {
> > > > + /* 'man 2 close' suggests to ignore close() errors except of EBADF
> > > > */
> > > > + if (!err || errno != EBADF) {
> > > > total_open_fd--;
> > > > }
> > > > return err;
> > >
> > > Or, as EBADF is somewhat unexpected here (assuming v9fs_co_closedir() was
> > > called by checking for a valid file handle), maybe it would make sense to
> > > log
> > > this?
> > >
> >
> > Getting EBADF could be the result of some unrelated code that closed
> > the fd from another thread or the 9p code using some stale fid structure
> > or some other serious bug. I'd personally g_assert().
>
> Wouldn't that be too harsh? Killing QEMU should be last resort if continuing
> to run resulted in a security threat or undefined behaviour. I'm not sure that
> would apply here.
>
Getting EBADF on a file descriptor this code is supposed to own already
smells like undefined behavior IMHO and, hopefully, such an assert should
never trigger, but I understand your concern and it's up to you to decide :-)
> > > if (unlikely(err && errno == EBADF)) {
> > > error_report("v9fs_co_closedir() failed with EBADF");
> > > } else {
> > > total_open_fd--;
> > > }
> > >
> > > In the sense, if EBADF happens here, it's an indication for a bug in 9p
> > > server.
>
>
--
Greg
