lkml.org 
[lkml]   [2004]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH] Missing return value check on do_write_mem
    Date
    Alle 22:46, martedì 9 marzo 2004, Andrew Morton ha scritto:
    > BlaisorBlade <blaisorblade_spam@yahoo.it> wrote:
    > > In drivers/char/mem.c do_write_mem can return -EFAULT but write_kmem
    > > forgets this and goes blindly.

    First: do not forget this first fix.

    > > Also, do_write_mem takes two unused params and is static - so I've
    > > removed those.

    The "file" parameter is anyway unused - so it can be removed; for the ppos
    parameter your reasoning holds. I'm posting the patch for these two things
    against 2.6.4 (now I do not remove realp so I avoid the race you describe).

    Thanks for answering me - you never lose a patch!

    > > I actually double-checked this - however please test
    > > compilation on Sparc/m68k, since there are some #ifdef.
    >
    > It's a small thing, but:
    > > --- ./drivers/char/mem.c.fix 2004-02-20 16:27:21.000000000 +0100
    > > +++ ./drivers/char/mem.c 2004-03-08 12:17:23.000000000 +0100
    > > @@ -96,13 +96,14 @@
    > > }
    > > #endif
    > >
    > > -static ssize_t do_write_mem(struct file * file, void *p, unsigned long
    > > realp, - const char * buf, size_t count, loff_t *ppos)
    > > +static ssize_t do_write_mem(void *p, const char * buf, size_t count,
    > > + loff_t *ppos)
    > > {
    > > - ssize_t written;
    > > + ssize_t written = 0;
    > >
    > > - written = 0;
    > > #if defined(__sparc__) || (defined(__mc68000__) && defined(CONFIG_MMU))
    > > + unsigned long realp = *ppos;
    > > +
    >
    > A thread which shares this fd can alter the value at *ppos at any time via
    > lseek() .

    Well, you are right (not found any locking). I've understood that lseek() is
    known to be racy, and that the bug in my patch is just the range check that
    does not work any more; but anyway the marked line in the 2.6.3/2.6.4 kernel
    is racy, too, and I'm unsure if that is allowed; i.e. man 2 write claims even
    that "POSIX requires that a
    read() which can be proved to occur after a write() has returned
    returns the new data. Note that not all file systems are POSIX con-
    forming." which would imply that write is not racy. Or not?

    static ssize_t do_write_mem(struct file * file, void *p, unsigned long realp,
    const char * buf, size_t count, loff_t *ppos)
    {
    ssize_t written;

    written = 0;
    #if defined(__sparc__) || (defined(__mc68000__) && defined(CONFIG_MMU))
    //[...]
    #endif
    if (copy_from_user(p, buf, count))
    return -EFAULT;
    written += count;
    *ppos += written; /*If a second thread lseek()'d between when we
    read realp and this line, we have a race*/
    return written;
    }



    --
    Paolo Giarrusso, aka Blaisorblade
    Linux registered user n. 292729
    --- ./drivers/char/mem.c.fix 2004-03-13 17:56:09.000000000 +0100
    +++ ./drivers/char/mem.c 2004-03-13 20:36:35.000000000 +0100
    @@ -102,7 +102,7 @@
    }
    #endif

    -static ssize_t do_write_mem(struct file * file, void *p, unsigned long realp,
    +static ssize_t do_write_mem(void *p, unsigned long realp,
    const char * buf, size_t count, loff_t *ppos)
    {
    ssize_t written;
    @@ -171,7 +171,7 @@

    if (!valid_phys_addr_range(p, &count))
    return -EFAULT;
    - return do_write_mem(file, __va(p), p, buf, count, ppos);
    + return do_write_mem(__va(p), p, buf, count, ppos);
    }

    static int mmap_mem(struct file * file, struct vm_area_struct * vma)
    @@ -282,7 +282,9 @@
    if (count > (unsigned long) high_memory - p)
    wrote = (unsigned long) high_memory - p;

    - wrote = do_write_mem(file, (void*)p, p, buf, wrote, ppos);
    + wrote = do_write_mem((void*)p, p, buf, wrote, ppos);
    + if (wrote < 0)
    + return wrote;

    p += wrote;
    buf += wrote;
    \
     
     \ /
      Last update: 2005-03-22 14:01    [W:0.032 / U:154.340 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site