From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: Remove fd_checkclosed
To: Visa Hankala <visa@hankala.org>
Cc: tech@openbsd.org
Date: Mon, 24 Nov 2025 21:42:09 +0300

On Mon, Nov 24, 2025 at 05:37:09PM +0000, Visa Hankala wrote:
> 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?
> 

Concurrent thread will override fdp->fd_ofiles[fd] just after
`fd_fplock' mutex release, but the old value will be used. Also, the
compiler will not reorder conditions in 'if' block.

There is no order problems here. This diff looks ok by me.

> > 
> > —
> > 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 **);
> > 
>