public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [Patch 1/2] ia32: use generic sys_pipe()
@ 2009-07-22  9:08 Amerigo Wang
  2009-07-22  9:08 ` [Patch 2/2] xtensa: " Amerigo Wang
  2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar
  0 siblings, 2 replies; 11+ messages in thread
From: Amerigo Wang @ 2009-07-22  9:08 UTC (permalink / raw)
  To: linux-kernel; +Cc: akpm, mingo, Al Viro, Amerigo Wang, Linus Torvalds


As suggested by Al, it's better to use the generic sys_pipe() for
ia32.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Al Viro <viro@zeniv.linux.org.uk>
CC: Linus Torvalds <torvalds@linux-foundation.org>

---
diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S
index e590261..ba331bf 100644
--- a/arch/x86/ia32/ia32entry.S
+++ b/arch/x86/ia32/ia32entry.S
@@ -537,7 +537,7 @@ ia32_sys_call_table:
 	.quad sys_mkdir
 	.quad sys_rmdir		/* 40 */
 	.quad sys_dup
-	.quad sys32_pipe
+	.quad sys_pipe
 	.quad compat_sys_times
 	.quad quiet_ni_syscall			/* old prof syscall holder */
 	.quad sys_brk		/* 45 */
diff --git a/arch/x86/ia32/sys_ia32.c b/arch/x86/ia32/sys_ia32.c
index 085a8c3..9f55271 100644
--- a/arch/x86/ia32/sys_ia32.c
+++ b/arch/x86/ia32/sys_ia32.c
@@ -189,20 +189,6 @@ asmlinkage long sys32_mprotect(unsigned long start, size_t len,
 	return sys_mprotect(start, len, prot);
 }
 
-asmlinkage long sys32_pipe(int __user *fd)
-{
-	int retval;
-	int fds[2];
-
-	retval = do_pipe_flags(fds, 0);
-	if (retval)
-		goto out;
-	if (copy_to_user(fd, fds, sizeof(fds)))
-		retval = -EFAULT;
-out:
-	return retval;
-}
-
 asmlinkage long sys32_rt_sigaction(int sig, struct sigaction32 __user *act,
 				   struct sigaction32 __user *oact,
 				   unsigned int sigsetsize)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Patch 2/2] xtensa: use generic sys_pipe()
  2009-07-22  9:08 [Patch 1/2] ia32: use generic sys_pipe() Amerigo Wang
@ 2009-07-22  9:08 ` Amerigo Wang
  2009-07-22 10:10   ` Johannes Weiner
  2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar
  1 sibling, 1 reply; 11+ messages in thread
From: Amerigo Wang @ 2009-07-22  9:08 UTC (permalink / raw)
  To: linux-kernel; +Cc: akpm, Linus Torvalds, Al Viro, mingo, Amerigo Wang


As suggested by Al, we can use the generic sys_pipe() instead of xtensa_pipe()
for xtensa.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk> 
CC: Linus Torvalds <torvalds@linux-foundation.org>

---
diff --git a/arch/xtensa/include/asm/syscall.h b/arch/xtensa/include/asm/syscall.h
index 05cebf8..76a1fb8 100644
--- a/arch/xtensa/include/asm/syscall.h
+++ b/arch/xtensa/include/asm/syscall.h
@@ -12,7 +12,6 @@ struct pt_regs;
 struct sigaction;
 asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*);
 asmlinkage long xtensa_clone(unsigned long, unsigned long, struct pt_regs*);
-asmlinkage long xtensa_pipe(int __user *);
 asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long,
     			     unsigned long, unsigned long, unsigned long);
 asmlinkage long xtensa_ptrace(long, long, long, long);
diff --git a/arch/xtensa/include/asm/unistd.h b/arch/xtensa/include/asm/unistd.h
index c092c8f..b6880c8 100644
--- a/arch/xtensa/include/asm/unistd.h
+++ b/arch/xtensa/include/asm/unistd.h
@@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3)
 #define __NR_mknod 				 36
 __SYSCALL( 36, sys_mknod, 3)
 #define __NR_pipe 				 37
-__SYSCALL( 37, xtensa_pipe, 1)
+__SYSCALL(37, sys_pipe, 1)
 #define __NR_unlink 				 38
 __SYSCALL( 38, sys_unlink, 1)
 #define __NR_rmdir 				 39
diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c
index ac15ecb..c7279be 100644
--- a/arch/xtensa/kernel/syscall.c
+++ b/arch/xtensa/kernel/syscall.c
@@ -39,24 +39,6 @@ syscall_t sys_call_table[__NR_syscall_count] /* FIXME __cacheline_aligned */= {
 #include <asm/unistd.h>
 };
 
-/*
- * xtensa_pipe() is the normal C calling standard for creating a pipe. It's not
- * the way unix traditional does this, though.
- */
-
-asmlinkage long xtensa_pipe(int __user *userfds)
-{
-	int fd[2];
-	int error;
-
-	error = do_pipe_flags(fd, 0);
-	if (!error) {
-		if (copy_to_user(userfds, fd, 2 * sizeof(int)))
-			error = -EFAULT;
-	}
-	return error;
-}
-
 
 asmlinkage long xtensa_mmap2(unsigned long addr, unsigned long len,
    			     unsigned long prot, unsigned long flags,

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Patch 2/2] xtensa: use generic sys_pipe()
  2009-07-22  9:08 ` [Patch 2/2] xtensa: " Amerigo Wang
