Re: [RFC 05/18] limits: track and present RLIMIT_NOFILE actual max

From: Andy Lutomirski <luto@amacapital.net>
To: Topi Miettinen <toiwoton@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Kees Cook <keescook@chromium.org>,
	Cyrill Gorcunov <gorcunov@openvz.org>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	John Stultz <john.stultz@linaro.org>,
	Janis Danisevskis <jdanis@google.com>,
	Calvin Owens <calvinowens@fb.com>, Jann Horn <jann@thejh.net>,
	"open list:FILESYSTEMS (VFS and infrastructure)" 
	<linux-fsdevel@vger.kernel.org>
Subject: Re: [RFC 05/18] limits: track and present RLIMIT_NOFILE actual max
Date: Mon, 13 Jun 2016 14:16:31 -0700	[thread overview]
Message-ID: <CALCETrXOR0XzwPsuCwTx5w2VLQP4=z5rAJCaMCSfpzqUFgFEcA@mail.gmail.com> (raw)
In-Reply-To: <ac66b69d-8bb6-b937-2bf0-f5f1cb22fe25@gmail.com>

On Mon, Jun 13, 2016 at 2:13 PM, Topi Miettinen <toiwoton@gmail.com> wrote:
> On 06/13/16 20:40, Andy Lutomirski wrote:
>> On 06/13/2016 12:44 PM, Topi Miettinen wrote:
>>> Track maximum number of files for the process, present current maximum
>>> in /proc/self/limits.
>>
>> The core part should be its own patch.
>>
>> Also, you have this weirdly named (and racy!) function bump_rlimit.
>
> I can change the name if you have better suggestions. rlimit_track_max?
>
> The max value is written often but read seldom, if ever. What kind of
> locking should I use then?

Possibly none, but WRITE_ONCE would be good as would a comment
indicating that your code in intentionally racy.  Or you could use
atomic_cmpxchg if that won't kill performance.

rlimit_track_max sounds like a better name to me.

>
>> Wouldn't this be nicer if you taught the rlimit code to track the
>> *current* usage generically and to derive the max usage from that?
>
> Current rlimit code performs checks against current limits. These are
> typically done early in the calling function and further checks could
> also fail. Thus max should not be updated until much later. Maybe these
> could be combined, but not easily if at all.

I mean:  why not actually show the current value in /proc/pid/limits
and track the max via whatever teaches proc about the current value?

>
>>
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index a11eb71..227997b 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -630,8 +630,8 @@ static int proc_pid_limits(struct seq_file *m,
>>> struct pid_namespace *ns,
>>>      /*
>>>       * print the file header
>>>       */
>>> -       seq_printf(m, "%-25s %-20s %-20s %-10s\n",
>>> -          "Limit", "Soft Limit", "Hard Limit", "Units");
>>> +    seq_printf(m, "%-25s %-20s %-20s %-10s %-20s\n",
>>> +           "Limit", "Soft Limit", "Hard Limit", "Units", "Max");
>>
>> What existing programs, if any, does this break?
>
> Using Debian codesearch for /limits" string, I'd check pam_limits and
> rtkit. The max values could be put into a new file if you prefer.

If it actually breaks them, then you need to change the patch so you
don't break them.