PR/57260 CVS commit: src/sys/kern

["Taylor R Campbell" <riastradh%netbsd.org@localhost>]

The following reply was made to PR kern/57260; it has been noted by GNATS.

From: "Taylor R Campbell" <riastradh%netbsd.org@localhost>
To: gnats-bugs%gnats.NetBSD.org@localhost
Cc: 
Subject: PR/57260 CVS commit: src/sys/kern
Date: Sun, 5 Mar 2023 14:40:32 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sun Mar  5 14:40:32 UTC 2023
 
 Modified Files:
 	src/sys/kern: vfs_syscalls.c
 
 Log Message:
 open(2): Don't map ERESTART to EINTR.
 
 If a file or device's open function returns ERESTART, respect that --
 restart the syscall; don't pretend a signal has been delivered when
 it was not.  If an SA_RESTART signal was delivered, POSIX does not
 allow it to fail with EINTR:
 
     SA_RESTART
         This flag affects the behavior of interruptible functions;
         that is, those specified to fail with errno set to [EINTR].
         If set, and a function specified as interruptible is
         interrupted by this signal, the function shall restart and
         shall not fail with [EINTR] unless otherwise specified.  If
         an interruptible function which uses a timeout is restarted,
         the duration of the timeout following the restart is set to
         an unspecified value that does not exceed the original
         timeout value.  If the flag is not set, interruptible
         functions interrupted by this signal shall fail with errno
         set to [EINTR].
 
 https://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaction.html
 
 Nothing in the POSIX definition of open specifies otherwise.
 
 In 1990, Kirk McKusick added these lines with a mysterious commit
 message:
 
 Author: Kirk McKusick <mckusick>
 Date:   Tue Apr 10 19:36:33 1990 -0800
 
     eliminate longjmp from the kernel (for karels)
 
 diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
 index 7bc7b39bbf..d572d3a32d 100644
 --- a/sys/kern/vfs_syscalls.c
 +++ b/sys/kern/vfs_syscalls.c
 @@ -14,7 +14,7 @@
   * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
   * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE.
   *
 - *	@(#)vfs_syscalls.c	7.42 (Berkeley) 3/26/90
 + *	@(#)vfs_syscalls.c	7.43 (Berkeley) 4/10/90
   */
 
  #include "param.h"
 @@ -530,8 +530,10 @@ copen(scp, fmode, cmode, ndp, resultfd)
  	if (error = vn_open(ndp, fmode, (cmode & 07777) &~ S_ISVTX)) {
  		crfree(fp->f_cred);
  		fp->f_count--;
 -		if (error == -1)	/* XXX from fdopen */
 -			return (0);	/* XXX from fdopen */
 +		if (error == EJUSTRETURN)	/* XXX from fdopen */
 +			return (0);		/* XXX from fdopen */
 +		if (error == ERESTART)
 +			error = EINTR;
  		scp->sc_ofile[indx] = NULL;
  		return (error);
  	}
 
 (found via this git import of the CSRG history:
 https://github.com/robohack/ucb-csrg-bsd/commit/cce2869b7ae5d360921eb411005b328a29c4a3fe)
 
 This change appears to have served two related purposes:
 
 1. The fdopen function (the erstwhile open routine for /dev/fd/N)
    used to return -1 as a hack to mean it had just duplicated the fd;
    it was recently changed by Mike Karels, in kern_descrip.c 7.9, to
    return EJUSTRETURN, now defined to be -2, presumably to avoid a
    conflict with ERESTART, defined to be -1.  So this change finished
    part of the change by Mike Karels to use a different magic return
    code from fdopen.
 
    Of course, today we use still another disgusting hack, EDUPFD, for
    the same purpose, so none of this is relevant any more.
 
 2. Prior to April 1990, the kernel handled signals during tsleep(9)
    by longjmping out to the system call entry point or similar.  In
    April 1990, Mike Karels worked to convert all of that into
    explicit unwind logic by passing through EINTR or ERESTART as
    appropriate, instead of setjmp at each entry point.
 
 However, it's not clear to me why this setjmp/longjmp and
 fdopen/-1/EJUSTRETURN renovation justifies unconditional logic to map
 ERESTART to EINTR in open(2).  I suspect it was a mistake.
 
 In 2013, the corresponding logic to map ERESTART to EINTR in open(2)
 was removed from FreeBSD:
 
    r246472 | kib | 2013-02-07 14:53:33 +0000 (Thu, 07 Feb 2013) | 11 lines
 
    Stop translating the ERESTART error from the open(2) into EINTR.
    Posix requires that open(2) is restartable for SA_RESTART.
 
    For non-posix objects, in particular, devfs nodes, still disable
    automatic restart of the opens. The open call to a driver could have
    significant side effects for the hardware.
 
    Noted and reviewed by:  jilles
    Discussed with: bde
    MFC after:      2 weeks
 
 Index: vfs_syscalls.c
 ===================================================================
 --- vfs_syscalls.c	(revision 246471)
 +++ vfs_syscalls.c	(revision 246472)
 @@ -1106,8 +1106,6 @@
  				goto success;
  		}
 
 -		if (error == ERESTART)
 -			error = EINTR;
  		goto bad;
  	}
  	td->td_dupfd = 0;
 
 https://cgit.freebsd.org/src/commit/sys/kern/vfs_syscalls.c?id=2ca49983425886121b506cb5126b60a705afc38c
 
 It's not clear to me that there's any reason to treat device nodes
 specially here; in fact, if a driver's .d_open routine sleeps and is
 woken by a concurrent revoke without a signal pending or with an
 SA_RESTART signal pending, it is wrong for it to fail with EINTR.
 But it MUST restart the whole system call rather than continue
 sleeping in a loop or just exit the loop and continue to open,
 because it is mandatory in the security model of revoke for open(2)
 to retry the permissions check at that point.
 
 PR kern/57260
 
 XXX pullup-8
 XXX pullup-9
 XXX pullup-10
 
 
 To generate a diff of this commit:
 cvs rdiff -u -r1.556 -r1.557 src/sys/kern/vfs_syscalls.c
 
 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.