hw/9pfs/9p.c | 14 ++++++++------ hw/9pfs/codir.c | 3 ++- hw/9pfs/cofile.c | 3 ++- 3 files changed, 12 insertions(+), 8 deletions(-)
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;
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;
--
2.39.5On 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?
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
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
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.
> > 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.
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
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
© 2016 - 2025 Red Hat, Inc.