From: Visa Hankala <visa@hankala.org>
Subject: Re: Remove fd_checkclosed
To: tech@openbsd.org
Date: Mon, 24 Nov 2025 17:37:09 +0000

On Mon, Nov 24, 2025 at 05:21:19AM +0000, Tim Leslie wrote:
> 
> All,
> 
> fd_checkclosed() is only used in two places, both as a best‑effort race detector rather than a strong synchronization point. In sys_fcntl()’s F_SETLK/F_SETLKW path, it is called after VOP_ADVLOCK() to see if close() or dup2() won the race and then issue a compensating F_UNLCK if so. In kqueue_kqfilter(), fd filters use it to check whether knote_fdclose() might have missed the knote while it was being attached.
> 
> In both cases, the caller already holds a reference on the struct file obtained earlier from fd_getfile(). This patch converts the check to a direct atomic grab and then removing the helper.​
> 
> There is no intended semantic change beyond avoiding an extra mutex acquisition around what is already documented as a racy snapshot in both call sites.​

If the check is done without the mutex, what will ensure proper
ordering when there is a concurrent close/dup call?

> 
> —
> Tim Leslie
> 
> diff --git a/share/man/man9/file.9 b/share/man/man9/file.9
> --- a/share/man/man9/file.9
> +++ b/share/man/man9/file.9
> @@ -32,7 +32,6 @@
>  .Nm FRELE ,
>  .Nm fd_getfile ,
>  .Nm fd_getfile_mode ,
> -.Nm fd_checkclosed ,
>  .Nm getsock ,
>  .Nm getvnode
>  .Nd an overview of file descriptor handling
> @@ -52,8 +51,6 @@
>  .Ft struct file *
>  .Fn fd_getfile_mode "struct filedesc *fdp" "int fd" "int mode"
>  .Ft int
> -.Fn fd_checkclosed "struct filedesc *fdp" "int fd" "struct file *fp"
> -.Ft int
>  .Fn getsock "struct proc *p" "int fd" "struct file **fpp"
>  .In sys/file.h
>  .In sys/filedesc.h
> @@ -112,13 +109,6 @@ is like
>  .Fn fd_getfile
>  but also checks if the file has been opened with the given mode.
>  .Pp
> -.Fn fd_checkclosed
> -checks if file descriptor
> -.Fa fd
> -has been closed and no longer points to file
> -.Fa fp .
> -The file must have been retrieved from the descriptor previously.
> -.Pp
>  The files are extracted from the process context using the
>  function
>  .Fn getsock
> diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
> --- a/sys/kern/kern_descrip.c
> +++ b/sys/kern/kern_descrip.c
> @@ -262,18 +262,6 @@ fd_getfile_mode(struct filedesc *fdp, int fd, int mode)
>  	return (fp);
>  }
> 
> -int
> -fd_checkclosed(struct filedesc *fdp, int fd, struct file *fp)
> -{
> -	int closed;
> -
> -	mtx_enter(&fdp->fd_fplock);
> -	KASSERT(fd < fdp->fd_nfiles);
> -	closed = (fdp->fd_ofiles[fd] != fp);
> -	mtx_leave(&fdp->fd_fplock);
> -	return (closed);
> -}
> -
>  /*
>   * System calls on descriptors.
>   */
> @@ -571,7 +559,7 @@ restart:
>  			goto out;
>  		}
> 
> -		if (fd_checkclosed(fdp, fd, fp)) {
> +		if (READ_ONCE(fdp->fd_ofiles[fd]) != fp) {
>  			/*
>  			 * We have lost the race with close() or dup2();
>  			 * unlock, pretend that we've won the race and that
> diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
> --- a/sys/kern/kern_event.c
> +++ b/sys/kern/kern_event.c
> @@ -1501,7 +1501,7 @@ again:
>  			 * ran before kn appeared in kq_knlist.
>  			 */
>  			if ((fops->f_flags & FILTEROP_ISFD) &&
> -			    fd_checkclosed(fdp, kev->ident, kn->kn_fp)) {
> +			    READ_ONCE(fdp->fd_ofiles[kev->ident]) != kn->kn_fp) {
>  				/*
>  				 * Drop the knote silently without error
>  				 * because another thread might already have
> diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
> --- a/sys/sys/filedesc.h
> +++ b/sys/sys/filedesc.h
> @@ -148,7 +148,6 @@ void	fdprepforexec(struct proc *);
>  struct file *fd_iterfile(struct file *, struct proc *);
>  struct file *fd_getfile(struct filedesc *, int);
>  struct file *fd_getfile_mode(struct filedesc *, int, int);
> -int	fd_checkclosed(struct filedesc *, int, struct file *);
> 
>  int	closef(struct file *, struct proc *);
>  int	getsock(struct proc *, int, struct file **);
>