lkml.org 
[lkml]   [2006]   [Nov]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Subject[PATCH] Avoid truncating to 'long' in ALIGN() macro
    From
    Date
    Commit 4c8bd7ee ("Do not truncate to 'int' in ALIGN() macro.") was
    merged to fix the case of code like the following:

    unsigned long addr;
    unsigned int alignment;
    addr = ALIGN(addr, alignment);

    The original ALIGN macro calculated a mask as ~(alignment - 1), and
    when alignment is just an int, this creates an int mask. If alignment
    is also unsigned, then this mask will not be sign extended when
    promoted to a long, which leads to the code above chopping off the top
    half of addr when long is 64 bits.

    However, the changed ALIGN macro, which computes the mask as
    ~(alignment - 1UL) actually breaks code like the following when long
    is 32 bits:

    u64 addr;
    int alignment;
    addr = ALIGN(addr, alignment);

    The reason this breaks is pretty much the same as the original bug
    that the change was supposed to fix: ~(alignment - 1UL) creates a mask
    that is an unsigned long, which is unsigned and therefore not sign
    extended when promoted to u64 (if long is 32 bits).

    I think the right fix is to use 1L instead of 1UL, to avoid the
    possibility of turning the mask unsigned by accident. This will still
    fix the original problem, without breaking code that uses ALIGN() the
    second way.

    This second construct is actually used in the amso1100 driver, so that
    driver does not work on 32-bit architectures right now. Unfortunately
    almost everyone using it runs 64-bit kernels, so this regression was
    not noticed until now.

    Signed-off-by: Roland Dreier <rolandd@cisco.com>

    ---
    If it's too late to merge this for 2.6.19, I can work around this in
    the amso1100 driver by doing ALIGN(addr, (u64) alignment) instead.
    Let me know if I should send that patch.

    However I think this should go in for 2.6.20 at least, since I see
    only danger with no benefit from having the constant 1 be unsigned.

    diff --git a/include/linux/kernel.h b/include/linux/kernel.h
    index 24b6111..cc542d3 100644
    --- a/include/linux/kernel.h
    +++ b/include/linux/kernel.h
    @@ -31,7 +31,7 @@ #define ULLONG_MAX (~0ULL)
    #define STACK_MAGIC 0xdeadbeef

    #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
    -#define ALIGN(x,a) (((x)+(a)-1UL)&~((a)-1UL))
    +#define ALIGN(x,a) (((x)+(a)-1L)&~((a)-1L))
    #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
    #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
    #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2006-11-25 06:45    [W:0.028 / U:31.868 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site