sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Geert Uytterhoeven]

On Fri, Sep 21, 2012 at 6:51 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> That brings up another question though - when was kcmp added to x86, and
> why aren't we getting notifications from checksyscalls.sh that ARM hasn't
> been updated?
>
> It seems to be that the script was broken, and no one has noticed.

It seems Heiko did notice: http://www.serverphorums.com/read.php?12,559093

Now, I'm a bit puzzled by what follows: Heiko proposes a patch to
ignore sys_kcmp,
as it's x86-specific, which is acked by Cyrill. Then it suddenly
switches to Heiko
enabling it on s390 anyway?

> commit 29dc54c673ea2531d589400badb4ada5f5f60dae
> Author: H. Peter Anvin <hpa@linux.intel.com>
> Date:   Fri Nov 11 15:57:53 2011 -0800
>
>     checksyscalls: Use arch/x86/syscalls/syscall_32.tbl as source
>
>     Use the new arch/x86/syscalls/syscall_32.tbl file as source instead of
>     arch/x86/include/asm/unistd_32.h.
>
>     Cc: Michal Marek <mmarek@suse.cz>
>     Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>     Cc: Sam Ravnborg <sam@ravnborg.org>
>     Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
>
> is the culpret, more specifically this fragment:
>
> +           echo <<EOF
> +#if !defined(__NR_${name}) && !defined(__IGNORE_${name})
> +#warning syscall ${name} not implemented
> +#endif
> +EOF
>
> "echo <<EOF" doesn't read from its stdin and output to stdout, so the
> above just generates a blank line for each entry in x86's syscalls_32.tbl,
> resulting in the compiler doing no checking for us.
>
> That "echo <<EOF" should be "cat <<EOF"... and with that fixed we get:
>
> <stdin>:1220:2: warning: #warning syscall kcmp not implemented
>
> So, actually, I want to add this kcmp syscall _now_ into -rc which I'm
> afraid will break your patch, and bump your syscall number on ARM to 379.

With a CC to stable for v3.5? ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Cyrill Gorcunov]

On Sat, Sep 22, 2012 at 12:56:42PM +0200, Geert Uytterhoeven wrote:
> On Fri, Sep 21, 2012 at 6:51 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> > That brings up another question though - when was kcmp added to x86, and
> > why aren't we getting notifications from checksyscalls.sh that ARM hasn't
> > been updated?
> >
> > It seems to be that the script was broken, and no one has noticed.
> 
> It seems Heiko did notice: http://www.serverphorums.com/read.php?12,559093
> 
> Now, I'm a bit puzzled by what follows: Heiko proposes a patch to
> ignore sys_kcmp,
> as it's x86-specific, which is acked by Cyrill. Then it suddenly

hpa@ pointed that better approach is to implement kcmp on other archs
after i've acked the patch. so then Heiko provided a patch for s390.

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Russell King]

On Sat, Sep 22, 2012 at 03:45:49PM +0400, Cyrill Gorcunov wrote:
> On Sat, Sep 22, 2012 at 12:56:42PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Sep 21, 2012 at 6:51 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> > > That brings up another question though - when was kcmp added to x86, and
> > > why aren't we getting notifications from checksyscalls.sh that ARM hasn't
> > > been updated?
> > >
> > > It seems to be that the script was broken, and no one has noticed.
> > 
> > It seems Heiko did notice: http://www.serverphorums.com/read.php?12,559093
> > 
> > Now, I'm a bit puzzled by what follows: Heiko proposes a patch to
> > ignore sys_kcmp,
> > as it's x86-specific, which is acked by Cyrill. Then it suddenly
> 
> hpa@ pointed that better approach is to implement kcmp on other archs
> after i've acked the patch. so then Heiko provided a patch for s390.

I discussed with hpa yesterday, and it seems the situation is as follows:

1. There exists a patch to fix checksyscalls.sh, and it's allegedly sitting
   in akpm's tree, and no one knows why it's just sitting there and hasn't
   been merged upstream.

2. There allegedly exists a patch to remove x86isms from sys_kcmp -
   allegedly also in akpm's tree.  However, I've looked through the code in
   mainline, and nothing stands out.  Ralf Beachle also said yesterday that
   he has looked through from the MIPS PoV and also can't see any x86isms,
   so we're both thinking that it should merely have the x86 dependency
   removed.

