* [PATCH] Typecasting required for comparing unlike datatypes
@ 2010-12-08 12:55 Harsh Prateek Bora
2010-12-09 18:32 ` Harsh Bora
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Harsh Prateek Bora @ 2010-12-08 12:55 UTC (permalink / raw)
To: linux-fsdevel, fengguang.wu, kamezawa.hiroyu
Cc: aneesh.kumar, jvrao, Harsh Prateek Bora
The existing code causes the if condition to pass when it should fail
on a *64-bit kernel* because of implicit data type conversions. It can
be observed by passing pos = -1 and count = some positive number.
This results in function returning EOVERFLOW instead of EINVAL.
With this patch, the function returns EINVAL when pos is -1 and count
is a positive number. This can be tested by calling sendfile with
offset = -1 and count = some positive number on a 64-bit kernel.
Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
---
fs/read_write.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 431a0ed..a8eabd4 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
* pos or pos+count is negative here, check overflow.
* too big "count" will be caught in rw_verify_area().
*/
- if ((pos < 0) && (pos + count < pos))
+ if ((pos < 0) && ( (loff_t) (pos + count) < pos))
return -EOVERFLOW;
if (file->f_mode & FMODE_UNSIGNED_OFFSET)
return 0;
--
1.7.1.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-08 12:55 [PATCH] Typecasting required for comparing unlike datatypes Harsh Prateek Bora
@ 2010-12-09 18:32 ` Harsh Bora
2010-12-10 0:06 ` KAMEZAWA Hiroyuki
2010-12-10 0:53 ` KAMEZAWA Hiroyuki
2010-12-10 8:18 ` KAMEZAWA Hiroyuki
2 siblings, 1 reply; 15+ messages in thread
From: Harsh Bora @ 2010-12-09 18:32 UTC (permalink / raw)
To: viro
Cc: Harsh Prateek Bora, linux-fsdevel, fengguang.wu, kamezawa.hiroyu,
aneesh.kumar, jvrao
Hi Al,
Any comments?
On 12/08/2010 06:25 PM, Harsh Prateek Bora wrote:
> The existing code causes the if condition to pass when it should fail
> on a *64-bit kernel* because of implicit data type conversions. It can
> be observed by passing pos = -1 and count = some positive number.
> This results in function returning EOVERFLOW instead of EINVAL.
>
> With this patch, the function returns EINVAL when pos is -1 and count
> is a positive number. This can be tested by calling sendfile with
> offset = -1 and count = some positive number on a 64-bit kernel.
>
> Signed-off-by: Harsh Prateek Bora<harsh@linux.vnet.ibm.com>
> ---
> fs/read_write.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 431a0ed..a8eabd4 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
> * pos or pos+count is negative here, check overflow.
> * too big "count" will be caught in rw_verify_area().
> */
> - if ((pos< 0)&& (pos + count< pos))
> + if ((pos< 0)&& ( (loff_t) (pos + count)< pos))
> return -EOVERFLOW;
> if (file->f_mode& FMODE_UNSIGNED_OFFSET)
> return 0;
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-09 18:32 ` Harsh Bora
@ 2010-12-10 0:06 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 15+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-10 0:06 UTC (permalink / raw)
To: Harsh Bora; +Cc: viro, linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On Fri, 10 Dec 2010 00:02:03 +0530
Harsh Bora <harsh@linux.vnet.ibm.com> wrote:
> Hi Al,
> Any comments?
>
> On 12/08/2010 06:25 PM, Harsh Prateek Bora wrote:
> > The existing code causes the if condition to pass when it should fail
> > on a *64-bit kernel* because of implicit data type conversions. It can
> > be observed by passing pos = -1 and count = some positive number.
> > This results in function returning EOVERFLOW instead of EINVAL.
> >
> > With this patch, the function returns EINVAL when pos is -1 and count
> > is a positive number. This can be tested by calling sendfile with
> > offset = -1 and count = some positive number on a 64-bit kernel.
> >
> > Signed-off-by: Harsh Prateek Bora<harsh@linux.vnet.ibm.com>
> > ---
> > fs/read_write.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index 431a0ed..a8eabd4 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
> > * pos or pos+count is negative here, check overflow.
> > * too big "count" will be caught in rw_verify_area().
> > */
> > - if ((pos< 0)&& (pos + count< pos))
> > + if ((pos< 0)&& ( (loff_t) (pos + count)< pos))
> > return -EOVERFLOW;
Hmm, maybe
if (!(file->f_mode & FMODE_UNSIGNED_OFFSET))
return -EINVAL;
if ((pos < 0) && (pos + count < pos))
return -EOVERFLOW;
return 0;
is a correct order ?
Thanks,
-Kame
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-08 12:55 [PATCH] Typecasting required for comparing unlike datatypes Harsh Prateek Bora
2010-12-09 18:32 ` Harsh Bora
@ 2010-12-10 0:53 ` KAMEZAWA Hiroyuki
2010-12-10 6:39 ` Harsh Bora
2010-12-10 8:18 ` KAMEZAWA Hiroyuki
2 siblings, 1 reply; 15+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-10 0:53 UTC (permalink / raw)
To: Harsh Prateek Bora; +Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On Wed, 8 Dec 2010 18:25:00 +0530
Harsh Prateek Bora <harsh@linux.vnet.ibm.com> wrote:
> The existing code causes the if condition to pass when it should fail
> on a *64-bit kernel* because of implicit data type conversions. It can
> be observed by passing pos = -1 and count = some positive number.
> This results in function returning EOVERFLOW instead of EINVAL.
>
> With this patch, the function returns EINVAL when pos is -1 and count
> is a positive number. This can be tested by calling sendfile with
> offset = -1 and count = some positive number on a 64-bit kernel.
>
Hmm, is this clearer ?
==
commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
negative (unsigned) page offset for very large files as /proc/<pid>/mem
and /dev/mem.
In that patch, overlap check routine is added but it was wrong.
Considering 'pos' is loff_t, a signed value,
In usual case, at comparing 'pos' and 'pos+count'
(positive) / (positive) OK
(positive) / (nevative) EOVERFLOW
(negative) / (positive) EINVAL
(negative) / (negative) EINVAL
In FMODE_UNSIGNED_OFFSET case,
(positive) / (positive) OK
(positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
(nevative) / (negative) OK
(negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
fs/read_write.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
Index: linux-2.6.37-rc5/fs/read_write.c
===================================================================
--- linux-2.6.37-rc5.orig/fs/read_write.c
+++ linux-2.6.37-rc5/fs/read_write.c
@@ -37,11 +37,24 @@ __negative_fpos_check(struct file *file,
* pos or pos+count is negative here, check overflow.
* too big "count" will be caught in rw_verify_area().
*/
- if ((pos < 0) && (pos + count < pos))
+ /* negative pos is allowed only when the flag is set */
+ if (!(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
+ if ((pos > 0) && (pos + count > 0))
+ return 0;
+ if ((pos > 0) && (pos + count < 0))
+ return -EOVERFLOW;
+ return -EINVAL;
+ }
+ /*
+ * The file supports 'unsigned long' offset. (but loff_t is signed)
+ * When pos is negative, -1 is the biggest number. So if pos + count
+ * is larger than pos, it's overflow.
+ * (ex) -1 + 10 = 9 ...means
+ * 0xffff + 0xa = 0x9 => overflow.
+ */
+ if ((pos < 0) && (pos + count > 0))
return -EOVERFLOW;
- if (file->f_mode & FMODE_UNSIGNED_OFFSET)
- return 0;
- return -EINVAL;
+ return 0;
}
/**
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-10 0:53 ` KAMEZAWA Hiroyuki
@ 2010-12-10 6:39 ` Harsh Bora
2010-12-10 7:01 ` KAMEZAWA Hiroyuki
0 siblings, 1 reply; 15+ messages in thread
From: Harsh Bora @ 2010-12-10 6:39 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On 12/10/2010 06:23 AM, KAMEZAWA Hiroyuki wrote:
> On Wed, 8 Dec 2010 18:25:00 +0530
> Harsh Prateek Bora<harsh@linux.vnet.ibm.com> wrote:
>
>> The existing code causes the if condition to pass when it should fail
>> on a *64-bit kernel* because of implicit data type conversions. It can
>> be observed by passing pos = -1 and count = some positive number.
>> This results in function returning EOVERFLOW instead of EINVAL.
>>
>> With this patch, the function returns EINVAL when pos is -1 and count
>> is a positive number. This can be tested by calling sendfile with
>> offset = -1 and count = some positive number on a 64-bit kernel.
>>
>
> Hmm, is this clearer ?
>
> ==
>
> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> negative (unsigned) page offset for very large files as /proc/<pid>/mem
> and /dev/mem.
>
> In that patch, overlap check routine is added but it was wrong.
>
> Considering 'pos' is loff_t, a signed value,
>
> In usual case, at comparing 'pos' and 'pos+count'
>
> (positive) / (positive) OK
> (positive) / (nevative) EOVERFLOW
> (negative) / (positive) EINVAL
> (negative) / (negative) EINVAL
>
> In FMODE_UNSIGNED_OFFSET case,
>
> (positive) / (positive) OK
> (positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
> (nevative) / (negative) OK
> (negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
>
> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> ---
> fs/read_write.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.37-rc5/fs/read_write.c
> ===================================================================
> --- linux-2.6.37-rc5.orig/fs/read_write.c
> +++ linux-2.6.37-rc5/fs/read_write.c
> @@ -37,11 +37,24 @@ __negative_fpos_check(struct file *file,
> * pos or pos+count is negative here, check overflow.
> * too big "count" will be caught in rw_verify_area().
> */
> - if ((pos< 0)&& (pos + count< pos))
> + /* negative pos is allowed only when the flag is set */
> + if (!(file->f_mode& FMODE_UNSIGNED_OFFSET)) {
> + if ((pos> 0)&& (pos + count> 0))
> + return 0;
> + if ((pos> 0)&& (pos + count< 0))
> + return -EOVERFLOW;
> + return -EINVAL;
> + }
> + /*
> + * The file supports 'unsigned long' offset. (but loff_t is signed)
> + * When pos is negative, -1 is the biggest number. So if pos + count
> + * is larger than pos, it's overflow.
> + * (ex) -1 + 10 = 9 ...means
> + * 0xffff + 0xa = 0x9 => overflow.
> + */
> + if ((pos< 0)&& (pos + count> 0))
Well, that works fine for what I am concerned but I think there is a
mismatch in the code and the comment above. As per the comments above,
it should be like:
if ((pos < 0) && (pos + count > pos))
Regards,
Harsh.
> return -EOVERFLOW;
> - if (file->f_mode& FMODE_UNSIGNED_OFFSET)
> - return 0;
> - return -EINVAL;
> + return 0;
> }
>
> /**
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-10 6:39 ` Harsh Bora
@ 2010-12-10 7:01 ` KAMEZAWA Hiroyuki
2010-12-10 7:18 ` Harsh Bora
0 siblings, 1 reply; 15+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-10 7:01 UTC (permalink / raw)
To: Harsh Bora; +Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On Fri, 10 Dec 2010 12:09:42 +0530
Harsh Bora <harsh@linux.vnet.ibm.com> wrote:
return -EINVAL;
> > + }
> > + /*
> > + * The file supports 'unsigned long' offset. (but loff_t is signed)
> > + * When pos is negative, -1 is the biggest number. So if pos + count
> > + * is larger than pos, it's overflow.
> > + * (ex) -1 + 10 = 9 ...means
> > + * 0xffff + 0xa = 0x9 => overflow.
> > + */
> > + if ((pos< 0)&& (pos + count> 0))
>
> Well, that works fine for what I am concerned but I think there is a
> mismatch in the code and the comment above. As per the comments above,
> it should be like:
> if ((pos < 0) && (pos + count > pos))
>
Ah, yes. updated. Thank you for review and test.
-Kame
==
commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
negative (unsigned) page offset for very large files as /proc/<pid>/mem
and /dev/mem.
In that patch, overlap check routine is added but it was wrong.
Considering 'pos' is loff_t, a signed value,
In usual case, at comparing 'pos' and 'pos+count'
(positive) / (positive) OK
(positive) / (nevative) EOVERFLOW
(negative) / (positive) EINVAL
(negative) / (negative) EINVAL
In FMODE_UNSIGNED_OFFSET case,
(positive) / (positive) OK
(positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
(nevative) / (negative) OK
(negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
Changelog:
- fixed a comment.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
fs/read_write.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
Index: linux-2.6.37-rc5/fs/read_write.c
===================================================================
--- linux-2.6.37-rc5.orig/fs/read_write.c
+++ linux-2.6.37-rc5/fs/read_write.c
@@ -37,11 +37,24 @@ __negative_fpos_check(struct file *file,
* pos or pos+count is negative here, check overflow.
* too big "count" will be caught in rw_verify_area().
*/
- if ((pos < 0) && (pos + count < pos))
+ /* negative pos is allowed only when the flag is set */
+ if (!(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
+ if ((pos > 0) && (pos + count > 0))
+ return 0;
+ if ((pos > 0) && (pos + count < 0))
+ return -EOVERFLOW;
+ return -EINVAL;
+ }
+ /*
+ * The file supports 'unsigned long' offset. (but loff_t is signed)
+ * When pos is negative, -1 is the biggest number. So if pos + count
+ * is larger than 0, it's overflow.
+ * (ex) -1 + 10 = 9 ...means
+ * 0xffff + 0xa = 0x9 => overflow.
+ */
+ if ((pos < 0) && (pos + count > 0))
return -EOVERFLOW;
- if (file->f_mode & FMODE_UNSIGNED_OFFSET)
- return 0;
- return -EINVAL;
+ return 0;
}
/**
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-10 7:01 ` KAMEZAWA Hiroyuki
@ 2010-12-10 7:18 ` Harsh Bora
2010-12-10 7:59 ` KAMEZAWA Hiroyuki
0 siblings, 1 reply; 15+ messages in thread
From: Harsh Bora @ 2010-12-10 7:18 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote:
> On Fri, 10 Dec 2010 12:09:42 +0530
> Harsh Bora<harsh@linux.vnet.ibm.com> wrote:
> return -EINVAL;
>>> + }
>>> + /*
>>> + * The file supports 'unsigned long' offset. (but loff_t is signed)
>>> + * When pos is negative, -1 is the biggest number. So if pos + count
>>> + * is larger than pos, it's overflow.
>>> + * (ex) -1 + 10 = 9 ...means
>>> + * 0xffff + 0xa = 0x9 => overflow.
>>> + */
>>> + if ((pos< 0)&& (pos + count> 0))
>>
>> Well, that works fine for what I am concerned but I think there is a
>> mismatch in the code and the comment above. As per the comments above,
>> it should be like:
>> if ((pos< 0)&& (pos + count> pos))
>>
>
> Ah, yes. updated. Thank you for review and test.
> -Kame
> ==
> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> negative (unsigned) page offset for very large files as /proc/<pid>/mem
> and /dev/mem.
>
> In that patch, overlap check routine is added but it was wrong.
>
> Considering 'pos' is loff_t, a signed value,
>
> In usual case, at comparing 'pos' and 'pos+count'
>
> (positive) / (positive) OK
> (positive) / (nevative) EOVERFLOW
> (negative) / (positive) EINVAL
> (negative) / (negative) EINVAL
>
> In FMODE_UNSIGNED_OFFSET case,
>
> (positive) / (positive) OK
> (positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
> (nevative) / (negative) OK
> (negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
>
> Changelog:
> - fixed a comment.
>
> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> ---
> fs/read_write.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.37-rc5/fs/read_write.c
> ===================================================================
> --- linux-2.6.37-rc5.orig/fs/read_write.c
> +++ linux-2.6.37-rc5/fs/read_write.c
> @@ -37,11 +37,24 @@ __negative_fpos_check(struct file *file,
> * pos or pos+count is negative here, check overflow.
> * too big "count" will be caught in rw_verify_area().
> */
> - if ((pos< 0)&& (pos + count< pos))
> + /* negative pos is allowed only when the flag is set */
> + if (!(file->f_mode& FMODE_UNSIGNED_OFFSET)) {
> + if ((pos> 0)&& (pos + count> 0))
Do we really need 2 checks? If first one is true, second one has to be
true for count being unsigned?
> + return 0;
> + if ((pos> 0)&& (pos + count< 0))
BTW, when will the above condition be true ? As if first condition is
true, the second cant be true, as the count is unsigned.
Sorry for the lazy review ..
Regards,
Harsh
> + return -EOVERFLOW;
> + return -EINVAL;
> + }
> + /*
> + * The file supports 'unsigned long' offset. (but loff_t is signed)
> + * When pos is negative, -1 is the biggest number. So if pos + count
> + * is larger than 0, it's overflow.
> + * (ex) -1 + 10 = 9 ...means
> + * 0xffff + 0xa = 0x9 => overflow.
> + */
> + if ((pos< 0)&& (pos + count> 0))
> return -EOVERFLOW;
> - if (file->f_mode& FMODE_UNSIGNED_OFFSET)
> - return 0;
> - return -EINVAL;
> + return 0;
> }
>
> /**
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-10 7:18 ` Harsh Bora
@ 2010-12-10 7:59 ` KAMEZAWA Hiroyuki
2010-12-10 8:13 ` Harsh Bora
0 siblings, 1 reply; 15+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-10 7:59 UTC (permalink / raw)
To: Harsh Bora; +Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On Fri, 10 Dec 2010 12:48:05 +0530
Harsh Bora <harsh@linux.vnet.ibm.com> wrote:
> On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote:
> > On Fri, 10 Dec 2010 12:09:42 +0530
> > Harsh Bora<harsh@linux.vnet.ibm.com> wrote:
> > return -EINVAL;
> >>> + }
> >>> + /*
> >>> + * The file supports 'unsigned long' offset. (but loff_t is signed)
> >>> + * When pos is negative, -1 is the biggest number. So if pos + count
> >>> + * is larger than pos, it's overflow.
> >>> + * (ex) -1 + 10 = 9 ...means
> >>> + * 0xffff + 0xa = 0x9 => overflow.
> >>> + */
> >>> + if ((pos< 0)&& (pos + count> 0))
> >>
> >> Well, that works fine for what I am concerned but I think there is a
> >> mismatch in the code and the comment above. As per the comments above,
> >> it should be like:
> >> if ((pos< 0)&& (pos + count> pos))
> >>
> >
> > Ah, yes. updated. Thank you for review and test.
> > -Kame
> > ==
> > commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> > negative (unsigned) page offset for very large files as /proc/<pid>/mem
> > and /dev/mem.
> >
> > In that patch, overlap check routine is added but it was wrong.
> >
> > Considering 'pos' is loff_t, a signed value,
> >
> > In usual case, at comparing 'pos' and 'pos+count'
> >
> > (positive) / (positive) OK
> > (positive) / (nevative) EOVERFLOW
> > (negative) / (positive) EINVAL
> > (negative) / (negative) EINVAL
> >
> > In FMODE_UNSIGNED_OFFSET case,
> >
> > (positive) / (positive) OK
> > (positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
> > (nevative) / (negative) OK
> > (negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
> >
> > Changelog:
> > - fixed a comment.
> >
> > Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> >
> > ---
> > fs/read_write.c | 21 +++++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > Index: linux-2.6.37-rc5/fs/read_write.c
> > ===================================================================
> > --- linux-2.6.37-rc5.orig/fs/read_write.c
> > +++ linux-2.6.37-rc5/fs/read_write.c
> > @@ -37,11 +37,24 @@ __negative_fpos_check(struct file *file,
> > * pos or pos+count is negative here, check overflow.
> > * too big "count" will be caught in rw_verify_area().
> > */
> > - if ((pos< 0)&& (pos + count< pos))
> > + /* negative pos is allowed only when the flag is set */
> > + if (!(file->f_mode& FMODE_UNSIGNED_OFFSET)) {
> > + if ((pos> 0)&& (pos + count> 0))
Hmm.
> Do we really need 2 checks? If first one is true, second one has to be
> true for count being unsigned?
pos is signed value. Then, if pos is near to LOGN_MAX, pos+count can be < 0.
> > + return 0;
> > + if ((pos> 0)&& (pos + count< 0))
> BTW, when will the above condition be true ? As if first condition is
> true, the second cant be true, as the count is unsigned.
>
Ah, hmm, type casting problem ?
(signed) + (unsigned) => (unsigned)
ah, ok. count should be signed...
Is this messy ?
==
commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
negative (unsigned) page offset for very large files as /proc/<pid>/mem
and /dev/mem.
In that patch, overlap check routine is added but it was wrong.
Considering 'pos' is loff_t, a signed value,
In usual case, at comparing 'pos' and 'pos+count'
(positive) / (positive) OK
(positive) / (nevative) EOVERFLOW
(negative) / (positive) EINVAL
(negative) / (negative) EINVAL
In FMODE_UNSIGNED_OFFSET case,
(positive) / (positive) OK
(positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
(nevative) / (negative) OK
(negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
Changelog v1->v2:
- fixed signed+unsigned=unsigned problem.
Changelog v0->v1:
- fixed a comment.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
fs/read_write.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
Index: linux-2.6.37-rc5/fs/read_write.c
===================================================================
--- linux-2.6.37-rc5.orig/fs/read_write.c
+++ linux-2.6.37-rc5/fs/read_write.c
@@ -33,15 +33,31 @@ EXPORT_SYMBOL(generic_ro_fops);
static int
__negative_fpos_check(struct file *file, loff_t pos, size_t count)
{
+ ssize_t len = (ssize_t)count;
+ /* len > 0 is checked before this call. */
+ BUG_ON(len < 0);
/*
* pos or pos+count is negative here, check overflow.
* too big "count" will be caught in rw_verify_area().
*/
- if ((pos < 0) && (pos + count < pos))
+ /* negative pos is allowed only when the flag is set */
+ if (!(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
+ if ((pos > 0) && (pos + len > 0))
+ return 0;
+ if ((pos > 0) && (pos + len < 0))
+ return -EOVERFLOW;
+ return -EINVAL;
+ }
+ /*
+ * The file supports 'unsigned long' offset. (but loff_t is signed)
+ * When pos is negative, -1 is the biggest number. So if pos + count
+ * is larger than 0, it's overflow.
+ * (ex) -1 + 10 = 9 ...means
+ * 0xffff + 0xa = 0x9 => overflow.
+ */
+ if ((pos < 0) && (pos + len > 0))
return -EOVERFLOW;
- if (file->f_mode & FMODE_UNSIGNED_OFFSET)
- return 0;
- return -EINVAL;
+ return 0;
}
/**
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-10 7:59 ` KAMEZAWA Hiroyuki
@ 2010-12-10 8:13 ` Harsh Bora
2010-12-10 8:20 ` KAMEZAWA Hiroyuki
0 siblings, 1 reply; 15+ messages in thread
From: Harsh Bora @ 2010-12-10 8:13 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao, M. Mohan Kumar
On 12/10/2010 01:29 PM, KAMEZAWA Hiroyuki wrote:
> On Fri, 10 Dec 2010 12:48:05 +0530
> Harsh Bora<harsh@linux.vnet.ibm.com> wrote:
>
>> On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote:
>>> On Fri, 10 Dec 2010 12:09:42 +0530
>>> Harsh Bora<harsh@linux.vnet.ibm.com> wrote:
>>> return -EINVAL;
>>>>> + }
>>>>> + /*
>>>>> + * The file supports 'unsigned long' offset. (but loff_t is signed)
>>>>> + * When pos is negative, -1 is the biggest number. So if pos + count
>>>>> + * is larger than pos, it's overflow.
>>>>> + * (ex) -1 + 10 = 9 ...means
>>>>> + * 0xffff + 0xa = 0x9 => overflow.
>>>>> + */
>>>>> + if ((pos< 0)&& (pos + count> 0))
>>>>
>>>> Well, that works fine for what I am concerned but I think there is a
>>>> mismatch in the code and the comment above. As per the comments above,
>>>> it should be like:
>>>> if ((pos< 0)&& (pos + count> pos))
>>>>
>>>
>>> Ah, yes. updated. Thank you for review and test.
>>> -Kame
>>> ==
>>> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
>>> negative (unsigned) page offset for very large files as /proc/<pid>/mem
>>> and /dev/mem.
>>>
>>> In that patch, overlap check routine is added but it was wrong.
>>>
>>> Considering 'pos' is loff_t, a signed value,
>>>
>>> In usual case, at comparing 'pos' and 'pos+count'
>>>
>>> (positive) / (positive) OK
>>> (positive) / (nevative) EOVERFLOW
>>> (negative) / (positive) EINVAL
>>> (negative) / (negative) EINVAL
>>>
>>> In FMODE_UNSIGNED_OFFSET case,
>>>
>>> (positive) / (positive) OK
>>> (positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
>>> (nevative) / (negative) OK
>>> (negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
>>>
>>> Changelog:
>>> - fixed a comment.
>>>
>>> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>>>
>>> ---
>>> fs/read_write.c | 21 +++++++++++++++++----
>>> 1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> Index: linux-2.6.37-rc5/fs/read_write.c
>>> ===================================================================
>>> --- linux-2.6.37-rc5.orig/fs/read_write.c
>>> +++ linux-2.6.37-rc5/fs/read_write.c
>>> @@ -37,11 +37,24 @@ __negative_fpos_check(struct file *file,
>>> * pos or pos+count is negative here, check overflow.
>>> * too big "count" will be caught in rw_verify_area().
>>> */
>>> - if ((pos< 0)&& (pos + count< pos))
>>> + /* negative pos is allowed only when the flag is set */
>>> + if (!(file->f_mode& FMODE_UNSIGNED_OFFSET)) {
>>> + if ((pos> 0)&& (pos + count> 0))
> Hmm.
>
>> Do we really need 2 checks? If first one is true, second one has to be
>> true for count being unsigned?
>
> pos is signed value. Then, if pos is near to LOGN_MAX, pos+count can be< 0.
Well, if you mean that, you need to typecast. Going back to what I
proposed, you need to put it like that:
if ((pos> 0)&& ( (loff_t) (pos + count) > 0))
otherwise, the result of pos + count becomes an unsigned value on a 64
bit system ..
>
>
>>> + return 0;
>>> + if ((pos> 0)&& (pos + count< 0))
>> BTW, when will the above condition be true ? As if first condition is
>> true, the second cant be true, as the count is unsigned.
>>
> Ah, hmm, type casting problem ?
>
> (signed) + (unsigned) => (unsigned)
>
> ah, ok. count should be signed...
No, count shouldnt be signed, you may guess why. typecating the sum to
loff_t is the solution.
Regards,
Harsh
> Is this messy ?
> ==
> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> negative (unsigned) page offset for very large files as /proc/<pid>/mem
> and /dev/mem.
>
> In that patch, overlap check routine is added but it was wrong.
>
> Considering 'pos' is loff_t, a signed value,
>
> In usual case, at comparing 'pos' and 'pos+count'
>
> (positive) / (positive) OK
> (positive) / (nevative) EOVERFLOW
> (negative) / (positive) EINVAL
> (negative) / (negative) EINVAL
>
> In FMODE_UNSIGNED_OFFSET case,
>
> (positive) / (positive) OK
> (positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
> (nevative) / (negative) OK
> (negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
>
> Changelog v1->v2:
> - fixed signed+unsigned=unsigned problem.
> Changelog v0->v1:
> - fixed a comment.
>
> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> ---
> fs/read_write.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> Index: linux-2.6.37-rc5/fs/read_write.c
> ===================================================================
> --- linux-2.6.37-rc5.orig/fs/read_write.c
> +++ linux-2.6.37-rc5/fs/read_write.c
> @@ -33,15 +33,31 @@ EXPORT_SYMBOL(generic_ro_fops);
> static int
> __negative_fpos_check(struct file *file, loff_t pos, size_t count)
> {
> + ssize_t len = (ssize_t)count;
> + /* len> 0 is checked before this call. */
> + BUG_ON(len< 0);
> /*
> * pos or pos+count is negative here, check overflow.
> * too big "count" will be caught in rw_verify_area().
> */
> - if ((pos< 0)&& (pos + count< pos))
> + /* negative pos is allowed only when the flag is set */
> + if (!(file->f_mode& FMODE_UNSIGNED_OFFSET)) {
> + if ((pos> 0)&& (pos + len> 0))
> + return 0;
> + if ((pos> 0)&& (pos + len< 0))
> + return -EOVERFLOW;
> + return -EINVAL;
> + }
> + /*
> + * The file supports 'unsigned long' offset. (but loff_t is signed)
> + * When pos is negative, -1 is the biggest number. So if pos + count
> + * is larger than 0, it's overflow.
> + * (ex) -1 + 10 = 9 ...means
> + * 0xffff + 0xa = 0x9 => overflow.
> + */
> + if ((pos< 0)&& (pos + len> 0))
> return -EOVERFLOW;
> - if (file->f_mode& FMODE_UNSIGNED_OFFSET)
> - return 0;
> - return -EINVAL;
> + return 0;
> }
>
> /**
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-08 12:55 [PATCH] Typecasting required for comparing unlike datatypes Harsh Prateek Bora
2010-12-09 18:32 ` Harsh Bora
2010-12-10 0:53 ` KAMEZAWA Hiroyuki
@ 2010-12-10 8:18 ` KAMEZAWA Hiroyuki
2010-12-10 8:31 ` Harsh Bora
2010-12-15 9:50 ` Al Viro
2 siblings, 2 replies; 15+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-10 8:18 UTC (permalink / raw)
To: Harsh Prateek Bora; +Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On Wed, 8 Dec 2010 18:25:00 +0530
Harsh Prateek Bora <harsh@linux.vnet.ibm.com> wrote:
> The existing code causes the if condition to pass when it should fail
> on a *64-bit kernel* because of implicit data type conversions. It can
> be observed by passing pos = -1 and count = some positive number.
> This results in function returning EOVERFLOW instead of EINVAL.
>
> With this patch, the function returns EINVAL when pos is -1 and count
> is a positive number. This can be tested by calling sendfile with
> offset = -1 and count = some positive number on a 64-bit kernel.
>
> Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
I'm sorry for annoying you.
> ---
> fs/read_write.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 431a0ed..a8eabd4 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
> * pos or pos+count is negative here, check overflow.
> * too big "count" will be caught in rw_verify_area().
> */
> - if ((pos < 0) && (pos + count < pos))
> + if ((pos < 0) && ( (loff_t) (pos + count) < pos))
> return -EOVERFLOW;
> if (file->f_mode & FMODE_UNSIGNED_OFFSET)
> return 0;
> --
> 1.7.1.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-10 8:13 ` Harsh Bora
@ 2010-12-10 8:20 ` KAMEZAWA Hiroyuki
0 siblings, 0 replies; 15+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-10 8:20 UTC (permalink / raw)
To: Harsh Bora
Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao, M. Mohan Kumar
On Fri, 10 Dec 2010 13:43:26 +0530
Harsh Bora <harsh@linux.vnet.ibm.com> wrote:
> On 12/10/2010 01:29 PM, KAMEZAWA Hiroyuki wrote:
> > On Fri, 10 Dec 2010 12:48:05 +0530
> > Harsh Bora<harsh@linux.vnet.ibm.com> wrote:
> >
> >> On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote:
> >>> On Fri, 10 Dec 2010 12:09:42 +0530
> >>> Harsh Bora<harsh@linux.vnet.ibm.com> wrote:
> >>> return -EINVAL;
> >>>>> + }
> >>>>> + /*
> >>>>> + * The file supports 'unsigned long' offset. (but loff_t is signed)
> >>>>> + * When pos is negative, -1 is the biggest number. So if pos + count
> >>>>> + * is larger than pos, it's overflow.
> >>>>> + * (ex) -1 + 10 = 9 ...means
> >>>>> + * 0xffff + 0xa = 0x9 => overflow.
> >>>>> + */
> >>>>> + if ((pos< 0)&& (pos + count> 0))
> >>>>
> >>>> Well, that works fine for what I am concerned but I think there is a
> >>>> mismatch in the code and the comment above. As per the comments above,
> >>>> it should be like:
> >>>> if ((pos< 0)&& (pos + count> pos))
> >>>>
> >>>
> >>> Ah, yes. updated. Thank you for review and test.
> >>> -Kame
> >>> ==
> >>> commit 4a3956c790290efeb647bbb0c3a90476bb57800e adds support for
> >>> negative (unsigned) page offset for very large files as /proc/<pid>/mem
> >>> and /dev/mem.
> >>>
> >>> In that patch, overlap check routine is added but it was wrong.
> >>>
> >>> Considering 'pos' is loff_t, a signed value,
> >>>
> >>> In usual case, at comparing 'pos' and 'pos+count'
> >>>
> >>> (positive) / (positive) OK
> >>> (positive) / (nevative) EOVERFLOW
> >>> (negative) / (positive) EINVAL
> >>> (negative) / (negative) EINVAL
> >>>
> >>> In FMODE_UNSIGNED_OFFSET case,
> >>>
> >>> (positive) / (positive) OK
> >>> (positive) / (nevative) OK (ex. 0x7fff -> 0x8000)
> >>> (nevative) / (negative) OK
> >>> (negative) / (positive) EOVERFLOW (ex. 0xffff -> 0x1)
> >>>
> >>> Changelog:
> >>> - fixed a comment.
> >>>
> >>> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> >>>
> >>> ---
> >>> fs/read_write.c | 21 +++++++++++++++++----
> >>> 1 file changed, 17 insertions(+), 4 deletions(-)
> >>>
> >>> Index: linux-2.6.37-rc5/fs/read_write.c
> >>> ===================================================================
> >>> --- linux-2.6.37-rc5.orig/fs/read_write.c
> >>> +++ linux-2.6.37-rc5/fs/read_write.c
> >>> @@ -37,11 +37,24 @@ __negative_fpos_check(struct file *file,
> >>> * pos or pos+count is negative here, check overflow.
> >>> * too big "count" will be caught in rw_verify_area().
> >>> */
> >>> - if ((pos< 0)&& (pos + count< pos))
> >>> + /* negative pos is allowed only when the flag is set */
> >>> + if (!(file->f_mode& FMODE_UNSIGNED_OFFSET)) {
> >>> + if ((pos> 0)&& (pos + count> 0))
> > Hmm.
> >
> >> Do we really need 2 checks? If first one is true, second one has to be
> >> true for count being unsigned?
> >
> > pos is signed value. Then, if pos is near to LOGN_MAX, pos+count can be< 0.
>
> Well, if you mean that, you need to typecast. Going back to what I
> proposed, you need to put it like that:
> if ((pos> 0)&& ( (loff_t) (pos + count) > 0))
> otherwise, the result of pos + count becomes an unsigned value on a 64
> bit system ..
> >
> >
> >>> + return 0;
> >>> + if ((pos> 0)&& (pos + count< 0))
> >> BTW, when will the above condition be true ? As if first condition is
> >> true, the second cant be true, as the count is unsigned.
> >>
> > Ah, hmm, type casting problem ?
> >
> > (signed) + (unsigned) => (unsigned)
> >
> > ah, ok. count should be signed...
> No, count shouldnt be signed, you may guess why. typecating the sum to
> loff_t is the solution.
>
Ok, I acked to your patch. Thank you for explaining.
Thanks,
-Kame
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-10 8:18 ` KAMEZAWA Hiroyuki
@ 2010-12-10 8:31 ` Harsh Bora
2010-12-15 9:50 ` Al Viro
1 sibling, 0 replies; 15+ messages in thread
From: Harsh Bora @ 2010-12-10 8:31 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao, M. Mohan Kumar
On 12/10/2010 01:48 PM, KAMEZAWA Hiroyuki wrote:
> On Wed, 8 Dec 2010 18:25:00 +0530
> Harsh Prateek Bora<harsh@linux.vnet.ibm.com> wrote:
>
>> The existing code causes the if condition to pass when it should fail
>> on a *64-bit kernel* because of implicit data type conversions. It can
>> be observed by passing pos = -1 and count = some positive number.
>> This results in function returning EOVERFLOW instead of EINVAL.
>>
>> With this patch, the function returns EINVAL when pos is -1 and count
>> is a positive number. This can be tested by calling sendfile with
>> offset = -1 and count = some positive number on a 64-bit kernel.
>>
>> Signed-off-by: Harsh Prateek Bora<harsh@linux.vnet.ibm.com>
>
> Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> I'm sorry for annoying you.
>
Oh, but I am not annoyed, .. Thanks for the ack though ! :)
>> ---
>> fs/read_write.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 431a0ed..a8eabd4 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -38,7 +38,7 @@ __negative_fpos_check(struct file *file, loff_t pos, size_t count)
>> * pos or pos+count is negative here, check overflow.
>> * too big "count" will be caught in rw_verify_area().
>> */
>> - if ((pos< 0)&& (pos + count< pos))
>> + if ((pos< 0)&& ( (loff_t) (pos + count)< pos))
>> return -EOVERFLOW;
>> if (file->f_mode& FMODE_UNSIGNED_OFFSET)
>> return 0;
>> --
>> 1.7.1.1
>>
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-10 8:18 ` KAMEZAWA Hiroyuki
2010-12-10 8:31 ` Harsh Bora
@ 2010-12-15 9:50 ` Al Viro
2010-12-16 0:24 ` KAMEZAWA Hiroyuki
1 sibling, 1 reply; 15+ messages in thread
From: Al Viro @ 2010-12-15 9:50 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki
Cc: Harsh Prateek Bora, linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On Fri, Dec 10, 2010 at 05:18:51PM +0900, KAMEZAWA Hiroyuki wrote:
> On Wed, 8 Dec 2010 18:25:00 +0530
> Harsh Prateek Bora <harsh@linux.vnet.ibm.com> wrote:
>
> > The existing code causes the if condition to pass when it should fail
> > on a *64-bit kernel* because of implicit data type conversions. It can
> > be observed by passing pos = -1 and count = some positive number.
> > This results in function returning EOVERFLOW instead of EINVAL.
> >
> > With this patch, the function returns EINVAL when pos is -1 and count
> > is a positive number. This can be tested by calling sendfile with
> > offset = -1 and count = some positive number on a 64-bit kernel.
> >
> > Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>
> I'm sorry for annoying you.
NAK. Look at what's getting done there; this check is the _only_ place
in function that uses count, it is constantly false if count is zero
_and_ we have only one caller that passes non-zero. Moreover, in case of
count being zero we ignore offset as well.
Obvious things to do:
a) take the check in question into the only caller that would
pass non-zero count (i.e. rw_verify_area())
b) lose the second and the third arguments of __negative_fpos_check()
c) sort out what the hell should be done in that place.
Note that current logics is very convoluted. First of all, we rely on
loff_t being long long in the kernel and size_t being not bigger than
unsigned long long. See ((loff_t)(pos + count) < 0) in there - if count
gets wider than pos, we suddenly get all kinds of odd crap.
That's OK; if we run into ports with size_t wider than 64 bits, we'll have
more trouble anyway.
So what we have for signed offset case is pos >= 0, count <= max loff_t - pos.
Fair enough. For unsigned offset case it's more interesting - we want to
allow pos < 0, and we just want to prohibit wraparounds from negative to
positive. IOW, it's pos >= 0 || count < -pos; note that we already have
count <= max ssize_t, which we assume to be no more than max loff_t.
So what we need is this:
if (unlikely(pos < 0)) {
if it's signed
-EINVAL
if (count >= -pos) /* both values are in 0..LLONG_MAX */
-EOVERFLOW
} else if (unlikely((loff_t)(pos + count) < 0)) {
if it's signed
-EINVAL
}
and we are done with that. IOW, how about the patch below?
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/read_write.c b/fs/read_write.c
index 5d431ba..5520f8a 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -30,18 +30,9 @@ const struct file_operations generic_ro_fops = {
EXPORT_SYMBOL(generic_ro_fops);
-static int
-__negative_fpos_check(struct file *file, loff_t pos, size_t count)
+static inline int unsigned_offsets(struct file *file)
{
- /*
- * pos or pos+count is negative here, check overflow.
- * too big "count" will be caught in rw_verify_area().
- */
- if ((pos < 0) && (pos + count < pos))
- return -EOVERFLOW;
- if (file->f_mode & FMODE_UNSIGNED_OFFSET)
- return 0;
- return -EINVAL;
+ return file->f_mode & FMODE_UNSIGNED_OFFSET;
}
/**
@@ -75,7 +66,7 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
break;
}
- if (offset < 0 && __negative_fpos_check(file, offset, 0))
+ if (offset < 0 && !unsigned_offsets(file))
return -EINVAL;
if (offset > inode->i_sb->s_maxbytes)
return -EINVAL;
@@ -152,7 +143,7 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin)
offset += file->f_pos;
}
retval = -EINVAL;
- if (offset >= 0 || !__negative_fpos_check(file, offset, 0)) {
+ if (offset >= 0 || unsigned_offsets(file)) {
if (offset != file->f_pos) {
file->f_pos = offset;
file->f_version = 0;
@@ -252,9 +243,13 @@ int rw_verify_area(int read_write, struct file *file, loff_t *ppos, size_t count
if (unlikely((ssize_t) count < 0))
return retval;
pos = *ppos;
- if (unlikely((pos < 0) || (loff_t) (pos + count) < 0)) {
- retval = __negative_fpos_check(file, pos, count);
- if (retval)
+ if (unlikely(pos < 0)) {
+ if (!unsigned_offsets(file))
+ return retval;
+ if (count >= -pos) /* both values are in 0..LLONG_MAX */
+ return -EOVERFLOW;
+ } else if (unlikely((loff_t) (pos + count) < 0)) {
+ if (!unsigned_offsets(file))
return retval;
}
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-15 9:50 ` Al Viro
@ 2010-12-16 0:24 ` KAMEZAWA Hiroyuki
2010-12-19 7:02 ` Harsh Bora
0 siblings, 1 reply; 15+ messages in thread
From: KAMEZAWA Hiroyuki @ 2010-12-16 0:24 UTC (permalink / raw)
To: Al Viro
Cc: Harsh Prateek Bora, linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On Wed, 15 Dec 2010 09:50:24 +0000
Al Viro <viro@ZenIV.linux.org.uk> wrote:
> On Fri, Dec 10, 2010 at 05:18:51PM +0900, KAMEZAWA Hiroyuki wrote:
> > On Wed, 8 Dec 2010 18:25:00 +0530
> > Harsh Prateek Bora <harsh@linux.vnet.ibm.com> wrote:
> >
> > > The existing code causes the if condition to pass when it should fail
> > > on a *64-bit kernel* because of implicit data type conversions. It can
> > > be observed by passing pos = -1 and count = some positive number.
> > > This results in function returning EOVERFLOW instead of EINVAL.
> > >
> > > With this patch, the function returns EINVAL when pos is -1 and count
> > > is a positive number. This can be tested by calling sendfile with
> > > offset = -1 and count = some positive number on a 64-bit kernel.
> > >
> > > Signed-off-by: Harsh Prateek Bora <harsh@linux.vnet.ibm.com>
> >
> > Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> >
> > I'm sorry for annoying you.
>
> NAK. Look at what's getting done there; this check is the _only_ place
> in function that uses count, it is constantly false if count is zero
> _and_ we have only one caller that passes non-zero. Moreover, in case of
> count being zero we ignore offset as well.
>
> Obvious things to do:
> a) take the check in question into the only caller that would
> pass non-zero count (i.e. rw_verify_area())
> b) lose the second and the third arguments of __negative_fpos_check()
> c) sort out what the hell should be done in that place.
>
> Note that current logics is very convoluted. First of all, we rely on
> loff_t being long long in the kernel and size_t being not bigger than
> unsigned long long. See ((loff_t)(pos + count) < 0) in there - if count
> gets wider than pos, we suddenly get all kinds of odd crap.
>
> That's OK; if we run into ports with size_t wider than 64 bits, we'll have
> more trouble anyway.
>
> So what we have for signed offset case is pos >= 0, count <= max loff_t - pos.
> Fair enough. For unsigned offset case it's more interesting - we want to
> allow pos < 0, and we just want to prohibit wraparounds from negative to
> positive. IOW, it's pos >= 0 || count < -pos; note that we already have
> count <= max ssize_t, which we assume to be no more than max loff_t.
>
> So what we need is this:
> if (unlikely(pos < 0)) {
> if it's signed
> -EINVAL
> if (count >= -pos) /* both values are in 0..LLONG_MAX */
> -EOVERFLOW
> } else if (unlikely((loff_t)(pos + count) < 0)) {
> if it's signed
> -EINVAL
> }
> and we are done with that. IOW, how about the patch below?
>
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
This seems much easier to read. thank you.
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
and sorry for my 1st implemenation.
> ---
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 5d431ba..5520f8a 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -30,18 +30,9 @@ const struct file_operations generic_ro_fops = {
>
> EXPORT_SYMBOL(generic_ro_fops);
>
> -static int
> -__negative_fpos_check(struct file *file, loff_t pos, size_t count)
> +static inline int unsigned_offsets(struct file *file)
> {
> - /*
> - * pos or pos+count is negative here, check overflow.
> - * too big "count" will be caught in rw_verify_area().
> - */
> - if ((pos < 0) && (pos + count < pos))
> - return -EOVERFLOW;
> - if (file->f_mode & FMODE_UNSIGNED_OFFSET)
> - return 0;
> - return -EINVAL;
> + return file->f_mode & FMODE_UNSIGNED_OFFSET;
> }
>
> /**
> @@ -75,7 +66,7 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
> break;
> }
>
> - if (offset < 0 && __negative_fpos_check(file, offset, 0))
> + if (offset < 0 && !unsigned_offsets(file))
> return -EINVAL;
> if (offset > inode->i_sb->s_maxbytes)
> return -EINVAL;
> @@ -152,7 +143,7 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin)
> offset += file->f_pos;
> }
> retval = -EINVAL;
> - if (offset >= 0 || !__negative_fpos_check(file, offset, 0)) {
> + if (offset >= 0 || unsigned_offsets(file)) {
> if (offset != file->f_pos) {
> file->f_pos = offset;
> file->f_version = 0;
> @@ -252,9 +243,13 @@ int rw_verify_area(int read_write, struct file *file, loff_t *ppos, size_t count
> if (unlikely((ssize_t) count < 0))
> return retval;
> pos = *ppos;
> - if (unlikely((pos < 0) || (loff_t) (pos + count) < 0)) {
> - retval = __negative_fpos_check(file, pos, count);
> - if (retval)
> + if (unlikely(pos < 0)) {
> + if (!unsigned_offsets(file))
> + return retval;
> + if (count >= -pos) /* both values are in 0..LLONG_MAX */
> + return -EOVERFLOW;
> + } else if (unlikely((loff_t) (pos + count) < 0)) {
> + if (!unsigned_offsets(file))
> return retval;
> }
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Typecasting required for comparing unlike datatypes
2010-12-16 0:24 ` KAMEZAWA Hiroyuki
@ 2010-12-19 7:02 ` Harsh Bora
0 siblings, 0 replies; 15+ messages in thread
From: Harsh Bora @ 2010-12-19 7:02 UTC (permalink / raw)
To: Al Viro
Cc: KAMEZAWA Hiroyuki, linux-fsdevel, fengguang.wu, aneesh.kumar, jvrao
On 12/16/2010 05:54 AM, KAMEZAWA Hiroyuki wrote:
> On Wed, 15 Dec 2010 09:50:24 +0000
> Al Viro<viro@ZenIV.linux.org.uk> wrote:
>
>> On Fri, Dec 10, 2010 at 05:18:51PM +0900, KAMEZAWA Hiroyuki wrote:
>>> On Wed, 8 Dec 2010 18:25:00 +0530
>>> Harsh Prateek Bora<harsh@linux.vnet.ibm.com> wrote:
>>>
>>>> The existing code causes the if condition to pass when it should fail
>>>> on a *64-bit kernel* because of implicit data type conversions. It can
>>>> be observed by passing pos = -1 and count = some positive number.
>>>> This results in function returning EOVERFLOW instead of EINVAL.
>>>>
>>>> With this patch, the function returns EINVAL when pos is -1 and count
>>>> is a positive number. This can be tested by calling sendfile with
>>>> offset = -1 and count = some positive number on a 64-bit kernel.
>>>>
>>>> Signed-off-by: Harsh Prateek Bora<harsh@linux.vnet.ibm.com>
>>>
>>> Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>>>
>>> I'm sorry for annoying you.
>>
>> NAK. Look at what's getting done there; this check is the _only_ place
>> in function that uses count, it is constantly false if count is zero
>> _and_ we have only one caller that passes non-zero. Moreover, in case of
>> count being zero we ignore offset as well.
>>
>> Obvious things to do:
>> a) take the check in question into the only caller that would
>> pass non-zero count (i.e. rw_verify_area())
>> b) lose the second and the third arguments of __negative_fpos_check()
>> c) sort out what the hell should be done in that place.
>>
>> Note that current logics is very convoluted. First of all, we rely on
>> loff_t being long long in the kernel and size_t being not bigger than
>> unsigned long long. See ((loff_t)(pos + count)< 0) in there - if count
>> gets wider than pos, we suddenly get all kinds of odd crap.
>>
>> That's OK; if we run into ports with size_t wider than 64 bits, we'll have
>> more trouble anyway.
>>
>> So what we have for signed offset case is pos>= 0, count<= max loff_t - pos.
>> Fair enough. For unsigned offset case it's more interesting - we want to
>> allow pos< 0, and we just want to prohibit wraparounds from negative to
>> positive. IOW, it's pos>= 0 || count< -pos; note that we already have
>> count<= max ssize_t, which we assume to be no more than max loff_t.
>>
>> So what we need is this:
>> if (unlikely(pos< 0)) {
>> if it's signed
>> -EINVAL
>> if (count>= -pos) /* both values are in 0..LLONG_MAX */
>> -EOVERFLOW
>> } else if (unlikely((loff_t)(pos + count)< 0)) {
>> if it's signed
>> -EINVAL
>> }
>> and we are done with that. IOW, how about the patch below?
>>
>>
>> Signed-off-by: Al Viro<viro@zeniv.linux.org.uk>
>
> This seems much easier to read. thank you.
>
> Acked-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>
> and sorry for my 1st implemenation.
>
>> ---
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 5d431ba..5520f8a 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -30,18 +30,9 @@ const struct file_operations generic_ro_fops = {
>>
>> EXPORT_SYMBOL(generic_ro_fops);
>>
>> -static int
>> -__negative_fpos_check(struct file *file, loff_t pos, size_t count)
>> +static inline int unsigned_offsets(struct file *file)
>> {
>> - /*
>> - * pos or pos+count is negative here, check overflow.
>> - * too big "count" will be caught in rw_verify_area().
>> - */
>> - if ((pos< 0)&& (pos + count< pos))
>> - return -EOVERFLOW;
>> - if (file->f_mode& FMODE_UNSIGNED_OFFSET)
>> - return 0;
>> - return -EINVAL;
>> + return file->f_mode& FMODE_UNSIGNED_OFFSET;
>> }
>>
>> /**
>> @@ -75,7 +66,7 @@ generic_file_llseek_unlocked(struct file *file, loff_t offset, int origin)
>> break;
>> }
>>
>> - if (offset< 0&& __negative_fpos_check(file, offset, 0))
>> + if (offset< 0&& !unsigned_offsets(file))
>> return -EINVAL;
>> if (offset> inode->i_sb->s_maxbytes)
>> return -EINVAL;
>> @@ -152,7 +143,7 @@ loff_t default_llseek(struct file *file, loff_t offset, int origin)
>> offset += file->f_pos;
>> }
>> retval = -EINVAL;
>> - if (offset>= 0 || !__negative_fpos_check(file, offset, 0)) {
>> + if (offset>= 0 || unsigned_offsets(file)) {
>> if (offset != file->f_pos) {
>> file->f_pos = offset;
>> file->f_version = 0;
>> @@ -252,9 +243,13 @@ int rw_verify_area(int read_write, struct file *file, loff_t *ppos, size_t count
>> if (unlikely((ssize_t) count< 0))
>> return retval;
>> pos = *ppos;
>> - if (unlikely((pos< 0) || (loff_t) (pos + count)< 0)) {
>> - retval = __negative_fpos_check(file, pos, count);
>> - if (retval)
>> + if (unlikely(pos< 0)) {
>> + if (!unsigned_offsets(file))
>> + return retval;
>> + if (count>= -pos) /* both values are in 0..LLONG_MAX */
Yeah .. thats a better reorg of code, however I am not sure if we need a
typecast above for future portability reasons (as count and pos can be
of diff width somewhere/sometime). Also, I personally would like to keep
the function name as is_offset_unsigned().
Anyways, +1 for ACK !
Regards,
Harsh
>> + return -EOVERFLOW;
>> + } else if (unlikely((loff_t) (pos + count)< 0)) {
>> + if (!unsigned_offsets(file))
>> return retval;
>> }
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2010-12-19 7:02 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-08 12:55 [PATCH] Typecasting required for comparing unlike datatypes Harsh Prateek Bora
2010-12-09 18:32 ` Harsh Bora
2010-12-10 0:06 ` KAMEZAWA Hiroyuki
2010-12-10 0:53 ` KAMEZAWA Hiroyuki
2010-12-10 6:39 ` Harsh Bora
2010-12-10 7:01 ` KAMEZAWA Hiroyuki
2010-12-10 7:18 ` Harsh Bora
2010-12-10 7:59 ` KAMEZAWA Hiroyuki
2010-12-10 8:13 ` Harsh Bora
2010-12-10 8:20 ` KAMEZAWA Hiroyuki
2010-12-10 8:18 ` KAMEZAWA Hiroyuki
2010-12-10 8:31 ` Harsh Bora
2010-12-15 9:50 ` Al Viro
2010-12-16 0:24 ` KAMEZAWA Hiroyuki
2010-12-19 7:02 ` Harsh Bora
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.