[BUG] x86info fails on 2.6.34 ?

[BUG] x86info fails on 2.6.34 ?

[Eric Dumazet]

Hi Dave

I just noticed x86info was not working anymore on latest kernel

x86info v1.24.  Dave Jones 2001-2009
Feedback to <davej@redhat.com>.

type seek: Value too large for defined data type


Offending syscall is an lseek(fd, -1, SEEK_CUR) on /dev/mem file



open("/dev/mem", O_RDONLY)              = 3
...
lseek(3, 1023092, SEEK_SET)             = 1023092
read(3, "PCMP\4\3\4\266HP      PROLIANT    \0\0\0\0\0"..., 44) = 44
read(3, "\0"..., 1)                     = 1
lseek(3, -1, SEEK_CUR)                  = -1 EOVERFLOW (Value too large
for defined data type)

It seems -1 is taken as an unsigned quantity ?

Re: [BUG] x86info fails on 2.6.34 ?

[Eric Dumazet]

Le lundi 05 avril 2010 à 22:24 +0200, Eric Dumazet a écrit :
> Hi Dave
> 
> I just noticed x86info was not working anymore on latest kernel
> 
> x86info v1.24.  Dave Jones 2001-2009
> Feedback to <davej@redhat.com>.
> 
> type seek: Value too large for defined data type
> 
> 
> Offending syscall is an lseek(fd, -1, SEEK_CUR) on /dev/mem file
> 
> 
> 
> open("/dev/mem", O_RDONLY)              = 3
> ...
> lseek(3, 1023092, SEEK_SET)             = 1023092
> read(3, "PCMP\4\3\4\266HP      PROLIANT    \0\0\0\0\0"..., 44) = 44
> read(3, "\0"..., 1)                     = 1
> lseek(3, -1, SEEK_CUR)                  = -1 EOVERFLOW (Value too large
> for defined data type)
> 
> It seems -1 is taken as an unsigned quantity ?
> 

Andrew, commit dcefafb6ac90ece8d68a6c203105f3d313e52da4 seems to be the
problem. 

It breaks rewinds (negative offsets to lseek (... SEEK_CUR))


commit dcefafb6ac90ece8d68a6c203105f3d313e52da4
Author: Wu Fengguang <fengguang.wu@intel.com>
Date:   Wed Mar 10 15:21:51 2010 -0800

    /dev/mem: dont allow seek to last page
    
    So as to return a uniform error -EOVERFLOW instead of a random one:
    
    # kmem-seek 0xfffffffffffffff0
    seek /dev/kmem: Device or resource busy
    # kmem-seek 0xfffffffffffffff1
    seek /dev/kmem: Block device required
    
    Suggested by OGAWA Hirofumi.
    
    Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
    Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

[PATCH] /dev/mem: Allow rewinding

[Eric Dumazet]

Le lundi 05 avril 2010 à 23:04 +0200, Eric Dumazet a écrit :
> Andrew, commit dcefafb6ac90ece8d68a6c203105f3d313e52da4 seems to be the
> problem. 
> 
> It breaks rewinds (negative offsets to lseek (... SEEK_CUR))
> 

Here is a patch to make rewind working again on /dev/mem

[PATCH] /dev/mem: Allow rewinding

commit dcefafb6 (/dev/mem: dont allow seek to last page)
inadvertently disabled rewinding on /dev/mem.

