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    [from the cache]
©2003-2011 Jasper Spaans