Re: [RFC PATCH 0/1] close(): stop exposing non-retryable EINTR

[Jan Kara]

On Mon 21-09-26 10:19:07, David Laight wrote:
> On Mon, 21 Sep 2026 10:49:23 +0200
> Jan Kara <jack@xxxxxxx> wrote:
> 
> > Hi!
> > 
> > On Mon 14-09-26 15:40:01, Mikko Rantalainen wrote:
> > > 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?  
> > 
> > I agree that nobody can sanely assume returning EINTR from open(2) helps
> > anything (or that userspace is able to do anything based on that).
> 
> What about opens of serial ports waiting for modem signals or opens
> of tape drives waiting for rewind, finding tape marks etc?

Err. Not sure how I wrote open(2) there. It should have been close(2)...

> > Generally any error (perhaps outside EBADF) from close(2) is at best
> > informative telling you that something is unhealthy but userspace cannot
> > sensibly do anything about it.
> 
> The program can exit with error to indicate that the output file is
> likely to be invalid.

Yes. That was covered by my paragraph below about "something is unhealthy"
message.

								Honza

> > OTOH this "something is unhealthy" message
> > does carry some value for possible debugging of the issues by sysadmin so
> > I'm not sure just ignoring the errors is the right way to go.
> > 
> > This raises a question: Are you actually seeing some case where you can see
> > EINTR returned? Because IMO the best fix is to just fix the .flush method
> > that can return EINTR to do that only in case of fatal signal and then you
> > don't have to be doing this special-casing in VFS.
> > 
> > 								Honza
> > 
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR