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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>