@ 2009-07-22 10:10   ` Johannes Weiner
  2009-07-23  9:47     ` Amerigo Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Johannes Weiner @ 2009-07-22 10:10 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: linux-kernel, akpm, Linus Torvalds, Al Viro, mingo, Chris Zankel

[CC Chris]

On Wed, Jul 22, 2009 at 05:08:42AM -0400, Amerigo Wang wrote:
> 
> As suggested by Al, we can use the generic sys_pipe() instead of xtensa_pipe()
> for xtensa.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk> 
> CC: Linus Torvalds <torvalds@linux-foundation.org>

Reviewed-by: Johannes Weiner <jw@emlix.com>

> ---
> diff --git a/arch/xtensa/include/asm/syscall.h b/arch/xtensa/include/asm/syscall.h
> index 05cebf8..76a1fb8 100644
> --- a/arch/xtensa/include/asm/syscall.h
> +++ b/arch/xtensa/include/asm/syscall.h
> @@ -12,7 +12,6 @@ struct pt_regs;
>  struct sigaction;
>  asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*);
>  asmlinkage long xtensa_clone(unsigned long, unsigned long, struct pt_regs*);
> -asmlinkage long xtensa_pipe(int __user *);
>  asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long,
>      			     unsigned long, unsigned long, unsigned long);
>  asmlinkage long xtensa_ptrace(long, long, long, long);
> diff --git a/arch/xtensa/include/asm/unistd.h b/arch/xtensa/include/asm/unistd.h
> index c092c8f..b6880c8 100644
> --- a/arch/xtensa/include/asm/unistd.h
> +++ b/arch/xtensa/include/asm/unistd.h
> @@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3)
>  #define __NR_mknod 				 36
>  __SYSCALL( 36, sys_mknod, 3)
>  #define __NR_pipe 				 37
> -__SYSCALL( 37, xtensa_pipe, 1)
> +__SYSCALL(37, sys_pipe, 1)

It would have been nice to keep the spacing but that shouldn't be a
show stopper..

