| Message ID | 20220110181923.5340-1-jack@suse.cz |
|---|---|
| State | Accepted |
| Commit | 68514dacf2715d11b91ca50d88de047c086fea9c |
| Headers | show |
| Series | [v2] select: Fix indefinitely sleeping task in poll_schedule_timeout() | expand |
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
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;
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(-)