From: Vitaliy Makkoveev Subject: Re: Remove fd_checkclosed To: Visa Hankala Cc: tech@openbsd.org Date: Mon, 24 Nov 2025 21:56:34 +0300 On Mon, Nov 24, 2025 at 09:42:09PM +0300, Vitaliy Makkoveev wrote: > 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. > Ooops, The diff is wrong, it is NOT ok mvs. There is no ordering problem with concurrent close()/dup() but the use-after-free issue caused by concurrent fdexpand(). It replaces the dynamically allocated `fd_ofiles' with the new one. Unlocked 'fdp->fd_ofiles' load returns the address of already released memory. Sorry for noise.