2008-07-19 04:26:45

by Yoshinori Sato

[permalink] [raw]
Subject: [PATCH] filldir write data missing size

"loff_t" is long long.
But "d_off" is unsigned long.

Signed-off-by: Yoshinori Sato <[email protected]>

diff --git a/fs/readdir.c b/fs/readdir.c
index 4e026e5..01e7152
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -159,7 +159,7 @@ static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
return -EOVERFLOW;
dirent = buf->previous;
if (dirent) {
- if (__put_user(offset, &dirent->d_off))
+ if (__put_user((unsigned long)offset, &dirent->d_off))
goto efault;
}
dirent = buf->current_dir;

--
Yoshinori Sato
<[email protected]>


2008-07-19 17:24:42

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] filldir write data missing size

Yoshinori Sato <[email protected]> writes:

> "loff_t" is long long.
> But "d_off" is unsigned long.
>
> Signed-off-by: Yoshinori Sato <[email protected]>
>
> diff --git a/fs/readdir.c b/fs/readdir.c
> index 4e026e5..01e7152
> --- a/fs/readdir.c
> +++ b/fs/readdir.c
> @@ -159,7 +159,7 @@ static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
> return -EOVERFLOW;
> dirent = buf->previous;
> if (dirent) {
> - if (__put_user(offset, &dirent->d_off))
> + if (__put_user((unsigned long)offset, &dirent->d_off))

Um.. __put_user() should be already doing it automatically..

> goto efault;
> }
> dirent = buf->current_dir;
--
OGAWA Hirofumi <[email protected]>

2008-07-19 18:28:18

by Yoshinori Sato

[permalink] [raw]
Subject: Re: [PATCH] filldir write data missing size

At Sun, 20 Jul 2008 02:24:28 +0900,
OGAWA Hirofumi wrote:
>
> Yoshinori Sato <[email protected]> writes:
>
> > "loff_t" is long long.
> > But "d_off" is unsigned long.
> >
> > Signed-off-by: Yoshinori Sato <[email protected]>
> >
> > diff --git a/fs/readdir.c b/fs/readdir.c
> > index 4e026e5..01e7152
> > --- a/fs/readdir.c
> > +++ b/fs/readdir.c
> > @@ -159,7 +159,7 @@ static int filldir(void * __buf, const char * name, int namlen, loff_t offset,
> > return -EOVERFLOW;
> > dirent = buf->previous;
> > if (dirent) {
> > - if (__put_user(offset, &dirent->d_off))
> > + if (__put_user((unsigned long)offset, &dirent->d_off))
>
> Um.. __put_user() should be already doing it automatically..

I'm mistake.

I checked object, and found bad code.
This problem fix __put_user.
Thanks.

> > goto efault;
> > }
> > dirent = buf->current_dir;
> --
> OGAWA Hirofumi <[email protected]>

--
Yoshinori Sato
<[email protected]>

2008-07-19 20:11:41

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] filldir write data missing size

Yoshinori Sato <[email protected]> writes:

>> > - if (__put_user(offset, &dirent->d_off))
>> > + if (__put_user((unsigned long)offset, &dirent->d_off))
>>
>> Um.. __put_user() should be already doing it automatically..
>
> I'm mistake.
>
> I checked object, and found bad code.
> This problem fix __put_user.

Um.. Could you explain the detail of problem? Is this a workaround or
something for the bug of some compiler? If so, shouldn't we fix
__put_user() instead of caller?

E.g. the following or something fixes it? (btw, this is for x86)

#define __put_user(x, ptr) ({ \
/* Since some compiler generates wrong code, we need __v.?? */ \
__typeof__(*(ptr)) __v = (x); \
__put_user_nocheck(__v, (ptr), sizeof(*(ptr))); \
})
--
OGAWA Hirofumi <[email protected]>

2008-07-21 15:48:55

by Yoshinori Sato

[permalink] [raw]
Subject: Re: [PATCH] filldir write data missing size

At Sun, 20 Jul 2008 05:11:29 +0900,
OGAWA Hirofumi wrote:
>
> Yoshinori Sato <[email protected]> writes:
>
> >> > - if (__put_user(offset, &dirent->d_off))
> >> > + if (__put_user((unsigned long)offset, &dirent->d_off))
> >>
> >> Um.. __put_user() should be already doing it automatically..
> >
> > I'm mistake.
> >
> > I checked object, and found bad code.
> > This problem fix __put_user.
>
> Um.. Could you explain the detail of problem? Is this a workaround or
> something for the bug of some compiler? If so, shouldn't we fix
> __put_user() instead of caller?
>
> E.g. the following or something fixes it? (btw, this is for x86)
>
> #define __put_user(x, ptr) ({ \
> /* Since some compiler generates wrong code, we need __v.?? */ \
> __typeof__(*(ptr)) __v = (x); \
> __put_user_nocheck(__v, (ptr), sizeof(*(ptr))); \
> })
> --
> OGAWA Hirofumi <[email protected]>

This problem is sh-linux-gcc v4.1.2 and target sh2(a)-bigendian.

"__put_user(s64, u32_ptr)" compiled.

Correct code.
*u32_ptr = s64 & 0xffffffff;

Bad code.
*u32_ptr = s64 >> 32;

I'm add cast put_user 4byte case.

--
Yoshinori Sato
<[email protected]>

2008-07-21 21:31:25

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] filldir write data missing size

Yoshinori Sato <[email protected]> writes:

> This problem is sh-linux-gcc v4.1.2 and target sh2(a)-bigendian.
>
> "__put_user(s64, u32_ptr)" compiled.
>
> Correct code.
> *u32_ptr = s64 & 0xffffffff;
>
> Bad code.
> *u32_ptr = s64 >> 32;
>
> I'm add cast put_user 4byte case.

