Re: [PATCH v2 2/3] libc: fix sign extension in fallocate()

On Tuesday 22 September 2015 11:46 PM, Yuriy Kolerov wrote:
> Arguments offset and len in fallocate system call in Linux
> kernel have 64-bit types. 

Perhaps better to say that for common generic syscall ABI fallocate syscall
handler in kernel expects 64 bit signed args for offset, len

> However these arguments in fallocate
> wrapper in uClibc may have 32-bit types. In this case it's
> necessary to pass two 32-bit words to the systemc call for every
> argument - an actual argument and its sign extension.

No - this is not technically accurate. User space in theory can pass a true 64 bit
value, not just a 32 bit value and a 32 bit sign extension.

> Thus high word of 64-bit value must be 0 or 0xFFFFFFFF depending
> on sign of the original 32-bit value (offset or len). It is how
> sign extansion works - all high bits of the negative value must be 1.
>
> fallocate does sign extension incorrectly when 32 bit values are
> passed (offset or len). It just fills the second word of 64-bit
> value by zeros. E.g. fallocate works incorrectly when offset or
> length is negative value - in this case kernel thinks that positive
> values are passed.

Again while the fix may be obvious, can u divulge the specific test case it fixes.


> Signed-off-by: Yuriy Kolerov <[email protected]>
> ---
>  libc/sysdeps/linux/common/fallocate.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/libc/sysdeps/linux/common/fallocate.c b/libc/sysdeps/linux/common/fallocate.c
> index b2309e9..446d9ef 100644
> --- a/libc/sysdeps/linux/common/fallocate.c
> +++ b/libc/sysdeps/linux/common/fallocate.c
> @@ -21,14 +21,12 @@ int attribute_hidden __libc_fallocate(int fd, int mode, __off_t offset, __off_t
>  	int ret;
>  
>  # if __WORDSIZE == 32
> -	uint32_t off_low = offset;
> -	uint32_t len_low = len;
> -	/* may assert that these >>31 are 0 */
> -	uint32_t zero = 0;
> +	int32_t offset_sign_extension = (offset < 0) ? 0xFFFFFFFF : 0;
> +	int32_t len_sign_extension = (len < 0) ? 0xFFFFFFFF : 0;
>  	INTERNAL_SYSCALL_DECL(err);
>  	ret = (int) (INTERNAL_SYSCALL(fallocate, err, 6, fd, mode,
> -		__LONG_LONG_PAIR (zero, off_low),
> -		__LONG_LONG_PAIR (zero, len_low)));
> +		__LONG_LONG_PAIR (offset_sign_extension, offset),
> +		__LONG_LONG_PAIR (len_sign_extension, len)));

This is too ugly.  Can u use existing macro OFF64_HI_LO() macro.
Something like below:

int64_t offset_ll = offset ; /* does appropriate sign extension */
int64_t len_ll = len;

ret = (int) (INTERNAL_SYSCALL(fallocate, err, 6, fd, mode,
            OFF64_HI_LO(offset_ll),
            OFF64_HI_LO(len_ll));


>  # elif __WORDSIZE == 64
>  	INTERNAL_SYSCALL_DECL(err);
>  	ret = (int) (INTERNAL_SYSCALL(fallocate, err, 4, fd, mode, offset, len));