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

Arguments offset and len in fallocate system call in Linux
kernel have 64-bit types. 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.

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.

Signed-off-by: Yuriy Kolerov <yuriy.kolerov@synopsys.com>
---
 libc/sysdeps/linux/common/fallocate.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

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 <yuriy.kolerov@synopsys.com>
> ---
>  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));

Hi Vineet!

Thanks for your remarks - I will fix my patches. Some comments below.

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

That is why 2 functions exist in uClibc: fallocate and fallocate64. If you want to pass a true 64-bit value on 32-bit machine you need to use fallocate64. In my comment I meant a case when fallocate (not fallocate64) is used on 32-bit machine. I will make it clearer in the next version of patch.

> > ---
> >  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));

I think it would be easier to call fallocate64 from fallocate:

int attribute_hidden __libc_fallocate(int fd, int mode, __off_t offset, __off_t len)
{
# if __WORDSIZE == 32
	return fallocate64(fd, mode, offset, len);
# elif __WORDSIZE == 64
	int ret;
	...

On Wednesday 23 September 2015 03:18 PM, Yuriy Kolerov wrote:
> I think it would be easier to call fallocate64 from fallocate:
>
> int attribute_hidden __libc_fallocate(int fd, int mode, __off_t offset, __off_t len)
> {
> # if __WORDSIZE == 32
> 	return fallocate64(fd, mode, offset, len);
> # elif __WORDSIZE == 64
> 	int ret;
> 	...

Indeed this is even better. You may wanna add a comment though which explains the
subtle upcasting.