Re: [PATCH] 9pfs: fix 'total_open_fd' decrementation

[Greg Kurz <groug@kaod.org>]

On Thu, 20 Mar 2025 10:48:11 +0100
Christian Schoenebeck <qemu_oss@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_oss@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-groug@kaod.org>
> > > > Signed-off-by: Christian Schoenebeck <qemu_oss@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