>  #define __NR_unlink 				 38
>  __SYSCALL( 38, sys_unlink, 1)
>  #define __NR_rmdir 				 39
> diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c
> index ac15ecb..c7279be 100644
> --- a/arch/xtensa/kernel/syscall.c
> +++ b/arch/xtensa/kernel/syscall.c
> @@ -39,24 +39,6 @@ syscall_t sys_call_table[__NR_syscall_count] /* FIXME __cacheline_aligned */= {
>  #include <asm/unistd.h>
>  };
>  
> -/*
> - * xtensa_pipe() is the normal C calling standard for creating a pipe. It's not
> - * the way unix traditional does this, though.
> - */
> -
> -asmlinkage long xtensa_pipe(int __user *userfds)
> -{
> -	int fd[2];
> -	int error;
> -
> -	error = do_pipe_flags(fd, 0);
> -	if (!error) {
> -		if (copy_to_user(userfds, fd, 2 * sizeof(int)))
> -			error = -EFAULT;
> -	}
> -	return error;
> -}
> -
>  
>  asmlinkage long xtensa_mmap2(unsigned long addr, unsigned long len,
>     			     unsigned long prot, unsigned long flags,

	Hannes

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch 2/2] xtensa: use generic sys_pipe()
  2009-07-22 10:10   ` Johannes Weiner
@ 2009-07-23  9:47     ` Amerigo Wang
  2009-07-23 11:18       ` Johannes Weiner
  0 siblings, 1 reply; 11+ messages in thread
From: Amerigo Wang @ 2009-07-23  9:47 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: linux-kernel, akpm, Linus Torvalds, Al Viro, mingo, Chris Zankel

Johannes Weiner wrote:
> [CC Chris]
>
> On Wed, Jul 22, 2009 at 05:08:42AM -0400, Amerigo Wang wrote:
>   
>> As suggested by Al, we can use the generic sys_pipe() instead of xtensa_pipe()
>> for xtensa.
>>
>> Signed-off-by: WANG Cong <amwang@redhat.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk> 
>> CC: Linus Torvalds <torvalds@linux-foundation.org>
>>     
>
> Reviewed-by: Johannes Weiner <jw@emlix.com>
>
>   

Thanks for your review.
>> ---
>> diff --git a/arch/xtensa/include/asm/syscall.h b/arch/xtensa/include/asm/syscall.h
>> index 05cebf8..76a1fb8 100644
>> --- a/arch/xtensa/include/asm/syscall.h
>> +++ b/arch/xtensa/include/asm/syscall.h
>> @@ -12,7 +12,6 @@ struct pt_regs;
>>  struct sigaction;
>>  asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*);
>>  asmlinkage long xtensa_clone(unsigned long, unsigned long, struct pt_regs*);
>> -asmlinkage long xtensa_pipe(int __user *);
>>  asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long,
>>      			     unsigned long, unsigned long, unsigned long);
>>  asmlinkage long xtensa_ptrace(long, long, long, long);
>> diff --git a/arch/xtensa/include/asm/unistd.h b/arch/xtensa/include/asm/unistd.h
>> index c092c8f..b6880c8 100644
>> --- a/arch/xtensa/include/asm/unistd.h
>> +++ b/arch/xtensa/include/asm/unistd.h
>> @@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3)
>>  #define __NR_mknod 				 36
>>  __SYSCALL( 36, sys_mknod, 3)
>>  #define __NR_pipe 				 37
>> -__SYSCALL( 37, xtensa_pipe, 1)
>> +__SYSCALL(37, sys_pipe, 1)
>>     
>
> It would have been nice to keep the spacing but that shouldn't be a
> show stopper..
>
>   

I did this, but checkpatch.pl complained about this... so I removed the
space. Do I need to update and resend the patch? :)

Thanks.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch 2/2] xtensa: use generic sys_pipe()
  2009-07-23  9:47     ` Amerigo Wang
@ 2009-07-23 11:18       ` Johannes Weiner
  2009-07-24  9:04         ` Amerigo Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Johannes Weiner @ 2009-07-23 11:18 UTC (permalink / raw)
  To: Amerigo Wang
  Cc: linux-kernel, akpm, Linus Torvalds, Al Viro, mingo, Chris Zankel

