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_...@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 :-)
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
