filldir write data missing size

PATCH In review Yoshinori Sato 3 participants 9 messages Jul 19, 2008 – Jul 28, 2008 VFS

lore

Share & tools

lore

Yoshinori Sato

author

2008-07-19 04:26 UTC #0 lore raw

PATCH 1/1 filldir write data missing size

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

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

fs/readdir.c +1 -1

index 4e026e5..01e7152
@@ -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;

Signature · 2 lines

Yoshinori Sato
<[email protected]>

OGAWA Hirofumi

2008-07-19 17:24 UTC #1 lore raw

Quoted 10 lines · Yoshinori Sato

"loff_t" is long long.
But "d_off" is unsigned long.
Signed-off-by Yoshinori Sato <[email protected]>
fs/readdir.c +0 -0
index 4e026e5..01e7152

fs/readdir.c static int filldir(void * __buf, const char * name, int namlen, loff_t offset, · line 162

Quoted 5 lines

fs/readdir.c +1 -1

@@ -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..

Quoted 3 lines

fs/readdir.c +0 -0

 			goto efault;
 	}
 	dirent = buf->current_dir;

Signature · 1 line

OGAWA Hirofumi <[email protected]>

Yoshinori Sato

author

2008-07-19 18:28 UTC #2 lore raw

At Sun, 20 Jul 2008 02:24:28 +0900,

Quoted 8 lines · OGAWA Hirofumi

Quoted 5 lines · Yoshinori Sato
"loff_t" is long long.
But "d_off" is unsigned long.
Signed-off-by Yoshinori Sato <[email protected]>

Quoted 12 lines

fs/readdir.c static int filldir(void * __buf, const char * name, int namlen, loff_t offset, · line 162
Quoted 10 lines
fs/readdir.c +1 -1
index 4e026e5..01e7152
@@ -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.

Quoted 5 lines

Quoted 3 lines
fs/readdir.c +0 -0
 			goto efault;
 	}
 	dirent = buf->current_dir;
Signature · 1 line
OGAWA Hirofumi <[email protected]>

Signature · 2 lines

Yoshinori Sato
<[email protected]>

OGAWA Hirofumi

2008-07-19 20:11 UTC #3 lore raw

Quoted 9 lines · Yoshinori Sato

Quoted 4 lines
fs/readdir.c static int filldir(void * __buf, const char * name, int namlen, loff_t offset, · line 162
Quoted 2 lines
fs/readdir.c +1 -1
-		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)));			\
})

Signature · 1 line

OGAWA Hirofumi <[email protected]>

Yoshinori Sato

author

2008-07-21 15:48 UTC #4 lore raw

At Sun, 20 Jul 2008 05:11:29 +0900,

Quoted 17 lines · OGAWA Hirofumi

Quoted 9 lines · Yoshinori Sato
Quoted 4 lines
fs/readdir.c static int filldir(void * __buf, const char * name, int namlen, loff_t offset, · line 162
Quoted 2 lines
fs/readdir.c +1 -1
-		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?

Quoted 9 lines

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)));			\
})
Signature · 1 line
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.

Signature · 2 lines

Yoshinori Sato
<[email protected]>

OGAWA Hirofumi

2008-07-21 21:31 UTC #5 lore raw

Quoted 11 lines · Yoshinori Sato

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.

include/asm-sh/uaccess_32.h renamed from include/asm-sh/uaccess_32.h~sh-fix +4 -2

diff -puN include/asm-sh/uaccess_32.h~sh-fix include/asm-sh/uaccess_32.h
@@ -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;						\
 })

Signature · 1 line

OGAWA Hirofumi <[email protected]>

Yoshinori Sato

author

2008-07-28 18:58 UTC #6 lore raw

At Tue, 22 Jul 2008 06:31:12 +0900,

Quoted 43 lines · OGAWA Hirofumi

Quoted 11 lines · Yoshinori Sato
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
include/asm-sh/uaccess_32.h renamed from include/asm-sh/uaccess_32.h~sh-fix +4 -2
@@ -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;						\
 })

Quoted 3 lines

_
Signature · 1 line
OGAWA Hirofumi <[email protected]>

Sorry too late reply.

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

Thanks.

Signature · 2 lines

Yoshinori Sato
<[email protected]>

Paul Mundt

2008-07-28 19:08 UTC #7 lore raw

In reply to OGAWA Hirofumi

Quoted 9 lines · OGAWA Hirofumi

Quoted 7 lines · Yoshinori Sato
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;

Quoted 8 lines

Quoted 4 lines
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.

OGAWA Hirofumi

2008-07-28 20:14 UTC #8 lore raw

Quoted 13 lines · Paul Mundt

Quoted 12 lines · OGAWA Hirofumi
Quoted 10 lines · Yoshinori Sato
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;

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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.

Signature · 1 line

OGAWA Hirofumi <[email protected]>