Messages in this thread | | | From | (Eric W. Biederman) | Subject | Re: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4 | Date | Tue, 24 Jan 2012 14:54:18 -0800 |
| |
Andrew Morton <akpm@linux-foundation.org> writes:
> <reads the code> > > Seems that it performs lookups only in the caller's PID namespace. > Maybe this is appropriate but it should be described and justified in > the changelog and in code comments, please. And in the forthcoming > manpage ;)
Well pids should always and only be looked up in the callers pid namespace. Any other behavior is broken. It is probably worth a mention in a manpage but you should not need to justify using abstractions as they were designed to be used.
>> +static int kcmp_ptr(long v1, long v2, int type) >> +{ >> + long ret; >> + >> + ret = kptr_obfuscate(v1, type) - kptr_obfuscate(v2, type); >> + >> + return (ret < 0) | ((ret > 0) << 1); >> +} >> + >> +#define KCMP_TASK_PTR(task1, task2, member, type) \ >> + kcmp_ptr((long)(task1)->member, \ >> + (long)(task2)->member, \ >> + type) >> + >> +#define KCMP_PTR(ptr1, ptr2, type) \ >> + kcmp_ptr((long)ptr1, (long)ptr2, type) > > ugh. This: > > static long kptr_obfuscate(void *p, enum you_forgot_to_name_the_enum type) > { > return ((long)p ^ cookies[type][0]) * cookies[type][1]; > } > > static int kcmp_task_pointers(void *task1, void *task2, size_t field_offset, > enum you_forgot_to_name_the_enum type) > { > void **field1 = t1 + field_offset; /* points to a pointer in the task_struct */ > void **field2 = t1 + field_offset; > long diff; > > diff = kptr_obfuscate(*field1, type) - kptr_obfuscate(*field2, type); > return (diff < 0) | ((diff > 0) << 1); > } > > ... > ret = kcmp_task_pointers(task1, task2, offsetof(task_struct, mm), > KCMP_VM); > ... > > see? No nasty macros, it's type-correct and it uses only a single > explicit typecast.
Seriously? Simply open coding the comparison would be better.
ret = kcmp_ptr(task1->files, task2->files, type);
All pointers are not encoded the same as void * pointers. Admittedly the only case I can think of are function pointers on Itanium, but what is a little wrong today can easily become a lot wrong tomorrow.
Making the kcmp_ptr arguments void * seems the way to go though.
Now there is one interesting case we are not handling properly. If any of our pointers can be NULL which I think happens in the file case we should return -EBADF instead of reporting two NULL pointers point to the same object.
Eric
| |