Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1543389

Re: [PATCH 1/4] mm: add new mmgrab() helper

Show all headers | View raw

On 12/16/2016 10:56 AM, Peter Zijlstra wrote:
> On Fri, Dec 16, 2016 at 09:21:59AM +0100, Vegard Nossum wrote:
>> Apart from adding the helper function itself, the rest of the kernel is
>> converted mechanically using:
>>
>>   git grep -l 'atomic_inc.*mm_count' | xargs sed -i 's/atomic_inc(&\(.*\)->mm_count);/mmgrab\(\1\);/'
>>   git grep -l 'atomic_inc.*mm_count' | xargs sed -i 's/atomic_inc(&\(.*\)\.mm_count);/mmgrab\(\&\1\);/'
>>
>> This is needed for a later patch that hooks into the helper, but might be
>> a worthwhile cleanup on its own.
>
> Given the desire to replace all refcounting with a specific refcount
> type, this seems to make sense.
>
> FYI: http://www.openwall.com/lists/kernel-hardening/2016/12/07/8

If we're going that way eventually (replacing all reference counting
things with a generic interface), I wonder if we shouldn't consider a
generic mechanism for reference counting debugging too.

We could wrap all the 'type *' + 'type_ref' occurrences in a struct, so
that with debugging it boils down to just a pointer (like we have now):

struct ref {
     void *ptr;
#ifdef CONFIG_REF_DEBUG
     /* list_entry, pid, stacktrace, etc. */
#endif
};

Instead of calling refcount_inc() in most of the kernel code, that would
be considered a low-level detail and you'd have the main interface be
something like:

void ref_acquire(refcount_t *count, struct ref *old, struct ref *new)
{
     refcount_inc(&count);
     new->ptr = old->ptr;
#ifdef CONFIG_REF_DEBUG
     /* extra code for debugging case */
#endif
}

So if you had old code that did (for example):

struct task_struct {
     struct mm_struct *mm;
     ...
};

int proc_pid_cmdline_read(struct task_struct *task)
{
     struct mm_struct *mm;

     task_lock(task);
     mm = task->mm;
     atomic_inc(&mm->mm_users);
     task_unlock(task);

     ...

     mmput(mm);
}

you'd instead have:

struct task_struct {
     struct ref mm;
};

int proc_pid_cmdline_read(struct task_struct *task)
{
     REF(mm);

     task_lock(task);
     ref_acquire(&mm->mm_users, &task->mm, &mm)
     task_unlock(task);

     ...

     ref_release(&mm->mm_users, &mm);
}

Of course you'd define a 'struct ref' per type using a macro or
something to keep it type safe (maybe even wrap the counter itself in
there, e.g. mm_users in the example above, so you wouldn't have to pass
it explicitly).

Functions that don't touch reference counts (because the caller holds
one) can just take a plain pointer as usual.

In the example above, you could also have ref_release() set mm->ptr =
NULL; as the pointer should not be considered usable after it has been
released anyway for added safety/debugability.

Best of both worlds?


Vegard

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH 1/4] mm: add new mmgrab() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 10:00 +0100
  Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting  debugging Michal Hocko <mhocko@kernel.org> - 2016-12-16 10:10 +0100
    Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting  debugging Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 10:50 +0100
      crash during oom reaper (was: Re: [PATCH 4/4] [RFC!] mm: 'struct  mm_struct' reference counting debugging) Michal Hocko <mhocko@kernel.org> - 2016-12-16 11:20 +0100
        Re: crash during oom reaper (was: Re: [PATCH 4/4] [RFC!] mm: 'struct  mm_struct' reference counting debugging) "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-12-16 11:50 +0100
          Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 13:00 +0100
            Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 13:20 +0100
            Re: crash during oom reaper "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-12-16 13:50 +0100
              Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 14:00 +0100
                Re: crash during oom reaper "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-12-16 14:10 +0100
                Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 14:20 +0100
                Re: crash during oom reaper Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-12-18 14:50 +0100
                Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-18 17:10 +0100
        Re: crash during oom reaper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 14:20 +0100
          Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 15:10 +0100
            Re: crash during oom reaper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 15:30 +0100
              Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 15:50 +0100
                Re: crash during oom reaper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 16:00 +0100
          Re: crash during oom reaper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 15:10 +0100
  Re: [PATCH 3/4] mm: use mmget_not_zero() helper Michal Hocko <mhocko@kernel.org> - 2016-12-16 10:30 +0100
  Re: [PATCH 2/4] mm: add new mmget() helper Michal Hocko <mhocko@kernel.org> - 2016-12-16 10:30 +0100
  [PATCH 3/4] mm: use mmget_not_zero() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 10:40 +0100
  [PATCH 2/4] mm: add new mmget() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 10:50 +0100
  Re: [PATCH 1/4] mm: add new mmgrab() helper Michal Hocko <mhocko@kernel.org> - 2016-12-16 10:50 +0100
  Re: [PATCH 1/4] mm: add new mmgrab() helper Peter Zijlstra <peterz@infradead.org> - 2016-12-16 11:00 +0100
    Re: [PATCH 1/4] mm: add new mmgrab() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-12-16 11:20 +0100
      Re: [PATCH 1/4] mm: add new mmgrab() helper Michal Hocko <mhocko@kernel.org> - 2016-12-16 11:50 +0100
      Re: [PATCH 1/4] mm: add new mmgrab() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 12:00 +0100
    Re: [PATCH 1/4] mm: add new mmgrab() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 12:20 +0100

csiph-web