On Thu, Jul 23, 2009 at 05:47:24PM +0800, Amerigo Wang wrote:
> Johannes Weiner wrote:
> >[CC Chris]
> >
> >On Wed, Jul 22, 2009 at 05:08:42AM -0400, Amerigo Wang wrote:
> >  
> >>As suggested by Al, we can use the generic sys_pipe() instead of 
> >>xtensa_pipe()
> >>for xtensa.
> >>
> >>Signed-off-by: WANG Cong <amwang@redhat.com>
> >>Cc: Al Viro <viro@zeniv.linux.org.uk> 
> >>CC: Linus Torvalds <torvalds@linux-foundation.org>
> >>    
> >
> >Reviewed-by: Johannes Weiner <jw@emlix.com>
> >
> >  
> 
> Thanks for your review.
> >>---
> >>diff --git a/arch/xtensa/include/asm/syscall.h 
> >>b/arch/xtensa/include/asm/syscall.h
> >>index 05cebf8..76a1fb8 100644
> >>--- a/arch/xtensa/include/asm/syscall.h
> >>+++ b/arch/xtensa/include/asm/syscall.h
> >>@@ -12,7 +12,6 @@ struct pt_regs;
> >> struct sigaction;
> >> asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*);
> >> asmlinkage long xtensa_clone(unsigned long, unsigned long, struct 
> >> pt_regs*);
> >>-asmlinkage long xtensa_pipe(int __user *);
> >> asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long,
> >>     			     unsigned long, unsigned long, unsigned 
> >>     			     long);
> >> asmlinkage long xtensa_ptrace(long, long, long, long);
> >>diff --git a/arch/xtensa/include/asm/unistd.h 
> >>b/arch/xtensa/include/asm/unistd.h
> >>index c092c8f..b6880c8 100644
> >>--- a/arch/xtensa/include/asm/unistd.h
> >>+++ b/arch/xtensa/include/asm/unistd.h
> >>@@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3)
> >> #define __NR_mknod 				 36
> >> __SYSCALL( 36, sys_mknod, 3)
> >> #define __NR_pipe 				 37
> >>-__SYSCALL( 37, xtensa_pipe, 1)
> >>+__SYSCALL(37, sys_pipe, 1)
> >>    
> >
> >It would have been nice to keep the spacing but that shouldn't be a
> >show stopper..
> >
> >  
> 
> I did this, but checkpatch.pl complained about this... so I removed the
> space.

Yeah, but look at the file.  checkpatch suggests alignment that is
common for function calls, but this doesn't look like a sequence of
function calls, rather like a table - and we align other tables (like
fields in structure definitions) as well because you usually don't
read them in a linear fashion but want to spot key values immediately.

Please ignore checkpatch in this case.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch 2/2] xtensa: use generic sys_pipe()
  2009-07-23 11:18       ` Johannes Weiner
@ 2009-07-24  9:04         ` Amerigo Wang
  0 siblings, 0 replies; 11+ messages in thread
From: Amerigo Wang @ 2009-07-24  9:04 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Amerigo Wang, linux-kernel, akpm, Linus Torvalds, Al Viro, mingo,
	Chris Zankel

On Thu, Jul 23, 2009 at 01:18:34PM +0200, Johannes Weiner wrote:
>On Thu, Jul 23, 2009 at 05:47:24PM +0800, Amerigo Wang wrote:
>> Johannes Weiner wrote:



<snip>

>> >>--- a/arch/xtensa/include/asm/unistd.h
>> >>+++ b/arch/xtensa/include/asm/unistd.h
>> >>@@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3)
>> >> #define __NR_mknod 				 36
>> >> __SYSCALL( 36, sys_mknod, 3)
>> >> #define __NR_pipe 				 37
>> >>-__SYSCALL( 37, xtensa_pipe, 1)
>> >>+__SYSCALL(37, sys_pipe, 1)
>> >>    
>> >
>> >It would have been nice to keep the spacing but that shouldn't be a
>> >show stopper..
>> >
>> >  
>> 
>> I did this, but checkpatch.pl complained about this... so I removed the
>> space.
>
>Yeah, but look at the file.  checkpatch suggests alignment that is
>common for function calls, but this doesn't look like a sequence of
>function calls, rather like a table - and we align other tables (like
>fields in structure definitions) as well because you usually don't
>read them in a linear fashion but want to spot key values immediately.
>
>Please ignore checkpatch in this case.

Ok, thanks! Below is it.

--------------------------->

As suggested by Al, we can use the generic sys_pipe() instead of xtensa_pipe()
for xtensa.

