Page MenuHomeFreeBSD

kqueue: Fix a race when adding an fd-based knote to a queue
ClosedPublic

Authored by markj on Sat, Mar 14, 2:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 29, 11:57 PM
Unknown Object (File)
Thu, Mar 26, 6:10 AM
Unknown Object (File)
Thu, Mar 26, 6:09 AM
Unknown Object (File)
Tue, Mar 24, 8:19 AM
Unknown Object (File)
Mon, Mar 23, 8:05 AM
Unknown Object (File)
Mon, Mar 23, 8:05 AM
Unknown Object (File)
Thu, Mar 19, 2:36 AM
Unknown Object (File)
Wed, Mar 18, 8:26 PM
Subscribers

Details

Summary

When registering a new kevent backed by a file descriptor, we first look
up the file description with fget(), then lock the kqueue, then see if a
corresponding knote is already registered. If not, and KN_ADD is
specified, we add the knote to the kqueue.

closefp_impl() interlocks with this process by calling knote_fdclose(),
which locks each kqueue and checks to see if the fd is registered with a
knote. But, if userspace closes an fd while a different thread is
registering it, i.e., after fget() succeeds but before the kqueue is
locked, then we may end up with a mismatch in the knote table, where the
knote kn_fp field points to a different file description than the knote
ident.

Fix the problem by double-checking before registering a knote. Add a
new fget_noref_unlocked() helper for this purpose. It is a clone of
fget_noref(). We could simply use fget_noref(), but I like having an
explicit unlocked variant.

PR: 293382

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 71572
Build 68455: arc lint + arc unit

Event Timeline

markj requested review of this revision.Sat, Mar 14, 2:21 AM
sys/kern/kern_event.c
1810

Should we check that fops->f_isfd is true?

markj marked an inline comment as done.

Add an isfd check

kib added inline comments.
sys/sys/filedesc.h
308

I suggest to add some comment in the spirit that there should be some external factors that keep the result of this function valid. Otherwise, the function must not be used.

This revision is now accepted and ready to land.Sun, Mar 15, 7:03 AM
kevans added inline comments.
sys/kern/kern_event.c
1813

I believe you want tkn = kn here to avoid leaking the knote that was just allocated

This revision now requires review to proceed.Fri, Mar 20, 6:23 AM
markj marked 2 inline comments as done.

Add a comment.

This revision is now accepted and ready to land.Fri, Mar 20, 4:58 PM