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

[Topi Miettinen]

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?

> 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.

> 
>> 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.

> 
>>
>>      for (i = 0; i < RLIM_NLIMITS; i++) {
>>          if (rlim[i].rlim_cur == RLIM_INFINITY)
>> @@ -647,9 +647,11 @@ static int proc_pid_limits(struct seq_file *m,
>> struct pid_namespace *ns,
>>              seq_printf(m, "%-20lu ", rlim[i].rlim_max);
>>
>>          if (lnames[i].unit)
>> -            seq_printf(m, "%-10s\n", lnames[i].unit);
>> +            seq_printf(m, "%-10s", lnames[i].unit);
>>          else
>> -            seq_putc(m, '\n');
>> +            seq_printf(m, "%-10s", "");
>> +        seq_printf(m, "%-20lu\n",
>> +               task->signal->rlim_curmax[i]);
>>      }
>>
>>      return 0;
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 9c48a08..0150380 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -782,6 +782,7 @@ struct signal_struct {
>>       * have no need to disable irqs.
>>       */
>>      struct rlimit rlim[RLIM_NLIMITS];
>> +    unsigned long rlim_curmax[RLIM_NLIMITS];
>>
>>  #ifdef CONFIG_BSD_PROCESS_ACCT
>>      struct pacct_struct pacct;    /* per-process accounting
>> information */
>> @@ -3376,6 +3377,12 @@ static inline unsigned long rlimit_max(unsigned
>> int limit)
>>      return task_rlimit_max(current, limit);
>>  }
>>
>> +static inline void bump_rlimit(unsigned int limit, unsigned long r)
>> +{
>> +    if (READ_ONCE(current->signal->rlim_curmax[limit]) < r)
>> +        current->signal->rlim_curmax[limit] = r;
>> +}
>> +
>>  #ifdef CONFIG_CPU_FREQ
>>  struct update_util_data {
>>      void (*func)(struct update_util_data *data,
>>
>