3. Until the x86 dependency is gone (that depends on what akpm proposes to
   do with the patches he's allegedly sitting on), non-x86 arches can only
   reserve the syscall, and add an IGNORE for it.

Maybe akpm can provide some input to this thread, and let us know what the
intentions are for checksyscalls.sh and kernel/kcmp.c, and whether he does
indeed have outstanding patches for these.

It would be good to at least get checksyscalls.sh fixed so arch maintainers
get their warnings for new syscalls back.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Ralf Baechle]

On Sat, Sep 22, 2012 at 02:20:46PM +0100, Russell King wrote:

> 2. There allegedly exists a patch to remove x86isms from sys_kcmp -
>    allegedly also in akpm's tree.  However, I've looked through the code in
>    mainline, and nothing stands out.  Ralf Beachle also said yesterday that
>    he has looked through from the MIPS PoV and also can't see any x86isms,
>    so we're both thinking that it should merely have the x86 dependency
>    removed.
> 
> 3. Until the x86 dependency is gone (that depends on what akpm proposes to
>    do with the patches he's allegedly sitting on), non-x86 arches can only
>    reserve the syscall, and add an IGNORE for it.

There is a weak definition provided in kernel/sys_ni.c so it actually can
be properly wired up in preparation for the day when the dependency in
Kconfig gets fixed.

> It would be good to at least get checksyscalls.sh fixed so arch maintainers
> get their warnings for new syscalls back.

Indeed.  That script has become just too important.

  Ralf

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Cyrill Gorcunov]

On Sat, Sep 22, 2012 at 03:38:47PM +0200, Ralf Baechle wrote:
> On Sat, Sep 22, 2012 at 02:20:46PM +0100, Russell King wrote:
> 
> > 2. There allegedly exists a patch to remove x86isms from sys_kcmp -
> >    allegedly also in akpm's tree.  However, I've looked through the code in
> >    mainline, and nothing stands out.  Ralf Beachle also said yesterday that
> >    he has looked through from the MIPS PoV and also can't see any x86isms,
> >    so we're both thinking that it should merely have the x86 dependency
> >    removed.
> > 
> > 3. Until the x86 dependency is gone (that depends on what akpm proposes to
> >    do with the patches he's allegedly sitting on), non-x86 arches can only
> >    reserve the syscall, and add an IGNORE for it.
> 
> There is a weak definition provided in kernel/sys_ni.c so it actually can
> be properly wired up in preparation for the day when the dependency in
> Kconfig gets fixed.
> 
> > It would be good to at least get checksyscalls.sh fixed so arch maintainers
> > get their warnings for new syscalls back.
> 
> Indeed.  That script has become just too important.

These are the patches from linux-next/akpm

commit 6dfc4cffd24b0c7dc04ca36471a4a6b2a9fc1377
Author: Heiko Carstens <heiko.carstens@de.ibm.com>
Date:   Fri Sep 21 11:01:56 2012 +1000

    syscalls: make kcmp syscall available for all architectures

commit 7f36f199e958ce7009285cd887323cb222ed6b1e
Author: Heiko Carstens <heiko.carstens@de.ibm.com>
Date:   Fri Sep 21 10:57:07 2012 +1000

    checksyscalls: fix "here document" handling

So I guess this tree in a good shape just checksyscalls.sh fix should
go upstream, right?

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Andrew Morton]

On Sat, 22 Sep 2012 14:20:46 +0100 Russell King <rmk@arm.linux.org.uk> wrote:

> On Sat, Sep 22, 2012 at 03:45:49PM +0400, Cyrill Gorcunov wrote:
> > On Sat, Sep 22, 2012 at 12:56:42PM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Sep 21, 2012 at 6:51 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> > > > That brings up another question though - when was kcmp added to x86, and
> > > > why aren't we getting notifications from checksyscalls.sh that ARM hasn't
> > > > been updated?
> > > >
> > > > It seems to be that the script was broken, and no one has noticed.
> > > 
> > > It seems Heiko did notice: http://www.serverphorums.com/read.php?12,559093
> > > 
> > > Now, I'm a bit puzzled by what follows: Heiko proposes a patch to
> > > ignore sys_kcmp,
> > > as it's x86-specific, which is acked by Cyrill. Then it suddenly
> > 
> > hpa@ pointed that better approach is to implement kcmp on other archs
> > after i've acked the patch. so then Heiko provided a patch for s390.
> 
> I discussed with hpa yesterday, and it seems the situation is as follows:
> 
> 1. There exists a patch to fix checksyscalls.sh, and it's allegedly sitting
>    in akpm's tree, and no one knows why it's just sitting there and hasn't
>    been merged upstream.

