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

[Greg Kurz <groug@kaod.org>]

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/9p.c b/hw/9pfs/9p.c
> > index b22df3aa2b..f4ca8e4db5 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -434,7 +434,6 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> >      V9fsFidState *f;
> >      GHashTableIter iter;
> >      gpointer fid;
> > -    int err;
> >      int nclosed = 0;
> >  
> >      /* prevent multiple coroutines running this function simultaniously */
> > @@ -507,13 +506,16 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> >       */
> >      v9fs_co_run_in_worker({
> >          QSLIST_FOREACH(f, &reclaim_list, reclaim_next) {
> > -            err = (f->fid_type == P9_FID_DIR) ?
> > +            /*
> > +             * 'man 2 close' suggests to ignore close() errors except of EBADF,
> > +             * not checking for EBADF here either as FIDs were picked above by
> > +             * having a valid file descriptor
> > +             */
> > +            (f->fid_type == P9_FID_DIR) ?
> >                  s->ops->closedir(&s->ctx, &f->fs_reclaim) :
> >                  s->ops->close(&s->ctx, &f->fs_reclaim);
> > -            if (!err) {
> > -                /* total_open_fd must only be mutated on main thread */
> > -                nclosed++;
> > -            }
> > +            /* total_open_fd must only be mutated on main thread */
> > +            nclosed++;
> >          }
> >      });
> >      total_open_fd -= nclosed;
> > 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().

>     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.
> 
> > diff --git a/hw/9pfs/cofile.c b/hw/9pfs/cofile.c
> > index 71174c3e4a..1e9f6da42a 100644
> > --- a/hw/9pfs/cofile.c
> > +++ b/hw/9pfs/cofile.c
> > @@ -197,7 +197,8 @@ int coroutine_fn v9fs_co_close(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;
> 
> Same here then.
> 
> /Christian
> 
> 

-- 
Greg