A task can end up indefinitely sleeping in do_select() -> poll_schedule_timeout() when the following race happens:
TASK1 (thread1) TASK2 TASK1 (thread2) do_select() setup poll_wqueues table with 'fd' write data to 'fd' pollwake() table->triggered = 1 closes 'fd' thread1 is waiting for poll_schedule_timeout() - sees table->triggered table->triggered = 0 return -EINTR loop back in do_select() but fdget() in the setup of poll_wqueues fails now so we never find 'fd' is ready for reading and sleep in poll_schedule_timeout() indefinitely.
Treat fd that got closed as a fd on which some event happened. This makes sure cannot block indefinitely in do_select().
Another option would be to return -EBADF in this case but that has a potential of subtly breaking applications that excercise this behavior and it happens to work for them. So returning fd as active seems like a safer choice.
Suggested-by: Linus Torvalds torvalds@linux-foundation.org CC: stable@vger.kernel.org Signed-off-by: Jan Kara jack@suse.cz --- fs/select.c | 63 ++++++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 30 deletions(-)
diff --git a/fs/select.c b/fs/select.c index 945896d0ac9e..5edffee1162c 100644 --- a/fs/select.c +++ b/fs/select.c @@ -458,9 +458,11 @@ static int max_select_fd(unsigned long n, fd_set_bits *fds) return max; }
-#define POLLIN_SET (EPOLLRDNORM | EPOLLRDBAND | EPOLLIN | EPOLLHUP | EPOLLERR) -#define POLLOUT_SET (EPOLLWRBAND | EPOLLWRNORM | EPOLLOUT | EPOLLERR) -#define POLLEX_SET (EPOLLPRI) +#define POLLIN_SET (EPOLLRDNORM | EPOLLRDBAND | EPOLLIN | EPOLLHUP | EPOLLERR |\ + EPOLLNVAL) +#define POLLOUT_SET (EPOLLWRBAND | EPOLLWRNORM | EPOLLOUT | EPOLLERR |\ + EPOLLNVAL) +#define POLLEX_SET (EPOLLPRI | EPOLLNVAL)
static inline void wait_key_set(poll_table *wait, unsigned long in, unsigned long out, unsigned long bit, @@ -527,6 +529,7 @@ static int do_select(int n, fd_set_bits *fds, struct timespec64 *end_time) break; if (!(bit & all_bits)) continue; + mask = EPOLLNVAL; f = fdget(i); if (f.file) { wait_key_set(wait, in, out, bit, @@ -534,34 +537,34 @@ static int do_select(int n, fd_set_bits *fds, struct timespec64 *end_time) mask = vfs_poll(f.file, wait);
fdput(f); - if ((mask & POLLIN_SET) && (in & bit)) { - res_in |= bit; - retval++; - wait->_qproc = NULL; - } - if ((mask & POLLOUT_SET) && (out & bit)) { - res_out |= bit; - retval++; - wait->_qproc = NULL; - } - if ((mask & POLLEX_SET) && (ex & bit)) { - res_ex |= bit; - retval++; - wait->_qproc = NULL; - } - /* got something, stop busy polling */ - if (retval) { - can_busy_loop = false; - busy_flag = 0; - - /* - * only remember a returned - * POLL_BUSY_LOOP if we asked for it - */ - } else if (busy_flag & mask) - can_busy_loop = true; - } + if ((mask & POLLIN_SET) && (in & bit)) { + res_in |= bit; + retval++; + wait->_qproc = NULL; + } + if ((mask & POLLOUT_SET) && (out & bit)) { + res_out |= bit; + retval++; + wait->_qproc = NULL; + } + if ((mask & POLLEX_SET) && (ex & bit)) { + res_ex |= bit; + retval++; + wait->_qproc = NULL; + } + /* got something, stop busy polling */ + if (retval) { + can_busy_loop = false; + busy_flag = 0; + + /* + * only remember a returned + * POLL_BUSY_LOOP if we asked for it + */ + } else if (busy_flag & mask) + can_busy_loop = true; + } if (res_in) *rinp = res_in;
On Mon, Jan 10, 2022 at 10:19 AM Jan Kara jack@suse.cz wrote:
A task can end up indefinitely sleeping in do_select() -> poll_schedule_timeout() when the following race happens: {...]
Ok, I decided to just take this as-is right now, and get it in early in the merge window, and see if anybody hollers.
I don't think the stable people will try to apply it until after the merge window closes anyway, but it's worth pointing out that this change (commit 68514dacf271: "select: Fix indefinitely sleeping task in poll_schedule_timeout()" in my tree now) is very much a change of behavior, and we may have to revert it if it causes any issues.
The most likely issue it would cause is that some program uses select() with an fd mask with extra garbage in it, and stale fd bits that pointed to closed file descriptors used to just be ignored. Now they'll cause select() to return immediately with those bits set.
And that might then cause a program to perhaps still work, but busy-spin on select(), wasting CPU time. Or it will walk the result bits, see them set, try to read/write to them, get EBADF, and clear them. Or not clear them and just be very unhappy indeed.
So while I think this version of the patch is still safer than the EBADF one - and I think better semantics that happen to match poll() too - I think this is a patch that could expose existing bad user space.
We'll see. I considered adding a WARN_ON_ONCE() just to make the change in behavior more visible, but ended up not really feeling it.
End result: I took this patch eagerly not because I was happy to do it, but simply because the earlier we test this, the earlier we'll know of any problems. Let's hope there are none.
Linus
On Tue, Jan 11, 2022 at 09:12:08AM -0800, Linus Torvalds wrote:
On Mon, Jan 10, 2022 at 10:19 AM Jan Kara jack@suse.cz wrote:
A task can end up indefinitely sleeping in do_select() -> poll_schedule_timeout() when the following race happens: {...]
Ok, I decided to just take this as-is right now, and get it in early in the merge window, and see if anybody hollers.
I don't think the stable people will try to apply it until after the merge window closes anyway, but it's worth pointing out that this change (commit 68514dacf271: "select: Fix indefinitely sleeping task in poll_schedule_timeout()" in my tree now) is very much a change of behavior, and we may have to revert it if it causes any issues.
The most likely issue it would cause is that some program uses select() with an fd mask with extra garbage in it, and stale fd bits that pointed to closed file descriptors used to just be ignored. Now they'll cause select() to return immediately with those bits set.
And that might then cause a program to perhaps still work, but busy-spin on select(), wasting CPU time. Or it will walk the result bits, see them set, try to read/write to them, get EBADF, and clear them. Or not clear them and just be very unhappy indeed.
So while I think this version of the patch is still safer than the EBADF one - and I think better semantics that happen to match poll() too - I think this is a patch that could expose existing bad user space.
We'll see. I considered adding a WARN_ON_ONCE() just to make the change in behavior more visible, but ended up not really feeling it.
End result: I took this patch eagerly not because I was happy to do it, but simply because the earlier we test this, the earlier we'll know of any problems. Let's hope there are none.
Thanks for the heads-up for the stable tree relevance, I'll watch out for this one.
greg k-h
On Tue 11-01-22 09:12:08, Linus Torvalds wrote:
On Mon, Jan 10, 2022 at 10:19 AM Jan Kara jack@suse.cz wrote:
A task can end up indefinitely sleeping in do_select() -> poll_schedule_timeout() when the following race happens: {...]
Ok, I decided to just take this as-is right now, and get it in early in the merge window, and see if anybody hollers.
I don't think the stable people will try to apply it until after the merge window closes anyway, but it's worth pointing out that this change (commit 68514dacf271: "select: Fix indefinitely sleeping task in poll_schedule_timeout()" in my tree now) is very much a change of behavior, and we may have to revert it if it causes any issues.
The most likely issue it would cause is that some program uses select() with an fd mask with extra garbage in it, and stale fd bits that pointed to closed file descriptors used to just be ignored. Now they'll cause select() to return immediately with those bits set.
That's not quite true. max_select_fd() called from do_select() will bail with -EBADF if any set contains a bit that is not in current->files->open_fds. So what you describe will already end with -EBADF from select(2) regardless of my patch. But yes, if fd becomes invalid while select(2) is already running, then the patch changes behavior. OTOH applications messing with file table from another thread while select is running should be much rarer.
Honza
On Tue, Jan 11, 2022 at 1:27 PM Jan Kara jack@suse.cz wrote:
That's not quite true. max_select_fd() called from do_select() will bail with -EBADF if any set contains a bit that is not in current->files->open_fds.
Yeah, that probably does take care of any normal case, since you need the race with close() to actually cause issues.
Good point.
Linus
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Jan Kara -
Linus Torvalds