Skip to contentSkip to conversation

sys_poll(): fix function definition/negative timeout values

PATCH In review Thomas Meyer 5 participants 7 messages Sep 15, 2011 – Oct 6, 2011

Subsystems (2)
VFS ABI/API
lore
Share & tools
lore

Thomas Meyer

author
#0 lore raw

PATCH 1/1 sys_poll(): fix function definition/negative timeout values

Fix negative timeout values for x86 userland on x86_64 kernels. Align sys_poll() definition to glibc's definition.

Signed-off-by Thomas Meyer <[email protected]>
---
 fs/select.c              |    2 +-
 include/linux/syscalls.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
fs/select.c +1 -1
index d33418f..e782258 100644
@@ -912,7 +912,7 @@ static long do_restart_poll(struct restart_block *restart_block)
 }
 
 SYSCALL_DEFINE3(poll, struct pollfd __user *, ufds, unsigned int, nfds,
-		long, timeout_msecs)
+		int, timeout_msecs)
 {
 	struct timespec end_time, *to = NULL;
 	int ret;
include/linux/syscalls.h +1 -1
index 1ff0ec2..8f482e5 100644
@@ -624,7 +624,7 @@ asmlinkage long sys_socketpair(int, int, int, int __user *);
 asmlinkage long sys_socketcall(int call, unsigned long __user *args);
 asmlinkage long sys_listen(int, int);
 asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
-				long timeout);
+				int timeout);
 asmlinkage long sys_select(int n, fd_set __user *inp, fd_set __user *outp,
 			fd_set __user *exp, struct timeval __user *tvp);
 asmlinkage long sys_old_select(struct sel_arg_struct __user *arg);
Signature · 1 line
1.7.6.2
#1 lore raw
Quoted 2 lines · Thomas Meyer

Fix negative timeout values for x86 userland on x86_64 kernels. Align sys_poll() definition to glibc's definition.

Nack. Please write a compat wrapper that sign extends.

-Andi

Signature · 1 line
[email protected] -- Speaking for myself only
#2 lore raw
Quoted 7 lines · Andi Kleen
Quoted 2 lines · Thomas Meyer

Fix negative timeout values for x86 userland on x86_64 kernels. Align sys_poll() definition to glibc's definition.

Nack. Please write a compat wrapper that sign extends.

Why ?

man poll

int poll(struct pollfd *ufds, unsigned int nfds, int delay);

#3 lore raw
Quoted 10 lines · Eric Dumazet
Quoted 7 lines · Andi Kleen
Quoted 2 lines · Thomas Meyer

Fix negative timeout values for x86 userland on x86_64 kernels. Align sys_poll() definition to glibc's definition.

Nack. Please write a compat wrapper that sign extends.

Why ?

Because we shouldn't change existing interfaces and there could be valid reasons on 64bit for really long delays.

-Andi

Signature · 1 line
[email protected] -- Speaking for myself only.
#4 lore raw
Quoted 7 lines · Andi Kleen
Quoted 6 lines · Eric Dumazet
Quoted 5 lines · Andi Kleen
Quoted 2 lines · Thomas Meyer

Fix negative timeout values for x86 userland on x86_64 kernels. Align sys_poll() definition to glibc's definition.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Nack. Please write a compat wrapper that sign extends.

Why ?

Because we shouldn't change existing interfaces and there could be valid reasons on 64bit for really long delays.

I disagree.

Existing interface and POSIX mandates "int delay"

The kernel part of the contract was fine when we supported 32bit only machines, by chance, because sizeof(int) == sizeof(long).

If you want more than 31 bits delay, then we need a new interface, and this new interface must also work on 32bit arches (sort of poll64())

#5 lore raw
Quoted 18 lines · On Thu, 15 Sep 2011 20:21:02 +0200 Eric Dumazet <[email protected]> wrote:

Le jeudi 15 septembre 2011 __ 20:12 +0200, Andi Kleen a __crit :

Quoted 7 lines · Eric Dumazet
Quoted 6 lines

Le jeudi 15 septembre 2011 __ 10:47 -0700, Andi Kleen a __crit :

Quoted 5 lines
Quoted 2 lines · Thomas Meyer

Fix negative timeout values for x86 userland on x86_64 kernels. Align sys_poll() definition to glibc's definition.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Nack. Please write a compat wrapper that sign extends.

Why ?

Because we shouldn't change existing interfaces and there could be valid reasons on 64bit for really long delays.

I disagree.

Quoted 7 lines

Existing interface and POSIX mandates "int delay"

The kernel part of the contract was fine when we supported 32bit only machines, by chance, because sizeof(int) == sizeof(long).

If you want more than 31 bits delay, then we need a new interface, and this new interface must also work on 32bit arches (sort of poll64())

So what's happening here?

If the change will break (or alter) existing code which does

	poll(..., something_greater_than_2g)

then I don't think we can apply the patch. There may be code out there which breaks, there may not be. We don't know. We screwed up, we get to wear the cost of having done so.

#6 lore raw
In reply to Thomas Meyer

fs/select.c static long do_restart_poll(struct restart_block *restart_block) · line 915

Quoted 5 lines · Thomas Meyer

Fix negative timeout values for x86 userland on x86_64 kernels. Align sys_poll() definition to glibc's definition.

fs/select.c +1 -1
-		long, timeout_msecs)
+		int, timeout_msecs)

Is this going to surprise a non-glibc userspace on any platforms?