lkml.org 
[lkml]   [2016]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/7] netfilter: fix IS_ERR_VALUE usage
    On Mon, Feb 15, 2016 at 03:35:19PM +0100, Andrzej Hajda wrote:
    > IS_ERR_VALUE should be used only with unsigned long type.
    > Otherwise it can work incorrectly. To achieve this function
    > xt_percpu_counter_alloc is modified to return unsigned long,
    > and its result is assigned to temporary variable to perform
    > error checking, before assigning to .pcnt field.

    Wrong fix, IMO. Just have an anon union of u64 pcnt and
    struct xt_counters __percpu *pcpu in there. And make this

    > +static inline unsigned long xt_percpu_counter_alloc(void)
    > {
    > if (nr_cpu_ids > 1) {
    > void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
    > sizeof(struct xt_counters));
    >
    > if (res == NULL)
    > - return (u64) -ENOMEM;
    > + return -ENOMEM;
    >
    > - return (u64) (__force unsigned long) res;
    > + return (__force unsigned long) res;
    > }
    >
    > return 0;

    take struct xt_counters * and return 0 or -ENOMEM. Storing the result of
    allocation in ->pcpu of passed structure.

    I mean, look at the callers -

    > - e->counters.pcnt = xt_percpu_counter_alloc();
    > - if (IS_ERR_VALUE(e->counters.pcnt))
    > + pcnt = xt_percpu_counter_alloc();
    > + if (IS_ERR_VALUE(pcnt))
    > return -ENOMEM;
    > + e->counters.pcnt = pcnt;

    should be
    if (xt_percpu_counter_alloc(&e->counters) < 0)
    return -ENOMEM;

    and similar for the rest of callers. Moreover, if you look at the _users_
    of that fields, you'll see that a bunch of those actually want to use
    ->pcpu instead of doing all those casts.

    Really, that's the point - IS_ERR_VALUE is a big red flag saying "we need
    to figure out what's going on in that place", which does include reading
    through the code. Mechanical "solutions" like that only hide the problem.

    \
     
     \ /
      Last update: 2016-02-17 04:01    [W:5.561 / U:0.368 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site