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.
> > 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.
