From mboxrd@z Thu Jan 1 00:00:00 1970 From: Harsh Bora Subject: Re: [PATCH] Typecasting required for comparing unlike datatypes Date: Fri, 10 Dec 2010 13:43:26 +0530 Message-ID: <4D01E126.2070705@linux.vnet.ibm.com> References: <1291812900-18311-1-git-send-email-harsh@linux.vnet.ibm.com> <20101210095336.a3f33d55.kamezawa.hiroyu@jp.fujitsu.com> <4D01CB2E.4000205@linux.vnet.ibm.com> <20101210160104.6704de94.kamezawa.hiroyu@jp.fujitsu.com> <4D01D42D.1030404@linux.vnet.ibm.com> <20101210165959.8b25d6c2.kamezawa.hiroyu@jp.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, fengguang.wu@intel.com, aneesh.kumar@linux.vnet.ibm.com, jvrao@linux.vnet.ibm.com, "M. Mohan Kumar" To: KAMEZAWA Hiroyuki Return-path: Received: from e28smtp03.in.ibm.com ([122.248.162.3]:43897 "EHLO e28smtp03.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753151Ab0LJINb (ORCPT ); Fri, 10 Dec 2010 03:13:31 -0500 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by e28smtp03.in.ibm.com (8.14.4/8.13.1) with ESMTP id oBA8DR2A017586 for ; Fri, 10 Dec 2010 13:43:27 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id oBA8DRl94055218 for ; Fri, 10 Dec 2010 13:43:27 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id oBA8DQJM025705 for ; Fri, 10 Dec 2010 13:43:27 +0530 In-Reply-To: <20101210165959.8b25d6c2.kamezawa.hiroyu@jp.fujitsu.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 12/10/2010 01:29 PM, KAMEZAWA Hiroyuki wrote: > On Fri, 10 Dec 2010 12:48:05 +0530 > Harsh Bora wrote: > >> On 12/10/2010 12:31 PM, KAMEZAWA Hiroyuki wrote: >>> On Fri, 10 Dec 2010 12:09:42 +0530 >>> Harsh Bora 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//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 >>> >>> --- >>> 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//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 > > --- > 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; > } > > /** > >