I see. How about the following patch? I guess the problems of this type
should be fixed.

diff -puN include/asm-sh/uaccess_32.h~sh-fix include/asm-sh/uaccess_32.h
--- linux-2.6/include/asm-sh/uaccess_32.h~sh-fix 2008-07-22 06:20:15.000000000 +0900
+++ linux-2.6-hirofumi/include/asm-sh/uaccess_32.h 2008-07-22 06:25:26.000000000 +0900
@@ -211,8 +211,9 @@ do { \
({ \
long __pu_err; \
__typeof__(*(ptr)) __user *__pu_addr = (ptr); \
+ __typeof__(*(ptr)) __pu_val = x; \
__chk_user_ptr(ptr); \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
+ __put_user_size(__pu_val, __pu_addr, (size), __pu_err); \
__pu_err; \
})

@@ -220,8 +221,9 @@ do { \
({ \
long __pu_err = -EFAULT; \
__typeof__(*(ptr)) __user *__pu_addr = (ptr); \
+ __typeof__(*(ptr)) __pu_val = x; \
if (likely(access_ok(VERIFY_WRITE, __pu_addr, size))) \
- __put_user_size((x), __pu_addr, (size), \
+ __put_user_size(__pu_val, __pu_addr, (size), \
__pu_err); \
__pu_err; \
})
_
--
OGAWA Hirofumi <[email protected]>

2008-07-28 18:58:40

by Yoshinori Sato

[permalink] [raw]
Subject: Re: [PATCH] filldir write data missing size

At Tue, 22 Jul 2008 06:31:12 +0900,
OGAWA Hirofumi wrote:
>
> Yoshinori Sato <[email protected]> writes:
>
> > This problem is sh-linux-gcc v4.1.2 and target sh2(a)-bigendian.
> >
> > "__put_user(s64, u32_ptr)" compiled.
> >
> > Correct code.
> > *u32_ptr = s64 & 0xffffffff;
> >
> > Bad code.
> > *u32_ptr = s64 >> 32;
> >
> > I'm add cast put_user 4byte case.
>
> I see. How about the following patch? I guess the problems of this type
> should be fixed.
>
> diff -puN include/asm-sh/uaccess_32.h~sh-fix include/asm-sh/uaccess_32.h
> --- linux-2.6/include/asm-sh/uaccess_32.h~sh-fix 2008-07-22 06:20:15.000000000 +0900
> +++ linux-2.6-hirofumi/include/asm-sh/uaccess_32.h 2008-07-22 06:25:26.000000000 +0900
> @@ -211,8 +211,9 @@ do { \
> ({ \
> long __pu_err; \
> __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
> + __typeof__(*(ptr)) __pu_val = x; \
> __chk_user_ptr(ptr); \
> - __put_user_size((x), __pu_addr, (size), __pu_err); \
> + __put_user_size(__pu_val, __pu_addr, (size), __pu_err); \
> __pu_err; \
> })
>
> @@ -220,8 +221,9 @@ do { \
> ({ \
> long __pu_err = -EFAULT; \
> __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
> + __typeof__(*(ptr)) __pu_val = x; \
> if (likely(access_ok(VERIFY_WRITE, __pu_addr, size))) \
> - __put_user_size((x), __pu_addr, (size), \
> + __put_user_size(__pu_val, __pu_addr, (size), \
> __pu_err); \
> __pu_err; \
> })
> _
> --
> OGAWA Hirofumi <[email protected]>

Sorry too late reply.

This fix is good.
But I send other workaround fix.

Thanks.

--
Yoshinori Sato
<[email protected]>

2008-07-28 19:08:48

by Paul Mundt

[permalink] [raw]
Subject: Re: [PATCH] filldir write data missing size

On Tue, Jul 22, 2008 at 06:31:12AM +0900, OGAWA Hirofumi wrote:
> Yoshinori Sato <[email protected]> writes:
>
> > This problem is sh-linux-gcc v4.1.2 and target sh2(a)-bigendian.
> >
> > "__put_user(s64, u32_ptr)" compiled.
> >
> > Correct code.
> > *u32_ptr = s64 & 0xffffffff;
> >
> > Bad code.
> > *u32_ptr = s64 >> 32;
> >
> > I'm add cast put_user 4byte case.
>
> I see. How about the following patch? I guess the problems of this type
> should be fixed.
>
I merged a different workaround as no one bothered to CC me on this
thread. Your fix looks more correct though, please provide a
Signed-off-by for it and I'll take this over the previous workaround.

2008-07-28 20:14:52

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] filldir write data missing size

Paul Mundt <[email protected]> writes:

> On Tue, Jul 22, 2008 at 06:31:12AM +0900, OGAWA Hirofumi wrote:
>> Yoshinori Sato <[email protected]> writes:
>>
>> > This problem is sh-linux-gcc v4.1.2 and target sh2(a)-bigendian.
>> >
>> > "__put_user(s64, u32_ptr)" compiled.
>> >
>> > Correct code.
>> > *u32_ptr = s64 & 0xffffffff;
>> >
>> > Bad code.
>> > *u32_ptr = s64 >> 32;
>> >
>> > I'm add cast put_user 4byte case.
>>
>> I see. How about the following patch? I guess the problems of this type
>> should be fixed.
>>
> I merged a different workaround as no one bothered to CC me on this
> thread. Your fix looks more correct though, please provide a
> Signed-off-by for it and I'll take this over the previous workaround.

Of course. But I didn't test it at all, so please test it.

Signed-off-by: OGAWA Hirofumi <[email protected]>

Thanks.
--
OGAWA Hirofumi <[email protected]>