Messages in this thread | | | Subject | Re: [PATCH 1/4] mm: add new mmgrab() helper | From | Vegard Nossum <> | Date | Fri, 16 Dec 2016 12:14:40 +0100 |
| |
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
| |