[PATCH] mm: enable RLIMIT_DATA by default with workaround for valgrind

[PATCH] mm: enable RLIMIT_DATA by default with workaround for valgrind

[Konstantin Khlebnikov]

Since commit 84638335900f ("mm: rework virtual memory accounting")
RLIMIT_DATA limits both brk() and private mmap() but this's disabled by
default because of incompatibility with older versions of valgrind.

Valgrind always set limit to zero and fails if RLIMIT_DATA is enabled.
Fortunately it changes only rlim_cur and keeps rlim_max for reverting
limit back when needed.

This patch checks current usage also against rlim_max if rlim_cur is zero.
Size of brk is still checked against rlim_cur, so this part is completely
compatible - zero rlim_cur forbids brk() but allows private mmap().

Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com>
Link: http://lkml.kernel.org/r/56A28613.5070104@de.ibm.com
---
 mm/mmap.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index bd2e1a533bc1..317e45409893 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -70,7 +70,7 @@ const int mmap_rnd_compat_bits_max = CONFIG_ARCH_MMAP_RND_COMPAT_BITS_MAX;
 int mmap_rnd_compat_bits __read_mostly = CONFIG_ARCH_MMAP_RND_COMPAT_BITS;
 #endif
 
-static bool ignore_rlimit_data = true;
+static bool ignore_rlimit_data = false;
 core_param(ignore_rlimit_data, ignore_rlimit_data, bool, 0644);
 
 static void unmap_region(struct mm_struct *mm,
@@ -2891,13 +2891,11 @@ bool may_expand_vm(struct mm_struct *mm, vm_flags_t flags, unsigned long npages)
 
 	if (is_data_mapping(flags) &&
 	    mm->data_vm + npages > rlimit(RLIMIT_DATA) >> PAGE_SHIFT) {
-		if (ignore_rlimit_data)
-			pr_warn_once("%s (%d): VmData %lu exceed data ulimit %lu. Will be forbidden soon.\n",
-				     current->comm, current->pid,
-				     (mm->data_vm + npages) << PAGE_SHIFT,
-				     rlimit(RLIMIT_DATA));
-		else
-			return false;
+		/* Workaround for Valgrind */
+		if (rlimit(RLIMIT_DATA) == 0 && mm->data_vm + npages <=
+				rlimit_max(RLIMIT_DATA) >> PAGE_SHIFT)
+			return true;
+		return ignore_rlimit_data;
 	}
 
 	return true;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: enable RLIMIT_DATA by default with workaround for valgrind

[Cyrill Gorcunov]

On Sun, Apr 24, 2016 at 11:07:23AM +0300, Konstantin Khlebnikov wrote:
> Since commit 84638335900f ("mm: rework virtual memory accounting")
> RLIMIT_DATA limits both brk() and private mmap() but this's disabled by
> default because of incompatibility with older versions of valgrind.
...
Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org>

Thank you!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: enable RLIMIT_DATA by default with workaround for valgrind

[Linus Torvalds]

On Sun, Apr 24, 2016 at 1:07 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote:
>
> This patch checks current usage also against rlim_max if rlim_cur is zero.
> Size of brk is still checked against rlim_cur, so this part is completely
> compatible - zero rlim_cur forbids brk() but allows private mmap().

The logic looks reasonable to me. My first reaction was that "but then
any process can set the limit to zero, and actually increase limits",
but witht he hard limit always being checked that's ok - the process
could have just set the soft limit to the hard limit instead.

The only part I don't like in that patch is the disgusting line breaking.

Breaking lines in the middle of a comparison is just nasty and wrong.
That code should have been written as

        if (rlimit(RLIMIT_DATA) != 0)
                return false;
        return mm->data_vm + npages <= rlimit_max(RLIMIT_DATA) >> PAGE_SHIFT;

or something like that. Since you removed the pr_warn_once(), you
should remove ignore_rlimit_data too.

Alternatively, if you want to keep ignore_rlimit_data, then you should
have kept the warning too. Making the actual rlimit data check an
inline helper function and having the ignore_rlimit_data check (and
printout) in the caller would make it pretty.

Because breaking lines in the middle of an actual expression is just
completely wrong. It's much worse than having a long line.

(The exception to that "middle of an expression" is breaking lines at
logical expression boundaries: things like adding up several
independent expressions, and having it be

     sum = a +
           b +
           c;

or be something like

     if (a ||
        b ||
        c)
            do_something():

where 'a', 'b' and 'c' are complex but fairly independent expressions).

                  Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: enable RLIMIT_DATA by default with workaround for valgrind

[Konstantin Khlebnikov]

On Sun, Apr 24, 2016 at 9:49 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sun, Apr 24, 2016 at 1:07 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote:
>>
>> This patch checks current usage also against rlim_max if rlim_cur is zero.
>> Size of brk is still checked against rlim_cur, so this part is completely
>> compatible - zero rlim_cur forbids brk() but allows private mmap().
>
> The logic looks reasonable to me. My first reaction was that "but then
> any process can set the limit to zero, and actually increase limits",
> but witht he hard limit always being checked that's ok - the process
> could have just set the soft limit to the hard limit instead.
>
> The only part I don't like in that patch is the disgusting line breaking.
>
> Breaking lines in the middle of a comparison is just nasty and wrong.
> That code should have been written as
>
>         if (rlimit(RLIMIT_DATA) != 0)
>                 return false;
>         return mm->data_vm + npages <= rlimit_max(RLIMIT_DATA) >> PAGE_SHIFT;
>
> or something like that. Since you removed the pr_warn_once(), you
> should remove ignore_rlimit_data too.
>
> Alternatively, if you want to keep ignore_rlimit_data, then you should
> have kept the warning too. Making the actual rlimit data check an
> inline helper function and having the ignore_rlimit_data check (and
> printout) in the caller would make it pretty.

Ok, I'll keep boot option and warning. This should make transition less
painful. That data segment limit was almost useless for a long time and
now it actually starts working, I'm sure there are a lot of strange
configurations around it.

And I want to keep stuff in one function - this simplifies revert =)

>
> Because breaking lines in the middle of an actual expression is just
> completely wrong. It's much worse than having a long line.
>
> (The exception to that "middle of an expression" is breaking lines at
> logical expression boundaries: things like adding up several
> independent expressions, and having it be
>
>      sum = a +
>            b +
>            c;
>
> or be something like
>
>      if (a ||
>         b ||
>         c)
>             do_something():
>
> where 'a', 'b' and 'c' are complex but fairly independent expressions).
>
>                   Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>