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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Thursday, March 20, 2025 11:59:38 AM CET Greg Kurz wrote:
> 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 :-)

I think in this case it's better to just log this case. I'll go for a big fat
warning though:

    /* 'man 2 close' suggests to ignore close() errors except of EBADF */
    if (unlikely(err && errno == EBADF)) {
        /* unexpected case as we should have checked for a valid file handle */
        error_report("9pfs: WARNING: v9fs_co_close() failed with EBADF");
    } else {
        total_open_fd--;
    }

That's because I currently don't see how this could be exploited, and assert()
would promote this case to a DoS, which I think is not justified.

I ran some tests here, with assert() that is, and at least it never triggered
for me.

So I say let's go this way, the error should be prominent enough, note that's
error_report(), not error_report_once(). So if people are able to trigger 
this, I am sure they'll annoyed enough to report it. On the long term this
could still be promoted to an assert().

/Christian