Signed-off-by: WANG Cong <amwang@redhat.com>
Reviewed-by: Johannes Weiner <jw@emlix.com>
Cc: Al Viro <viro@zeniv.linux.org.uk> 
CC: Linus Torvalds <torvalds@linux-foundation.org>

---
diff --git a/arch/xtensa/include/asm/syscall.h b/arch/xtensa/include/asm/syscall.h
index 05cebf8..76a1fb8 100644
--- a/arch/xtensa/include/asm/syscall.h
+++ b/arch/xtensa/include/asm/syscall.h
@@ -12,7 +12,6 @@ struct pt_regs;
 struct sigaction;
 asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*);
 asmlinkage long xtensa_clone(unsigned long, unsigned long, struct pt_regs*);
-asmlinkage long xtensa_pipe(int __user *);
 asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long,
     			     unsigned long, unsigned long, unsigned long);
 asmlinkage long xtensa_ptrace(long, long, long, long);
diff --git a/arch/xtensa/include/asm/unistd.h b/arch/xtensa/include/asm/unistd.h
index c092c8f..b6880c8 100644
--- a/arch/xtensa/include/asm/unistd.h
+++ b/arch/xtensa/include/asm/unistd.h
@@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3)
 #define __NR_mknod 				 36
 __SYSCALL( 36, sys_mknod, 3)
 #define __NR_pipe 				 37
-__SYSCALL( 37, xtensa_pipe, 1)
+__SYSCALL( 37, sys_pipe, 1)
 #define __NR_unlink 				 38
 __SYSCALL( 38, sys_unlink, 1)
 #define __NR_rmdir 				 39
diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c
index ac15ecb..c7279be 100644
--- a/arch/xtensa/kernel/syscall.c
+++ b/arch/xtensa/kernel/syscall.c
@@ -39,24 +39,6 @@ syscall_t sys_call_table[__NR_syscall_count] /* FIXME __cacheline_aligned */= {
 #include <asm/unistd.h>
 };
 
-/*
- * xtensa_pipe() is the normal C calling standard for creating a pipe. It's not
- * the way unix traditional does this, though.
- */
-
-asmlinkage long xtensa_pipe(int __user *userfds)
-{
-	int fd[2];
-	int error;
-
-	error = do_pipe_flags(fd, 0);
-	if (!error) {
-		if (copy_to_user(userfds, fd, 2 * sizeof(int)))
-			error = -EFAULT;
-	}
-	return error;
-}
-
 
 asmlinkage long xtensa_mmap2(unsigned long addr, unsigned long len,
    			     unsigned long prot, unsigned long flags,

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Patch 1/2] ia32: use generic sys_pipe()
  2009-07-22  9:08 [Patch 1/2] ia32: use generic sys_pipe() Amerigo Wang
  2009-07-22  9:08 ` [Patch 2/2] xtensa: " Amerigo Wang
@ 2009-08-04 12:24 ` Ingo Molnar
  2009-08-05 11:38   ` Amerigo Wang
  2009-09-12 13:30   ` Al Viro
  1 sibling, 2 replies; 11+ messages in thread
From: Ingo Molnar @ 2009-08-04 12:24 UTC (permalink / raw)
  To: Amerigo Wang; +Cc: linux-kernel, akpm, Al Viro, Linus Torvalds


* Amerigo Wang <amwang@redhat.com> wrote:

> As suggested by Al, it's better to use the generic sys_pipe() for 
> ia32.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> CC: Linus Torvalds <torvalds@linux-foundation.org>

> --- a/arch/x86/ia32/sys_ia32.c
> +++ b/arch/x86/ia32/sys_ia32.c
> @@ -189,20 +189,6 @@ asmlinkage long sys32_mprotect(unsigned long start, size_t len,
>  	return sys_mprotect(start, len, prot);
>  }
>  
> -asmlinkage long sys32_pipe(int __user *fd)
> -{
> -	int retval;
> -	int fds[2];
> -
> -	retval = do_pipe_flags(fds, 0);
> -	if (retval)
> -		goto out;
> -	if (copy_to_user(fd, fds, sizeof(fds)))
> -		retval = -EFAULT;
> -out:
> -	return retval;
> -}

