[PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Manfred Spraul]

Hi Andrew,

poll() returns -EINTR if a signal is pending.
EINTR is a bad choice: it means that poll returns to user space if the
task is stopped by SIGSTOP/SIGCONT or by the freezer.
select() and ppoll() both use ERESTARTNOHAND, this avoids a return to
user space for signals that are handled by the kernel.

The patch switches poll() to ERESTARTNOHAND.
Tested with FC6. Patch against 2.6.23-rc1-mm1.

Signed-off-by: Manfred Spraul <manfred@colorfullife.com>

--- 2.6/fs/select.c	2007-07-28 20:31:51.000000000 +0200
+++ build-2.6/fs/select.c	2007-07-28 21:21:52.000000000 +0200
@@ -621,7 +621,7 @@ static int do_poll(unsigned int nfds,  s
 		if (!count) {
 			count = wait->error;
 			if (signal_pending(current))
-				count = -EINTR;
+				count = -ERESTARTNOHAND;
 		}
 		if (count || !*timeout)
 			break;
@@ -774,7 +774,7 @@ asmlinkage long sys_ppoll(struct pollfd 
 	ret = do_sys_poll(ufds, nfds, &timeout);
 
 	/* We can restart this syscall, usually */
-	if (ret == -EINTR) {
+	if (ret == -ERESTARTNOHAND) {
 		/*
 		 * Don't restore the signal mask yet. Let do_signal() deliver
 		 * the signal on the way back to userspace, before the signal
@@ -785,7 +785,6 @@ asmlinkage long sys_ppoll(struct pollfd 
 					sizeof(sigsaved));
 			set_thread_flag(TIF_RESTORE_SIGMASK);
 		}
-		ret = -ERESTARTNOHAND;
 	} else if (sigmask)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Andrew Morton]

On Sun, 29 Jul 2007 19:05:05 +0200
Manfred Spraul <manfred@colorfullife.com> wrote:

> Hi Andrew,
> 
> poll() returns -EINTR if a signal is pending.
> EINTR is a bad choice: it means that poll returns to user space if the
> task is stopped by SIGSTOP/SIGCONT or by the freezer.
> select() and ppoll() both use ERESTARTNOHAND, this avoids a return to
> user space for signals that are handled by the kernel.
> 
> The patch switches poll() to ERESTARTNOHAND.
> Tested with FC6. Patch against 2.6.23-rc1-mm1.

hm.  Is this a fix against
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc1/2.6.23-rc1-mm1/broken-out/do_poll-return-eintr-when-signalled.patch
only, or does mainline also need fixing?

I guess the consequences of the thing-which-this-fixes aren't huge, s I ca
queue this up for 2.6.24, after Oleg's
do_poll-return-eintr-when-signalled.patch?

> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> 
> --- 2.6/fs/select.c	2007-07-28 20:31:51.000000000 +0200
> +++ build-2.6/fs/select.c	2007-07-28 21:21:52.000000000 +0200
> @@ -621,7 +621,7 @@ static int do_poll(unsigned int nfds,  s
>  		if (!count) {
>  			count = wait->error;
>  			if (signal_pending(current))
> -				count = -EINTR;
> +				count = -ERESTARTNOHAND;
>  		}
>  		if (count || !*timeout)
>  			break;
> @@ -774,7 +774,7 @@ asmlinkage long sys_ppoll(struct pollfd 
>  	ret = do_sys_poll(ufds, nfds, &timeout);
>  
>  	/* We can restart this syscall, usually */
> -	if (ret == -EINTR) {
> +	if (ret == -ERESTARTNOHAND) {
>  		/*
>  		 * Don't restore the signal mask yet. Let do_signal() deliver
>  		 * the signal on the way back to userspace, before the signal
> @@ -785,7 +785,6 @@ asmlinkage long sys_ppoll(struct pollfd 
>  					sizeof(sigsaved));
>  			set_thread_flag(TIF_RESTORE_SIGMASK);
>  		}
> -		ret = -ERESTARTNOHAND;
>  	} else if (sigmask)
>  		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
>  

I spied this comment in there:

/*
 * We can actually return ERESTARTSYS instead of EINTR, but I'd
 * like to be certain this leads to no problems. So I return
 * EINTR just for safety.
 *
 * Update: ERESTARTSYS breaks at least the xview clock binary, so
 * I'm trying ERESTARTNOHAND which restart only when you want to.
 */

it is very old and perhaps is no longer relevant?

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Chris Wright]

* Manfred Spraul (manfred@colorfullife.com) wrote:
> poll() returns -EINTR if a signal is pending.
> EINTR is a bad choice: it means that poll returns to user space if the
> task is stopped by SIGSTOP/SIGCONT or by the freezer.
> select() and ppoll() both use ERESTARTNOHAND, this avoids a return to
> user space for signals that are handled by the kernel.

This does change user visible behaviour, however, it's already
inconsistent as you've noted.  I was concerned about that change, but
wrote some tests and this behaviour does make sense to me.

thanks,
-chris

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Chris Wright]

* Andrew Morton (akpm@linux-foundation.org) wrote:
> On Sun, 29 Jul 2007 19:05:05 +0200 Manfred Spraul <manfred@colorfullife.com> wrote:
> > poll() returns -EINTR if a signal is pending.
> > EINTR is a bad choice: it means that poll returns to user space if the
> > task is stopped by SIGSTOP/SIGCONT or by the freezer.
> > select() and ppoll() both use ERESTARTNOHAND, this avoids a return to
> > user space for signals that are handled by the kernel.
> > 
> > The patch switches poll() to ERESTARTNOHAND.
> > Tested with FC6. Patch against 2.6.23-rc1-mm1.
> 
> hm.  Is this a fix against
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc1/2.6.23-rc1-mm1/broken-out/do_poll-return-eintr-when-signalled.patch
> only, or does mainline also need fixing?

Mainline has same issue (although fix is certainly a bit different w/out
Oleg's patch).

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Oleg Nesterov]

On 07/30, Andrew Morton wrote:
>
> On Sun, 29 Jul 2007 19:05:05 +0200
> Manfred Spraul <manfred@colorfullife.com> wrote:
> 
> > Hi Andrew,
> > 
> > poll() returns -EINTR if a signal is pending.
> > EINTR is a bad choice: it means that poll returns to user space if the
> > task is stopped by SIGSTOP/SIGCONT or by the freezer.
> > select() and ppoll() both use ERESTARTNOHAND, this avoids a return to
> > user space for signals that are handled by the kernel.
> > 
> > The patch switches poll() to ERESTARTNOHAND.
> > Tested with FC6. Patch against 2.6.23-rc1-mm1.
> 
> hm.  Is this a fix against
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc1/2.6.23-rc1-mm1/broken-out/do_poll-return-eintr-when-signalled.patch
> only, or does mainline also need fixing?

That patch doesn't (at least shouldn't) change the behaviour.

> I guess the consequences of the thing-which-this-fixes aren't huge, s I ca
> queue this up for 2.6.24, after Oleg's
> do_poll-return-eintr-when-signalled.patch?
> 
> > Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> > 
> > --- 2.6/fs/select.c	2007-07-28 20:31:51.000000000 +0200
> > +++ build-2.6/fs/select.c	2007-07-28 21:21:52.000000000 +0200
> > @@ -621,7 +621,7 @@ static int do_poll(unsigned int nfds,  s
> >  		if (!count) {
> >  			count = wait->error;
> >  			if (signal_pending(current))
> > -				count = -EINTR;
> > +				count = -ERESTARTNOHAND;

I am not sure. This means we restart sys_poll() with the same timeout
if there is no pending signal. I think we need ERESTART_RESTARTBLOCK
logic.

Oleg.

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Oleg Nesterov]

On 07/31, Oleg Nesterov wrote:
>
> > Manfred Spraul <manfred@colorfullife.com> wrote:
> > 
> > > poll() returns -EINTR if a signal is pending.
> > > EINTR is a bad choice: it means that poll returns to user space if the
> > > task is stopped by SIGSTOP/SIGCONT or by the freezer.
> > > select() and ppoll() both use ERESTARTNOHAND, this avoids a return to
> > > user space for signals that are handled by the kernel.
> > > 
> > > The patch switches poll() to ERESTARTNOHAND.
> > > Tested with FC6. Patch against 2.6.23-rc1-mm1.
> > 
> > > --- 2.6/fs/select.c	2007-07-28 20:31:51.000000000 +0200
> > > +++ build-2.6/fs/select.c	2007-07-28 21:21:52.000000000 +0200
> > > @@ -621,7 +621,7 @@ static int do_poll(unsigned int nfds,  s
> > >  		if (!count) {
> > >  			count = wait->error;
> > >  			if (signal_pending(current))
> > > -				count = -EINTR;
> > > +				count = -ERESTARTNOHAND;
> 
> I am not sure. This means we restart sys_poll() with the same timeout
> if there is no pending signal. I think we need ERESTART_RESTARTBLOCK
> logic.

Forgot to mention, sys_select() can use ERESTARTNOHAND because it
modifies "struct timeval __user *tvp" before return, but sys_poll()
gets timeout_msecs by value.

Oleg.

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Chris Wright]

* Oleg Nesterov (oleg@tv-sign.ru) wrote:
> > I am not sure. This means we restart sys_poll() with the same timeout
> > if there is no pending signal. I think we need ERESTART_RESTARTBLOCK
> > logic.
> 
> Forgot to mention, sys_select() can use ERESTARTNOHAND because it
> modifies "struct timeval __user *tvp" before return, but sys_poll()
> gets timeout_msecs by value.

Yeah, you're right.  Means sys_poll with STOP/CONT cycles going in
background (and no fds ready) would never return.

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Manfred Spraul]

Andrew Morton wrote:
> On Sun, 29 Jul 2007 19:05:05 +0200
> Manfred Spraul <manfred@colorfullife.com> wrote:
>
>   
>> Hi Andrew,
>>
>> poll() returns -EINTR if a signal is pending.
>> EINTR is a bad choice: it means that poll returns to user space if the
>> task is stopped by SIGSTOP/SIGCONT or by the freezer.
>> select() and ppoll() both use ERESTARTNOHAND, this avoids a return to
>> user space for signals that are handled by the kernel.
>>
>> The patch switches poll() to ERESTARTNOHAND.
>> Tested with FC6. Patch against 2.6.23-rc1-mm1.
>>     
>
> hm.  Is this a fix against
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.23-rc1/2.6.23-rc1-mm1/broken-out/do_poll-return-eintr-when-signalled.patch
> only, or does mainline also need fixing?
>
>   
Mainline has the same problem: poll() returns to user space if it's 
interrupted by SIGSTOP/SIGCONT.
> I guess the consequences of the thing-which-this-fixes aren't huge, s I ca
> queue this up for 2.6.24, after Oleg's
> do_poll-return-eintr-when-signalled.patch?
>
>   
Yes, please queue it: most/all linux versions show this behavior. 
Additionally, poll() is usually called in a loop and a spurious wakeup 
has no consequences.

--
    Manfred

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Oleg Nesterov]

On 07/31, Manfred Spraul wrote:
>
> Mainline has the same problem: poll() returns to user space if it's 
> interrupted by SIGSTOP/SIGCONT.
> >I guess the consequences of the thing-which-this-fixes aren't huge, s I ca
> >queue this up for 2.6.24, after Oleg's
> >do_poll-return-eintr-when-signalled.patch?
> >
> >  
> Yes, please queue it: most/all linux versions show this behavior. 
> Additionally, poll() is usually called in a loop and a spurious wakeup 
> has no consequences.

... and so the current behaviour is more or less correct, even if not
optimal.

But this patch in fact adds a bug. We don't even need the "special"
signals like SIGSTOP/SIGCONT or freezer to hit this bug.

Suppose that sys_poll() was interrupted by a "normal" signal which
has a handler. It is quite possible that another thread can steal
this signal before us. Now, ERESTARTNOHAND means we restart sys_poll()
with the same (old) timeout, this means that sys_poll() does _not_
return when timeout expired (if no fds ready), and this is bug.

Also, the false signal_wake_up() is possible, and again, the spurious
-EINTR is better than restart with the same timeout.

What we need is ERESTART_RESTARTBLOCK, and restart_block.arg2 should
have the new timeout value, which takes the time we already slept
into account.

Oleg.

[PATCH] Use ERESTART_RESTARTBLOCK if poll() is interrupted by a signal (was: Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal)

[Chris Wright]

* Oleg Nesterov (oleg@tv-sign.ru) wrote:
> What we need is ERESTART_RESTARTBLOCK, and restart_block.arg2 should
> have the new timeout value, which takes the time we already slept
> into account.

This passes my simple 32-bit and 64-bit testing.  See any issues with
this one?

thanks,
-chris
--

Subject: [PATCH] Use ERESTART_RESTARTBLOCK if poll() is interrupted by a signal
From: Chris Wright <chrisw@sous-sol.org>

Lomesh reported poll returning EINTR during suspend/resume cycle.
This is caused by the STOP/CONT cycle that the freezer uses, generating
a pending signal for what in effect is an ignored signal.  In general
poll is a little eager in returning EINTR, when it could try not bother
userspace and simply restart the syscall.  Both select and ppoll do use
ERESTARTNOHAND to restart the syscall.  Oleg points out that simply using
ERESTARTNOHAND will cause poll to restart with original timeout value.
which could ultimately lead to process never returning to userspace.
Instead use ERESTART_RESTARTBLOCK, and restart poll with updated timeout
value.  Inspired by Manfred's use ERESTARTNOHAND in poll patch.

Cc: Manfred Spraul <manfred@colorfullife.com>
Cc: Oleg Nesterov <oleg@tv-sign.ru>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Roland McGrath <roland@redhat.com>
Cc: "Agarwal, Lomesh" <lomesh.agarwal@intel.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
Patch against git-7a883eaf

 fs/select.c |   34 +++++++++++++++++++++++++++++++++-
 1 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index a974082..50e6d8e 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -736,10 +736,29 @@ out_fds:
 	return err;
 }
 
+long do_restart_poll(struct restart_block *restart_block)
+{
+	struct pollfd __user *ufds = (struct pollfd __user*)restart_block->arg0;
+	int nfds = restart_block->arg1;
+	s64 timeout = ((s64)restart_block->arg3<<32) | (s64)restart_block->arg2;
+	int ret;
+
+	restart_block->fn = do_no_restart_syscall;
+	ret = do_sys_poll(ufds, nfds, &timeout);
+	if (ret == -EINTR) {
+		restart_block->fn = do_restart_poll;
+		restart_block->arg2 = timeout & 0xFFFFFFFF;
+		restart_block->arg3 = timeout >> 32;
+		ret = -ERESTART_RESTARTBLOCK;
+	}
+	return ret;
+}
+
 asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 			long timeout_msecs)
 {
 	s64 timeout_jiffies;
+	int ret;
 
 	if (timeout_msecs > 0) {
 #if HZ > 1000
@@ -754,7 +773,20 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 		timeout_jiffies = timeout_msecs;
 	}
 
-	return do_sys_poll(ufds, nfds, &timeout_jiffies);
+	ret = do_sys_poll(ufds, nfds, &timeout_jiffies);
+	if (ret == -EINTR) {
+		if (timeout_msecs > 0) {
+			struct restart_block *restart_block;
+			restart_block = &current_thread_info()->restart_block;
+			restart_block->fn = do_restart_poll;
+			restart_block->arg0 = (unsigned long)ufds;
+			restart_block->arg1 = nfds;
+			restart_block->arg2 = timeout_jiffies & 0xFFFFFFFF;
+			restart_block->arg3 = timeout_jiffies >> 32;
+			ret = -ERESTART_RESTARTBLOCK;
+		}
+	}
+	return ret;
 }
 
 #ifdef TIF_RESTORE_SIGMASK

Re: [PATCH] Use ERESTART_RESTARTBLOCK if poll() is interrupted by a signal (was: Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal)

[Oleg Nesterov]

On 08/03, Chris Wright wrote:
>
> +long do_restart_poll(struct restart_block *restart_block)
> +{
> +	struct pollfd __user *ufds = (struct pollfd __user*)restart_block->arg0;
> +	int nfds = restart_block->arg1;
> +	s64 timeout = ((s64)restart_block->arg3<<32) | (s64)restart_block->arg2;
> +	int ret;
> +
> +	restart_block->fn = do_no_restart_syscall;

(just in case, this is not strictly necessary)

> +	ret = do_sys_poll(ufds, nfds, &timeout);
> +	if (ret == -EINTR) {
> +		restart_block->fn = do_restart_poll;
> +		restart_block->arg2 = timeout & 0xFFFFFFFF;
> +		restart_block->arg3 = timeout >> 32;
> +		ret = -ERESTART_RESTARTBLOCK;
> +	}
> +	return ret;
> +}
> +
>  asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
>  			long timeout_msecs)
>  {
>  	s64 timeout_jiffies;
> +	int ret;
>  
>  	if (timeout_msecs > 0) {
>  #if HZ > 1000
> @@ -754,7 +773,20 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
>  		timeout_jiffies = timeout_msecs;
>  	}
>  
> -	return do_sys_poll(ufds, nfds, &timeout_jiffies);
> +	ret = do_sys_poll(ufds, nfds, &timeout_jiffies);
> +	if (ret == -EINTR) {
> +		if (timeout_msecs > 0) {

This should be "if (timeout_msecs)", a negative timeout means
"wait indefinitely", so we should restart as well.

> +			struct restart_block *restart_block;
> +			restart_block = &current_thread_info()->restart_block;
> +			restart_block->fn = do_restart_poll;
> +			restart_block->arg0 = (unsigned long)ufds;
> +			restart_block->arg1 = nfds;
> +			restart_block->arg2 = timeout_jiffies & 0xFFFFFFFF;
> +			restart_block->arg3 = timeout_jiffies >> 32;

and, in that case, we should use "(u64)timeout_jiffies >> 32".


Small nit: sys_poll() and do_restart_poll() are not "symmetrical", the latter
doesn't check *timeout at all. Either way is correct, restart with zero timeout
means "try again once but doesn't wait".

There is a subtle (but harmless) difference though. Suppose that *timeout == 0
(either because timeout_msecs == 0 or because timeout expiried) and we have a
"false" signal_pending(). In that case sys_poll() returns EINTR, but do_restart_poll()
returns 0.

I was a bit surprized that sys_poll() return EINTR even if timeout expired, but
preserved this behaviour in do_poll-return-eintr-when-signalled.patch.

Perhaps it is better to just remove "if (timeout_msecs > 0)" check from sys_poll().
Then we can modify do_poll() to return EINTR only when !*timeout, to eliminate
unneeded (but correct) restart.

What do you think?

Oleg.

[PATCH take2] Use ERESTART_RESTARTBLOCK if poll() is interrupted by a signal

[Chris Wright]

* Oleg Nesterov (oleg@tv-sign.ru) wrote:
> On 08/03, Chris Wright wrote:
> >
> > +long do_restart_poll(struct restart_block *restart_block)
> > +{
> > +	struct pollfd __user *ufds = (struct pollfd __user*)restart_block->arg0;
> > +	int nfds = restart_block->arg1;
> > +	s64 timeout = ((s64)restart_block->arg3<<32) | (s64)restart_block->arg2;
> > +	int ret;
> > +
> > +	restart_block->fn = do_no_restart_syscall;
> 
> (just in case, this is not strictly necessary)

Removed, thanks.

> > +	ret = do_sys_poll(ufds, nfds, &timeout);
> > +	if (ret == -EINTR) {
> > +		restart_block->fn = do_restart_poll;
> > +		restart_block->arg2 = timeout & 0xFFFFFFFF;
> > +		restart_block->arg3 = timeout >> 32;
> > +		ret = -ERESTART_RESTARTBLOCK;
> > +	}
> > +	return ret;
> > +}
> > +
> >  asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
> >  			long timeout_msecs)
> >  {
> >  	s64 timeout_jiffies;
> > +	int ret;
> >  
> >  	if (timeout_msecs > 0) {
> >  #if HZ > 1000
> > @@ -754,7 +773,20 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
> >  		timeout_jiffies = timeout_msecs;
> >  	}
> >  
> > -	return do_sys_poll(ufds, nfds, &timeout_jiffies);
> > +	ret = do_sys_poll(ufds, nfds, &timeout_jiffies);
> > +	if (ret == -EINTR) {
> > +		if (timeout_msecs > 0) {
> 
> This should be "if (timeout_msecs)", a negative timeout means
> "wait indefinitely", so we should restart as well.

Yes, you're right (not really sure what I was thinking there ;-)

> > +			struct restart_block *restart_block;
> > +			restart_block = &current_thread_info()->restart_block;
> > +			restart_block->fn = do_restart_poll;
> > +			restart_block->arg0 = (unsigned long)ufds;
> > +			restart_block->arg1 = nfds;
> > +			restart_block->arg2 = timeout_jiffies & 0xFFFFFFFF;
> > +			restart_block->arg3 = timeout_jiffies >> 32;
> 
> and, in that case, we should use "(u64)timeout_jiffies >> 32".

Sure, sign extension should get shifted back off, but that is more
accurate.

> Small nit: sys_poll() and do_restart_poll() are not "symmetrical", the latter
> doesn't check *timeout at all. Either way is correct, restart with zero timeout
> means "try again once but doesn't wait".
> 
> There is a subtle (but harmless) difference though. Suppose that *timeout == 0
> (either because timeout_msecs == 0 or because timeout expiried) and we have a
> "false" signal_pending(). In that case sys_poll() returns EINTR, but do_restart_poll()
> returns 0.
> 
> I was a bit surprized that sys_poll() return EINTR even if timeout expired, but
> preserved this behaviour in do_poll-return-eintr-when-signalled.patch.
> 
> Perhaps it is better to just remove "if (timeout_msecs > 0)" check from sys_poll().
> Then we can modify do_poll() to return EINTR only when !*timeout, to eliminate
> unneeded (but correct) restart.
> 
> What do you think?

I agree.  Here's the respin.

thanks,
-chris
--

Subject: [PATCH take2] Use ERESTART_RESTARTBLOCK if poll() is interrupted by a signal
From: Chris Wright <chrisw@sous-sol.org>

Lomesh reported poll returning EINTR during suspend/resume cycle.
This is caused by the STOP/CONT cycle that the freezer uses, generating
a pending signal for what in effect is an ignored signal.  In general
poll is a little eager in returning EINTR, when it could try not bother
userspace and simply restart the syscall.  Both select and ppoll do use
ERESTARTNOHAND to restart the syscall.  Oleg points out that simply using
ERESTARTNOHAND will cause poll to restart with original timeout value.
which could ultimately lead to process never returning to userspace.
Instead use ERESTART_RESTARTBLOCK, and restart poll with updated timeout
value.  Inspired by Manfred's use ERESTARTNOHAND in poll patch.

Cc: Manfred Spraul <manfred@colorfullife.com>
Cc: Oleg Nesterov <oleg@tv-sign.ru>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Roland McGrath <roland@redhat.com>
Cc: "Agarwal, Lomesh" <lomesh.agarwal@intel.com>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
Patch against git-28e8351a

 fs/select.c |   31 ++++++++++++++++++++++++++++++-
 1 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index a974082..5562195 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -736,10 +736,28 @@ out_fds:
 	return err;
 }
 
+long do_restart_poll(struct restart_block *restart_block)
+{
+	struct pollfd __user *ufds = (struct pollfd __user*)restart_block->arg0;
+	int nfds = restart_block->arg1;
+	s64 timeout = ((s64)restart_block->arg3<<32) | (s64)restart_block->arg2;
+	int ret;
+
+	ret = do_sys_poll(ufds, nfds, &timeout);
+	if (ret == -EINTR) {
+		restart_block->fn = do_restart_poll;
+		restart_block->arg2 = timeout & 0xFFFFFFFF;
+		restart_block->arg3 = (u64)timeout >> 32;
+		ret = -ERESTART_RESTARTBLOCK;
+	}
+	return ret;
+}
+
 asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 			long timeout_msecs)
 {
 	s64 timeout_jiffies;
+	int ret;
 
 	if (timeout_msecs > 0) {
 #if HZ > 1000
@@ -754,7 +772,18 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 		timeout_jiffies = timeout_msecs;
 	}
 
-	return do_sys_poll(ufds, nfds, &timeout_jiffies);
+	ret = do_sys_poll(ufds, nfds, &timeout_jiffies);
+	if (ret == -EINTR) {
+		struct restart_block *restart_block;
+		restart_block = &current_thread_info()->restart_block;
+		restart_block->fn = do_restart_poll;
+		restart_block->arg0 = (unsigned long)ufds;
+		restart_block->arg1 = nfds;
+		restart_block->arg2 = timeout_jiffies & 0xFFFFFFFF;
+		restart_block->arg3 = (u64)timeout_jiffies >> 32;
+		ret = -ERESTART_RESTARTBLOCK;
+	}
+	return ret;
 }
 
 #ifdef TIF_RESTORE_SIGMASK

Re: [PATCH take2] Use ERESTART_RESTARTBLOCK if poll() is interrupted by a signal

[Oleg Nesterov]

On 08/15, Chris Wright wrote:
>
>  fs/select.c |   31 ++++++++++++++++++++++++++++++-
>  1 files changed, 30 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/select.c b/fs/select.c
> index a974082..5562195 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -736,10 +736,28 @@ out_fds:
>  	return err;
>  }
>  
> +long do_restart_poll(struct restart_block *restart_block)
> +{
> +	struct pollfd __user *ufds = (struct pollfd __user*)restart_block->arg0;
> +	int nfds = restart_block->arg1;
> +	s64 timeout = ((s64)restart_block->arg3<<32) | (s64)restart_block->arg2;
> +	int ret;
> +
> +	ret = do_sys_poll(ufds, nfds, &timeout);
> +	if (ret == -EINTR) {
> +		restart_block->fn = do_restart_poll;
> +		restart_block->arg2 = timeout & 0xFFFFFFFF;
> +		restart_block->arg3 = (u64)timeout >> 32;
> +		ret = -ERESTART_RESTARTBLOCK;
> +	}
> +	return ret;
> +}
> +
>  asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
>  			long timeout_msecs)
>  {
>  	s64 timeout_jiffies;
> +	int ret;
>  
>  	if (timeout_msecs > 0) {
>  #if HZ > 1000
> @@ -754,7 +772,18 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
>  		timeout_jiffies = timeout_msecs;
>  	}
>  
> -	return do_sys_poll(ufds, nfds, &timeout_jiffies);
> +	ret = do_sys_poll(ufds, nfds, &timeout_jiffies);
> +	if (ret == -EINTR) {
> +		struct restart_block *restart_block;
> +		restart_block = &current_thread_info()->restart_block;
> +		restart_block->fn = do_restart_poll;
> +		restart_block->arg0 = (unsigned long)ufds;
> +		restart_block->arg1 = nfds;
> +		restart_block->arg2 = timeout_jiffies & 0xFFFFFFFF;
> +		restart_block->arg3 = (u64)timeout_jiffies >> 32;
> +		ret = -ERESTART_RESTARTBLOCK;
> +	}
> +	return ret;
>  }
>  
>  #ifdef TIF_RESTORE_SIGMASK

Great, I think the patch is correct.

Oleg.

Re: [PATCH] Use ERESTARTNOHAND if poll() is interrupted by a signal

[Roland McGrath]

In POSIX, select/pselect is specifically exempted from the usual SA_RESTART
behavior.  I'm not really sure if that is entirely historical or because
after a handler has run even an adjusted relative timeout may no longer be
what you actually want.  There is no such exemption for poll/ppoll, though
in general their behavior and specification are very parallel.  I wonder if
they really should diverge.


Thanks,
Roland