lkml.org 
[lkml]   [2008]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put_user.
    From
    Date

    On Thu, 2008-06-05 at 12:01 -0400, Thomas Tuttle wrote:
    > On Thu, Jun 5, 2008 at 11:57 AM, Matt Mackall <mpm@selenic.com> wrote:
    > >
    > > On Thu, 2008-06-05 at 11:04 -0400, Thomas Tuttle wrote:
    > >> While fiddling with pagemap, I discovered a bug in add_to_pagemap.
    > >> When it is copying an entry that is not at the end of the buffer, it
    > >> uses put_user to copy a u64 into a char* buffer. The problem is that
    > >> put_user determines how much to copy based on the size of the
    > >> *destination*, not the source, so it only copied one byte. To fix
    > >> this, I replaced the call to put_user with a call to copy_to_user, as
    > >> is used when copying the last (possibly partial) PFN into the buffer.0
    > >
    > > This looks fine to me, so:
    > >
    > > Acked-by: Matt Mackall <mpm@selenic.com>
    > >
    > > But your 3/5 undoes this, right? So we should just take one or the other
    > > route. While I like going the simplifying route, it's not very pretty
    > > from the user interface point of view. But it does have plenty of
    > > precedent in direct-I/O-like things.
    >
    > No, my 3/5 further keeps the use of copy_to_user instead of put_user,
    > but eliminates entirely the code for partial reads. 3/5 depends on
    > 1/5.

    Hmm, I read this backwards the first time through then. Rather than
    finding a small bug at the end of the buffer, you've found a REALLY BIG
    WEAR-A-BROWN-PAPER-BAG-ON-MY-HEAD BUG.

    Sigh. Some clever person (me?) probably removed a cast or changed a
    pointer type somewhere along the way and didn't notice the fallout
    because the MSBs still came through.

    I'm pretty sure we want to do the put_user because it's faster. On a
    64-bit system, it boils down to a simple assignment. If we commit
    ourselves to the alignment thing, then we should probably just change
    all the relevant pointers to u64, and then it will just do the right
    thing.

    ps: perhaps put_user can be made to compare the src and dst sizes at
    compile time?

    --
    Mathematics is the supreme nostalgia of our time.

    --
    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: 2008-06-05 18:23    [W:3.223 / U:0.508 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site