People sometimes just reply to my commit emails, ignoring the
reply-to:lkml and the "Before you just go and hit reply" request.  I could
start cc'ing the lists like tip-bot, but that seems a bit noisy.

> 2. There allegedly exists a patch to remove x86isms from sys_kcmp -
>    allegedly also in akpm's tree.  However, I've looked through the code in
>    mainline, and nothing stands out.  Ralf Beachle also said yesterday that
>    he has looked through from the MIPS PoV and also can't see any x86isms,
>    so we're both thinking that it should merely have the x86 dependency
>    removed.

http://ozlabs.org/~akpm/mmotm/broken-out/syscalls-make-kcmp-syscall-available-for-all-architectures.patch

I have that queued for 3.7.  There is of course a little risk here.  We
do have a test in tools/testing/selftests/kcmp/ - I suggest that arch
people run it!  In fact all the tools/testing/selftests should execute
successfully on all architectures - if not, please let's fix things
up.

> 3. Until the x86 dependency is gone (that depends on what akpm proposes to
>    do with the patches he's allegedly sitting on), non-x86 arches can only
>    reserve the syscall, and add an IGNORE for it.
> 
> Maybe akpm can provide some input to this thread, and let us know what the
> intentions are for checksyscalls.sh and kernel/kcmp.c, and whether he does
> indeed have outstanding patches for these.
> 
> It would be good to at least get checksyscalls.sh fixed so arch maintainers
> get their warnings for new syscalls back.

http://ozlabs.org/~akpm/mmotm/broken-out/checksyscalls-fix-here-document-handling.patch

I had it queued for 3.7.  I now see that was a mistake and I'll get it
into 3.6.

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Mark Salter]

On Sat, 2012-09-22 at 11:47 -0700, Andrew Morton wrote:
> > 2. There allegedly exists a patch to remove x86isms from sys_kcmp -
> >    allegedly also in akpm's tree.  However, I've looked through the code in
> >    mainline, and nothing stands out.  Ralf Beachle also said yesterday that
> >    he has looked through from the MIPS PoV and also can't see any x86isms,
> >    so we're both thinking that it should merely have the x86 dependency
> >    removed.
> 
> http://ozlabs.org/~akpm/mmotm/broken-out/syscalls-make-kcmp-syscall-available-for-all-architectures.patch

The following is needed to get rid of the syscall warning on architectures
using the generic syscall list:

diff --git a/include/asm-generic/unistd.h b/include/asm-generic/unistd.h
index 991ef01..3748ec9 100644
--- a/include/asm-generic/unistd.h
+++ b/include/asm-generic/unistd.h
@@ -691,9 +691,11 @@ __SC_COMP(__NR_process_vm_readv, sys_process_vm_readv, \
 #define __NR_process_vm_writev 271
 __SC_COMP(__NR_process_vm_writev, sys_process_vm_writev, \
           compat_sys_process_vm_writev)
+#define __NR_kcmp 272
+__SYSCALL(__NR_kcmp, sys_kcmp)
 
 #undef __NR_syscalls
-#define __NR_syscalls 272
+#define __NR_syscalls 273

> 
> I have that queued for 3.7.  There is of course a little risk here.  We
> do have a test in tools/testing/selftests/kcmp/ - I suggest that arch
> people run it!  In fact all the tools/testing/selftests should execute
> successfully on all architectures - if not, please let's fix things
> up.

I ran into a build error on C6X (no-MMU) when I enabled CHECKPOINT_RESTORE:

  linux-next/kernel/sys.c: In function 'prctl_set_mm':
  linux-next/kernel/sys.c:1869:34: error: 'mmap_min_addr' undeclared (first use in this function)

I got past that with:

diff --git a/include/linux/security.h b/include/linux/security.h
index 01ef030..14e394d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -118,6 +118,7 @@ void reset_security_ops(void);
 extern unsigned long mmap_min_addr;
 extern unsigned long dac_mmap_min_addr;
 #else
+#define mmap_min_addr          0UL
 #define dac_mmap_min_addr      0UL
 #endif

Looking at kcmp_test.c, it uses fork, so won't work without MMU. Is
the kcmp syscall even meaningful for no-MMU? I suppose some of the
tests in kcmp_test.c could be accomplished using clone directly
rather than fork.

--Mark

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Cyrill Gorcunov]

