mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Mikko Rantalainen <mikko.rantalainen@peda.net>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
	linux-kernel@vger.kernel.org, brauner@kernel.org,
	viro@zeniv.linux.org.uk, jack@suse.cz, alx@kernel.org,
	dalias@libc.org
Subject: Re: [RFC PATCH 0/1] close(): stop exposing non-retryable EINTR
Date: Mon, 14 Sep 2026 15:40:01 +0300	[thread overview]
Message-ID: <cf83be66-e540-4abf-b377-7e344d62a1dd@peda.net> (raw)
In-Reply-To: <bcdb8f85-d53a-4824-8e34-8d190ec7f4b2@peda.net>

Mikko Rantalainen (2026-09-14 12:07 Europe/Helsinki):
> ---
> retval = filp_flush(file, current->files);
> 
> WARN_ONCE(retval == -EINTR ||
>           retval == -ERESTARTSYS ||
>           retval == -ERESTARTNOINTR ||
>           retval == -ERESTARTNOHAND ||
>           retval == -ERESTART_RESTARTBLOCK,
>           "close: ->flush %ps returned interrupt error %d\n",
>           file->f_op->flush, retval);
> ---
> 
> That would leave the existing userspace ABI unchanged while making
> remaining offending implementations easier to find and fix.
> 
> I also considered retrying filp_flush() inside close(), but I don't
> think that can be done generically. ->flush() is not documented as
> safe to restart from the beginning after partial execution, and
> an interruptible wait could immediately encounter the same
> still-pending signal again. So fixing the interruptibility at the
> offending wait seems safer if the above invariant is indeed
> the intended one.

Another thing I noticed is that there are already several paths where
the kernel calls filp_close() and intentionally ignores its return value.

For example, close_files() does:

    filp_close(file, files);

without checking the result. The same is true for do_close_on_exec(),
and close_range() explicitly says:

    Currently, errors to close a given file descriptor are ignored.

So I don't think a ->flush() implementation can rely on returning EINTR
and having somebody retry the interrupted operation. There are valid
close paths where nobody will ever see that return value, even when the
process itself continues running.

This seems to strengthen Matthew's point: if some work performed by
->flush() is required for correctness, that work has to tolerate these
close paths without depending on userspace retry. Returning an
interruption result cannot be the recovery mechanism.

I'm therefore leaning towards treating an observable -EINTR/-ERESTART*
from ->flush() as suspicious in general, rather than just special-casing
the close(2) syscall. The fatal-signal case is harmless because the task
will not observe the result, but close-on-exec and close_range() show
that unobserved filp_close() errors are already part of normal operation
as well.

That also makes me think documenting the intended ->flush() contract
would be useful: if required close-time work must not depend on the
caller retrying filp_close(), that seems like an important invariant for
implementations to know.

What guarantees must file_operations::flush provide when its caller may
have no way to act on its return value?

In any case, I'm now thinking that returning EINTR for close() is a bug
when file descriptor is already freed. I think the only question is how
it should be solved. I initially thought it should just be mapped to
success. Maybe it should be logged as subsystem bug *and* mapped to
success for userspace instead?

-- 
Mikko

      reply	other threads:[~2026-09-14 12:40 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-13 19:38 Mikko Rantalainen
2026-09-13 19:38 ` [RFC PATCH 1/1] fs: don't return EINTR from close() Mikko Rantalainen
2026-09-13 20:53 ` [RFC PATCH 0/1] close(): stop exposing non-retryable EINTR Rich Felker
2026-09-13 21:27   ` Alejandro Colomar
2026-09-14  6:36   ` Mikko Rantalainen
2026-09-14  9:44     ` Alejandro Colomar
2026-09-13 22:42 ` Matthew Wilcox
2026-09-13 23:51   ` Rich Felker
2026-09-14  9:38     ` Mikko Rantalainen
2026-09-14  9:07   ` Mikko Rantalainen
2026-09-14 12:40     ` Mikko Rantalainen [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cf83be66-e540-4abf-b377-7e344d62a1dd@peda.net \
    --to=mikko.rantalainen@peda.net \
    --cc=alx@kernel.org \
    --cc=brauner@kernel.org \
    --cc=dalias@libc.org \
    --cc=jack@suse.cz \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®