Please _ALWAYS_ mention the change in behavior in the changelog, 
just in case someone ends up bisecting it. I only found out when i 
reviewed the two syscalls out of caution.

The syscall you remove kept stale fd's around in case of -EFAULT 
from copy_to_user(). The generic version does an explicit close of 
those files:

                        sys_close(fd[0]);
                        sys_close(fd[1]);
                        error = -EFAULT;

The generic version looks like the better choice to me but this 
difference should be mentioned in the changelog nevertheless, just 
in case some buggy app runs into this issue.

	Ingo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch 1/2] ia32: use generic sys_pipe()
  2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar
@ 2009-08-05 11:38   ` Amerigo Wang
  2009-09-12 13:30   ` Al Viro
  1 sibling, 0 replies; 11+ messages in thread
From: Amerigo Wang @ 2009-08-05 11:38 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: linux-kernel, akpm, Al Viro, Linus Torvalds

Ingo Molnar wrote:
> * Amerigo Wang <amwang@redhat.com> wrote:
>
>   
>> As suggested by Al, it's better to use the generic sys_pipe() for 
>> ia32.
>>
>> Signed-off-by: WANG Cong <amwang@redhat.com>
>> Cc: Ingo Molnar <mingo@elte.hu>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> CC: Linus Torvalds <torvalds@linux-foundation.org>
>>     
>
>   
>> --- a/arch/x86/ia32/sys_ia32.c
>> +++ b/arch/x86/ia32/sys_ia32.c
>> @@ -189,20 +189,6 @@ asmlinkage long sys32_mprotect(unsigned long start, size_t len,
>>  	return sys_mprotect(start, len, prot);
>>  }
>>  
>> -asmlinkage long sys32_pipe(int __user *fd)
>> -{
>> -	int retval;
>> -	int fds[2];
>> -
>> -	retval = do_pipe_flags(fds, 0);
>> -	if (retval)
>> -		goto out;
>> -	if (copy_to_user(fd, fds, sizeof(fds)))
>> -		retval = -EFAULT;
>> -out:
>> -	return retval;
>> -}
>>     
>
> Please _ALWAYS_ mention the change in behavior in the changelog, 
> just in case someone ends up bisecting it. I only found out when i 
> reviewed the two syscalls out of caution.
>
> The syscall you remove kept stale fd's around in case of -EFAULT 
> from copy_to_user(). The generic version does an explicit close of 
> those files:
>
>                         sys_close(fd[0]);
>                         sys_close(fd[1]);
>                         error = -EFAULT;
>
> The generic version looks like the better choice to me but this 
> difference should be mentioned in the changelog nevertheless, just 
> in case some buggy app runs into this issue.
>   

Hi, yes, exactly.
Thank you! I will be careful next time.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch 1/2] ia32: use generic sys_pipe()
  2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar
  2009-08-05 11:38   ` Amerigo Wang
@ 2009-09-12 13:30   ` Al Viro
  2009-09-12 14:48     ` Al Viro
  2009-09-18  9:46     ` Ingo Molnar
  1 sibling, 2 replies; 11+ messages in thread
From: Al Viro @ 2009-09-12 13:30 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Amerigo Wang, linux-kernel, akpm, Linus Torvalds

On Tue, Aug 04, 2009 at 02:24:32PM +0200, Ingo Molnar wrote:

> Please _ALWAYS_ mention the change in behavior in the changelog, 
> just in case someone ends up bisecting it. I only found out when i 
> reviewed the two syscalls out of caution.
> 
> The syscall you remove kept stale fd's around in case of -EFAULT 
> from copy_to_user(). The generic version does an explicit close of 
> those files:
> 
>                         sys_close(fd[0]);
>                         sys_close(fd[1]);
>                         error = -EFAULT;

> The generic version looks like the better choice to me but this 
> difference should be mentioned in the changelog nevertheless, just 
> in case some buggy app runs into this issue.

It's not a matter of QOI, actually - sys32_pipe() is supposed to do what
sys_pipe() would do on i386 host.  So any difference in handling of an
error case is simply wrong.