On Mon, Sep 24, 2012 at 12:21:46PM -0400, Mark Salter wrote:
> diff --git a/include/asm-generic/unistd.h b/include/asm-generic/unistd.h
> index 991ef01..3748ec9 100644
> --- a/include/asm-generic/unistd.h
> +++ b/include/asm-generic/unistd.h
> @@ -691,9 +691,11 @@ __SC_COMP(__NR_process_vm_readv, sys_process_vm_readv, \
>  #define __NR_process_vm_writev 271
>  __SC_COMP(__NR_process_vm_writev, sys_process_vm_writev, \
>            compat_sys_process_vm_writev)
> +#define __NR_kcmp 272
> +__SYSCALL(__NR_kcmp, sys_kcmp)
>  
>  #undef __NR_syscalls
> -#define __NR_syscalls 272
> +#define __NR_syscalls 273
> 
> > 
> > I have that queued for 3.7.  There is of course a little risk here.  We
> > do have a test in tools/testing/selftests/kcmp/ - I suggest that arch
> > people run it!  In fact all the tools/testing/selftests should execute
> > successfully on all architectures - if not, please let's fix things
> > up.
> 
> I ran into a build error on C6X (no-MMU) when I enabled CHECKPOINT_RESTORE:
> 
>   linux-next/kernel/sys.c: In function 'prctl_set_mm':
>   linux-next/kernel/sys.c:1869:34: error: 'mmap_min_addr' undeclared (first use in this function)
> 
> I got past that with:
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 01ef030..14e394d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -118,6 +118,7 @@ void reset_security_ops(void);
>  extern unsigned long mmap_min_addr;
>  extern unsigned long dac_mmap_min_addr;
>  #else
> +#define mmap_min_addr          0UL
>  #define dac_mmap_min_addr      0UL
>  #endif
> 
> Looking at kcmp_test.c, it uses fork, so won't work without MMU. Is
> the kcmp syscall even meaningful for no-MMU? I suppose some of the
> tests in kcmp_test.c could be accomplished using clone directly
> rather than fork.

Hi Mark, good catch! I think it should be two patches then. As to kcmp_test.c,
I've been using fork there simply because it was simplier. The clone could
be used as well. (kcmp should work even with mmu emulated i think).

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Cyrill Gorcunov]

On Mon, Sep 24, 2012 at 08:49:42PM +0400, Cyrill Gorcunov wrote:
> > I got past that with:
> > 
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 01ef030..14e394d 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -118,6 +118,7 @@ void reset_security_ops(void);
> >  extern unsigned long mmap_min_addr;
> >  extern unsigned long dac_mmap_min_addr;
> >  #else
> > +#define mmap_min_addr          0UL
> >  #define dac_mmap_min_addr      0UL
> >  #endif
> > 

I think better to add CONFIG_MMU test here.
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: prctl: prctl_set_mm -- Don't test for mmap_min_addr on non-MMU config

In case if CONFIG_MMU is not set the @mmap_min_addr
is undefined leading to build error. Thus test for
it iif CONFIG_MMU is present.

Note this code snippet depends on CONFIG_CHECKPOINT_RESTORE=y.

Reported-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 kernel/sys.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux-2.6.git/kernel/sys.c
===================================================================
--- linux-2.6.git.orig/kernel/sys.c
+++ linux-2.6.git/kernel/sys.c
@@ -1865,7 +1865,11 @@ static int prctl_set_mm(int opt, unsigne
 	if (opt == PR_SET_MM_EXE_FILE)
 		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
 
+#ifdef CONFIG_MMU
 	if (addr >= TASK_SIZE || addr < mmap_min_addr)
+#else
+	if (addr >= TASK_SIZE)
+#endif
 		return -EINVAL;
 
 	error = -EINVAL;

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Mark Salter]

On Mon, 2012-09-24 at 21:42 +0400, Cyrill Gorcunov wrote:
> On Mon, Sep 24, 2012 at 08:49:42PM +0400, Cyrill Gorcunov wrote:
> > > I got past that with:
> > > 
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index 01ef030..14e394d 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -118,6 +118,7 @@ void reset_security_ops(void);
> > >  extern unsigned long mmap_min_addr;
> > >  extern unsigned long dac_mmap_min_addr;
> > >  #else
> > > +#define mmap_min_addr          0UL
> > >  #define dac_mmap_min_addr      0UL
> > >  #endif
> > > 
> 
> I think better to add CONFIG_MMU test here.

Well, my patch was just something quick to get the kernel to build, but
thinking about it a bit, I still prefer it. The CONFIG_MMU check is in
security.h already so I think it is less clutter and better for future
code which may use mmap_min_addr. In any case, the compiler will drop
any test for "x < 0UL" so the end result is the same.

> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: prctl: prctl_set_mm -- Don't test for mmap_min_addr on non-MMU config
> 
> In case if CONFIG_MMU is not set the @mmap_min_addr
> is undefined leading to build error. Thus test for
> it iif CONFIG_MMU is present.
> 
> Note this code snippet depends on CONFIG_CHECKPOINT_RESTORE=y.
> 
> Reported-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  kernel/sys.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: linux-2.6.git/kernel/sys.c
> ===================================================================
> --- linux-2.6.git.orig/kernel/sys.c
> +++ linux-2.6.git/kernel/sys.c
> @@ -1865,7 +1865,11 @@ static int prctl_set_mm(int opt, unsigne
>  	if (opt == PR_SET_MM_EXE_FILE)
>  		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
>  
> +#ifdef CONFIG_MMU
>  	if (addr >= TASK_SIZE || addr < mmap_min_addr)
> +#else
> +	if (addr >= TASK_SIZE)
> +#endif
>  		return -EINVAL;
>  
>  	error = -EINVAL;

Re: sys_kcmp

[Eric W. Biederman]

Cyrill Gorcunov <gorcunov@openvz.org> writes:

> On Mon, Sep 24, 2012 at 08:49:42PM +0400, Cyrill Gorcunov wrote:
>> > I got past that with:
>> > 
>> > diff --git a/include/linux/security.h b/include/linux/security.h
>> > index 01ef030..14e394d 100644
>> > --- a/include/linux/security.h
>> > +++ b/include/linux/security.h
>> > @@ -118,6 +118,7 @@ void reset_security_ops(void);
>> >  extern unsigned long mmap_min_addr;
>> >  extern unsigned long dac_mmap_min_addr;
>> >  #else
>> > +#define mmap_min_addr          0UL
>> >  #define dac_mmap_min_addr      0UL
>> >  #endif
>> > 
>
> I think better to add CONFIG_MMU test here.
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: prctl: prctl_set_mm -- Don't test for mmap_min_addr on non-MMU config
>
> In case if CONFIG_MMU is not set the @mmap_min_addr
> is undefined leading to build error. Thus test for
> it iif CONFIG_MMU is present.
>
> Note this code snippet depends on CONFIG_CHECKPOINT_RESTORE=y.
>
> Reported-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  kernel/sys.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> Index: linux-2.6.git/kernel/sys.c
> ===================================================================
> --- linux-2.6.git.orig/kernel/sys.c
> +++ linux-2.6.git/kernel/sys.c
> @@ -1865,7 +1865,11 @@ static int prctl_set_mm(int opt, unsigne
>  	if (opt == PR_SET_MM_EXE_FILE)
>  		return prctl_set_mm_exe_file(mm, (unsigned int)addr);
>  
> +#ifdef CONFIG_MMU
>  	if (addr >= TASK_SIZE || addr < mmap_min_addr)
> +#else
> +	if (addr >= TASK_SIZE)
> +#endif

I expect what you want is a call to access_ok, rather than hard coding
details about task layout here.  This test certainly looks wrong
for a 32bit process on a 64bit kernel. If I read your test right it
appears I can set values of say 0x100000000 on a 32bit process...

As for mmap_min_addr I would expect your find_vma check would make that
test unnecessary, simply by not finding a vma...

Eric

>  		return -EINVAL;
>  
>  	error = -EINVAL;

Re: sys_kcmp

[Cyrill Gorcunov]

On Mon, Sep 24, 2012 at 11:29:09AM -0700, Eric W. Biederman wrote:
> >  
> > +#ifdef CONFIG_MMU
> >  	if (addr >= TASK_SIZE || addr < mmap_min_addr)
> > +#else
> > +	if (addr >= TASK_SIZE)
> > +#endif
> 
> I expect what you want is a call to access_ok, rather than hard coding
> details about task layout here.  This test certainly looks wrong
> for a 32bit process on a 64bit kernel. If I read your test right it
> appears I can set values of say 0x100000000 on a 32bit process...
> 
> As for mmap_min_addr I would expect your find_vma check would make that
> test unnecessary, simply by not finding a vma...

Good point, Eric, thanks! I'm cooking a new patch now.

Re: sys_kcmp (was: Re: [PATCH 1/2] ARM: add finit_module syscall to ARM)

[Cyrill Gorcunov]

On Mon, Sep 24, 2012 at 02:16:09PM -0400, Mark Salter wrote:
> > > >  #else
> > > > +#define mmap_min_addr          0UL
> > > >  #define dac_mmap_min_addr      0UL
> > > >  #endif
> > > > 
> > 
> > I think better to add CONFIG_MMU test here.
> 
> Well, my patch was just something quick to get the kernel to build, but
> thinking about it a bit, I still prefer it. The CONFIG_MMU check is in
> security.h already so I think it is less clutter and better for future
> code which may use mmap_min_addr. In any case, the compiler will drop
> any test for "x < 0UL" so the end result is the same.

Well, sure compiler should optimize it out but frankly i prefer #ifdef here.
Anyway, i'm cooking some new patch. Will post once it's ready and tested.

Re: sys_kcmp

[Cyrill Gorcunov]

On Mon, Sep 24, 2012 at 10:51:19PM +0400, Cyrill Gorcunov wrote:
> > I expect what you want is a call to access_ok, rather than hard coding
> > details about task layout here.  This test certainly looks wrong
> > for a 32bit process on a 64bit kernel. If I read your test right it
> > appears I can set values of say 0x100000000 on a 32bit process...
> > 
> > As for mmap_min_addr I would expect your find_vma check would make that
> > test unnecessary, simply by not finding a vma...
> 
> Good point, Eric, thanks! I'm cooking a new patch now.

Btw, Eric, I somehow miss one bit -- how would you set this 0x100000000
if TASK_SIZE is a macro which does check for TIF_ADDR32 and sets limit
acordingly? What i'm missing?

Re: sys_kcmp

[Eric W. Biederman]

Cyrill Gorcunov <gorcunov@openvz.org> writes:

> On Mon, Sep 24, 2012 at 10:51:19PM +0400, Cyrill Gorcunov wrote:
>> > I expect what you want is a call to access_ok, rather than hard coding
>> > details about task layout here.  This test certainly looks wrong
>> > for a 32bit process on a 64bit kernel. If I read your test right it
>> > appears I can set values of say 0x100000000 on a 32bit process...
>> > 
>> > As for mmap_min_addr I would expect your find_vma check would make that
>> > test unnecessary, simply by not finding a vma...
>> 
>> Good point, Eric, thanks! I'm cooking a new patch now.
>
> Btw, Eric, I somehow miss one bit -- how would you set this 0x100000000
> if TASK_SIZE is a macro which does check for TIF_ADDR32 and sets limit
> acordingly? What i'm missing?

How odd.  Last time I had looked TASK_SIZE was a simple constant.

Still I wonder a little if all architectures currently run from 0 to
TASK_SIZE, for address space available.  I seem to remember there have
been some exceptions to that rule.  But I can't recall what they were.

Eric

Re: sys_kcmp

[Cyrill Gorcunov]

On Mon, Sep 24, 2012 at 01:44:47PM -0700, Eric W. Biederman wrote:
> Cyrill Gorcunov <gorcunov@openvz.org> writes:
> 
> > On Mon, Sep 24, 2012 at 10:51:19PM +0400, Cyrill Gorcunov wrote:
> >> > I expect what you want is a call to access_ok, rather than hard coding
> >> > details about task layout here.  This test certainly looks wrong
> >> > for a 32bit process on a 64bit kernel. If I read your test right it
> >> > appears I can set values of say 0x100000000 on a 32bit process...
> >> > 
> >> > As for mmap_min_addr I would expect your find_vma check would make that
> >> > test unnecessary, simply by not finding a vma...
> >> 
> >> Good point, Eric, thanks! I'm cooking a new patch now.
> >
> > Btw, Eric, I somehow miss one bit -- how would you set this 0x100000000
> > if TASK_SIZE is a macro which does check for TIF_ADDR32 and sets limit
> > acordingly? What i'm missing?
> 
> How odd.  Last time I had looked TASK_SIZE was a simple constant.

Ah, I see.

> Still I wonder a little if all architectures currently run from 0 to
> TASK_SIZE, for address space available.  I seem to remember there have
> been some exceptions to that rule.  But I can't recall what they were.

Actually I;ve tuned up the code to use access_ok instead but now I'm trying
to fugure out situation if it can somehow affect c/r process (well, i've
ran all test cases we use for c/r and all are passed well, but still...).

Mark, after some more thinking, I agree that your proposal with min-address
should work better than mine explicit CONFIG_MMU. Could you please send
your patch for that? As to access_ok -- gimme some more time, i need to double
check everything and I'll patch the code on top of your patch a bit later, ok?