lkml.org 
[lkml]   [2012]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [patch 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v4
    Date
    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


    \
     
     \ /
      Last update: 2012-01-24 23:53    [W:0.028 / U:29.456 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site