Whether we want those sys_close() in sys_pipe() or not is a separate
question, but we definitely want the same behaviour when 32bit process is
run natively and when it's run on amd64.  So sys32_pipe() has no business
existing at all.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch 1/2] ia32: use generic sys_pipe()
  2009-09-12 13:30   ` Al Viro
@ 2009-09-12 14:48     ` Al Viro
  2009-09-18  9:46     ` Ingo Molnar
  1 sibling, 0 replies; 11+ messages in thread
From: Al Viro @ 2009-09-12 14:48 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: Amerigo Wang, linux-kernel, akpm, Linus Torvalds

On Sat, Sep 12, 2009 at 02:30:14PM +0100, Al Viro wrote:
> On Tue, Aug 04, 2009 at 02:24:32PM +0200, Ingo Molnar wrote:
> 
> > Please _ALWAYS_ mention the change in behavior in the changelog, 
> > just in case someone ends up bisecting it. I only found out when i 
> > reviewed the two syscalls out of caution.

[snip]

> > The generic version looks like the better choice to me but this 
> > difference should be mentioned in the changelog nevertheless, just 
> > in case some buggy app runs into this issue.
> 
> It's not a matter of QOI, actually - sys32_pipe() is supposed to do what
> sys_pipe() would do on i386 host.  So any difference in handling of an
> error case is simply wrong.
> 
> Whether we want those sys_close() in sys_pipe() or not is a separate
> question, but we definitely want the same behaviour when 32bit process is
> run natively and when it's run on amd64.  So sys32_pipe() has no business
> existing at all.

PS: said that, mentioning the change in commit message is a Good Idea(tm),
obviously...

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch 1/2] ia32: use generic sys_pipe()
  2009-09-12 13:30   ` Al Viro
  2009-09-12 14:48     ` Al Viro
@ 2009-09-18  9:46     ` Ingo Molnar
  1 sibling, 0 replies; 11+ messages in thread
From: Ingo Molnar @ 2009-09-18  9:46 UTC (permalink / raw)
  To: Al Viro; +Cc: Amerigo Wang, linux-kernel, akpm, Linus Torvalds


* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Tue, Aug 04, 2009 at 02:24:32PM +0200, Ingo Molnar wrote:
> 
> > Please _ALWAYS_ mention the change in behavior in the changelog, 
> > just in case someone ends up bisecting it. I only found out when i 
> > reviewed the two syscalls out of caution.
> > 
> > The syscall you remove kept stale fd's around in case of -EFAULT 
> > from copy_to_user(). The generic version does an explicit close of 
> > those files:
> > 
> >                         sys_close(fd[0]);
> >                         sys_close(fd[1]);
> >                         error = -EFAULT;
> 
> > The generic version looks like the better choice to me but this 
> > difference should be mentioned in the changelog nevertheless, just 
> > in case some buggy app runs into this issue.
> 
> It's not a matter of QOI, actually - sys32_pipe() is supposed to do 
> what sys_pipe() would do on i386 host.  So any difference in handling 
> of an error case is simply wrong.
> 
> Whether we want those sys_close() in sys_pipe() or not is a separate 
> question, but we definitely want the same behaviour when 32bit process 
> is run natively and when it's run on amd64.  So sys32_pipe() has no 
> business existing at all.

I generally agree except the notion that we 'must' do this change.

This is a (small) past ABI sin of ours and we have to live with the 
consequences without breaking apps. We didnt break any it appears so 
it's all good.

	Ingo

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2009-09-18  9:46 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-07-22  9:08 [Patch 1/2] ia32: use generic sys_pipe() Amerigo Wang
2009-07-22  9:08 ` [Patch 2/2] xtensa: " Amerigo Wang
2009-07-22 10:10   ` Johannes Weiner
2009-07-23  9:47     ` Amerigo Wang
2009-07-23 11:18       ` Johannes Weiner
2009-07-24  9:04         ` Amerigo Wang
2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar
2009-08-05 11:38   ` Amerigo Wang
2009-09-12 13:30   ` Al Viro
2009-09-12 14:48     ` Al Viro
2009-09-18  9:46     ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox