[PATCH v2] select: Fix indefinitely sleeping task in poll_schedule_timeout()

[PATCH v2] select: Fix indefinitely sleeping task in poll_schedule_timeout()

[Jan Kara]

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;
-- 
2.31.1

Re: [PATCH v2] select: Fix indefinitely sleeping task in poll_schedule_timeout()

[Linus Torvalds]

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

Re: [PATCH v2] select: Fix indefinitely sleeping task in poll_schedule_timeout()

[Greg KH]

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

Re: [PATCH v2] select: Fix indefinitely sleeping task in poll_schedule_timeout()

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] select: Fix indefinitely sleeping task in poll_schedule_timeout()

[Linus Torvalds]

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