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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFzPY23bL7oRaH8=C=CQ5egcWCEwieD5rhm5xV=Rv7T7RQ@mail.gmail.com>
Date:	Sun, 24 Apr 2016 11:49:20 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Konstantin Khlebnikov <koct9i@...il.com>
Cc:	linux-mm <linux-mm@...ck.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	Christian Borntraeger <borntraeger@...ibm.com>
Subject: Re: [PATCH] mm: enable RLIMIT_DATA by default with workaround for valgrind

On Sun, Apr 24, 2016 at 1:07 AM, Konstantin Khlebnikov <koct9i@...il.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