This broke x86info for example.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 1f3215a..3973a1d 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -710,11 +710,6 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
 	switch (orig) {
 	case SEEK_CUR:
 		offset += file->f_pos;
-		if ((unsigned long long)offset <
-		    (unsigned long long)file->f_pos) {
-			ret = -EOVERFLOW;
-			break;
-		}
 	case SEEK_SET:
 		/* to avoid userland mistaking f_pos=-9 as -EBADF=-9 */
 		if ((unsigned long long)offset >= ~0xFFFULL) {

Re: [PATCH] /dev/mem: Allow rewinding

[Wu Fengguang]

On Tue, Apr 06, 2010 at 05:19:05AM +0800, Eric Dumazet wrote:
> Le lundi 05 avril 2010 à 23:04 +0200, Eric Dumazet a écrit :
> > Andrew, commit dcefafb6ac90ece8d68a6c203105f3d313e52da4 seems to be the
> > problem. 
> > 
> > It breaks rewinds (negative offsets to lseek (... SEEK_CUR))
> > 
> 
> Here is a patch to make rewind working again on /dev/mem

Looks good to me. Thanks for the fix!

Thanks,
Fengguang

> [PATCH] /dev/mem: Allow rewinding
> 
> commit dcefafb6 (/dev/mem: dont allow seek to last page)
> inadvertently disabled rewinding on /dev/mem.
> 
> This broke x86info for example.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 1f3215a..3973a1d 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -710,11 +710,6 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
>  	switch (orig) {
>  	case SEEK_CUR:
>  		offset += file->f_pos;
> -		if ((unsigned long long)offset <
> -		    (unsigned long long)file->f_pos) {
> -			ret = -EOVERFLOW;
> -			break;
> -		}
>  	case SEEK_SET:
>  		/* to avoid userland mistaking f_pos=-9 as -EBADF=-9 */
>  		if ((unsigned long long)offset >= ~0xFFFULL) {
> 
> 

Re: [PATCH] /dev/mem: Allow rewinding

[Américo Wang]

On Tue, Apr 6, 2010 at 5:19 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 05 avril 2010 à 23:04 +0200, Eric Dumazet a écrit :
>> Andrew, commit dcefafb6ac90ece8d68a6c203105f3d313e52da4 seems to be the
>> problem.
>>
>> It breaks rewinds (negative offsets to lseek (... SEEK_CUR))
>>
>
> Here is a patch to make rewind working again on /dev/mem
>
> [PATCH] /dev/mem: Allow rewinding
>
> commit dcefafb6 (/dev/mem: dont allow seek to last page)
> inadvertently disabled rewinding on /dev/mem.
>
> This broke x86info for example.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 1f3215a..3973a1d 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -710,11 +710,6 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
>        switch (orig) {
>        case SEEK_CUR:
>                offset += file->f_pos;
> -               if ((unsigned long long)offset <
> -                   (unsigned long long)file->f_pos) {
> -                       ret = -EOVERFLOW;
> -                       break;
> -               }

Why completely dropping the overflow check? What you need to do is just
adding the 'offset < 0' case check.

Thanks.

Re: [PATCH] /dev/mem: Allow rewinding

[Wu Fengguang]

On Tue, Apr 06, 2010 at 01:30:29PM +0800, Américo Wang wrote:
> On Tue, Apr 6, 2010 at 5:19 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Le lundi 05 avril 2010 à 23:04 +0200, Eric Dumazet a écrit :
> >> Andrew, commit dcefafb6ac90ece8d68a6c203105f3d313e52da4 seems to be the
> >> problem.
> >>
> >> It breaks rewinds (negative offsets to lseek (... SEEK_CUR))
> >>
> >
> > Here is a patch to make rewind working again on /dev/mem
> >
> > [PATCH] /dev/mem: Allow rewinding
> >
> > commit dcefafb6 (/dev/mem: dont allow seek to last page)
> > inadvertently disabled rewinding on /dev/mem.
> >
> > This broke x86info for example.
> >
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> > diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> > index 1f3215a..3973a1d 100644
> > --- a/drivers/char/mem.c
> > +++ b/drivers/char/mem.c
> > @@ -710,11 +710,6 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
> >        switch (orig) {
> >        case SEEK_CUR:
> >                offset += file->f_pos;
> > -               if ((unsigned long long)offset <
> > -                   (unsigned long long)file->f_pos) {
> > -                       ret = -EOVERFLOW;
> > -                       break;
> > -               }
> 
> Why completely dropping the overflow check? What you need to do is just
> adding the 'offset < 0' case check.

It will fall through to the next EOVERFLOW check in the SEEK_SET case :)

Thanks,
Fengguang

Re: [PATCH] /dev/mem: Allow rewinding

[Américo Wang]

On Tue, Apr 6, 2010 at 1:43 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> On Tue, Apr 06, 2010 at 01:30:29PM +0800, Américo Wang wrote:
>> On Tue, Apr 6, 2010 at 5:19 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > Le lundi 05 avril 2010 à 23:04 +0200, Eric Dumazet a écrit :
>> >> Andrew, commit dcefafb6ac90ece8d68a6c203105f3d313e52da4 seems to be the
>> >> problem.
>> >>
>> >> It breaks rewinds (negative offsets to lseek (... SEEK_CUR))
>> >>
>> >
>> > Here is a patch to make rewind working again on /dev/mem
>> >
>> > [PATCH] /dev/mem: Allow rewinding
>> >
>> > commit dcefafb6 (/dev/mem: dont allow seek to last page)
>> > inadvertently disabled rewinding on /dev/mem.
>> >
>> > This broke x86info for example.
>> >
>> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> > ---
>> > diff --git a/drivers/char/mem.c b/drivers/char/mem.c
>> > index 1f3215a..3973a1d 100644
>> > --- a/drivers/char/mem.c
>> > +++ b/drivers/char/mem.c
>> > @@ -710,11 +710,6 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
>> >        switch (orig) {
>> >        case SEEK_CUR:
>> >                offset += file->f_pos;
>> > -               if ((unsigned long long)offset <
>> > -                   (unsigned long long)file->f_pos) {
>> > -                       ret = -EOVERFLOW;
>> > -                       break;
>> > -               }
>>
>> Why completely dropping the overflow check? What you need to do is just
>> adding the 'offset < 0' case check.
>
> It will fall through to the next EOVERFLOW check in the SEEK_SET case :)
>

Oh, right, I didn't read the code, merely the patch.

Thanks.

Re: [PATCH] /dev/mem: Allow rewinding

[Eric Dumazet]

Le mardi 06 avril 2010 à 13:30 +0800, Américo Wang a écrit :
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > ---
> > diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> > index 1f3215a..3973a1d 100644
> > --- a/drivers/char/mem.c
> > +++ b/drivers/char/mem.c
> > @@ -710,11 +710,6 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
> >        switch (orig) {
> >        case SEEK_CUR:
> >                offset += file->f_pos;
> > -               if ((unsigned long long)offset <
> > -                   (unsigned long long)file->f_pos) {
> > -                       ret = -EOVERFLOW;
> > -                       break;
> > -               }
> 
> Why completely dropping the overflow check? What you need to do is just
> adding the 'offset < 0' case check.
> 

Because the overflow check at this point is completely redundant, it is
performed a few lines after...