Re: [PATCH] Typecasting required for comparing unlike datatypes

[Harsh Bora <harsh@linux.vnet.ibm.com>]

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;
>   }
>
>   /**
>
>