[PATCH] ceph: fix type promotion bug on 32bit systems

[PATCH] ceph: fix type promotion bug on 32bit systems

[Dan Carpenter]

In this code "ret" is type long and "src_objlen" is unsigned int.  The
problem is that on 32bit systems, when we do the comparison signed longs
are type promoted to unsigned int.  So negative error codes from
do_splice_direct() are treated as success instead of failure.

Fixes: 1b0c3b9f91f0 ("ceph: re-org copy_file_range and fix some error paths")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
32bit is so weird and ancient.  It's strange to think that unsigned int
has more positive bits than signed long.

 fs/ceph/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index b1da02f5dbe3..b5f8038065d7 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -2969,7 +2969,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 		ret = do_splice_direct(src_file, &src_off, dst_file,
 				       &dst_off, src_objlen, flags);
 		/* Abort on short copies or on error */
-		if (ret < src_objlen) {
+		if (ret < (long)src_objlen) {
 			dout("Failed partial copy (%zd)\n", ret);
 			goto out;
 		}
-- 
2.39.2

Re: [PATCH] ceph: fix type promotion bug on 32bit systems

[Xiubo Li]

On 10/7/23 16:52, Dan Carpenter wrote:
> In this code "ret" is type long and "src_objlen" is unsigned int.  The
> problem is that on 32bit systems, when we do the comparison signed longs
> are type promoted to unsigned int.  So negative error codes from
> do_splice_direct() are treated as success instead of failure.
>
> Fixes: 1b0c3b9f91f0 ("ceph: re-org copy_file_range and fix some error paths")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> 32bit is so weird and ancient.  It's strange to think that unsigned int
> has more positive bits than signed long.
>
>   fs/ceph/file.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index b1da02f5dbe3..b5f8038065d7 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -2969,7 +2969,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
>   		ret = do_splice_direct(src_file, &src_off, dst_file,
>   				       &dst_off, src_objlen, flags);
>   		/* Abort on short copies or on error */
> -		if (ret < src_objlen) {
> +		if (ret < (long)src_objlen) {
>   			dout("Failed partial copy (%zd)\n", ret);
>   			goto out;
>   		}

Good catch and makes sense to me.

I also ran a test in 64bit system, the output is the same too:

int x = -1
unsigned int y = 2
x > y

Reviewed-by: Xiubo Li <xiubli@redhat.com>

Thanks

- Xiubo

Re: [PATCH] ceph: fix type promotion bug on 32bit systems

[Luis Henriques]

Dan Carpenter <dan.carpenter@linaro.org> writes:

> In this code "ret" is type long and "src_objlen" is unsigned int.  The
> problem is that on 32bit systems, when we do the comparison signed longs
> are type promoted to unsigned int.  So negative error codes from
> do_splice_direct() are treated as success instead of failure.
>
> Fixes: 1b0c3b9f91f0 ("ceph: re-org copy_file_range and fix some error paths")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> 32bit is so weird and ancient.  It's strange to think that unsigned int
> has more positive bits than signed long.

Yikes! Thanks for catching this, Dan.  Really tricky.  I guess you used
some static analysis tool (smatch?) to highlight this issue for you,
right?

Anyway, feel free to add my

Reviewed-by: Luis Henriques <lhenriques@suse.de>

Cheers,
-- 
Luís


>
> fs/ceph/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index b1da02f5dbe3..b5f8038065d7 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -2969,7 +2969,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
>  		ret = do_splice_direct(src_file, &src_off, dst_file,
>  				       &dst_off, src_objlen, flags);
>  		/* Abort on short copies or on error */
> -		if (ret < src_objlen) {
> +		if (ret < (long)src_objlen) {
>  			dout("Failed partial copy (%zd)\n", ret);
>  			goto out;
>  		}
> -- 
>
> 2.39.2
>

Re: [PATCH] ceph: fix type promotion bug on 32bit systems

[Dan Carpenter]

On Mon, Oct 09, 2023 at 02:39:38PM +0100, Luis Henriques wrote:
> Dan Carpenter <dan.carpenter@linaro.org> writes:
> 
> > In this code "ret" is type long and "src_objlen" is unsigned int.  The
> > problem is that on 32bit systems, when we do the comparison signed longs
> > are type promoted to unsigned int.  So negative error codes from
> > do_splice_direct() are treated as success instead of failure.
> >
> > Fixes: 1b0c3b9f91f0 ("ceph: re-org copy_file_range and fix some error paths")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > 32bit is so weird and ancient.  It's strange to think that unsigned int
> > has more positive bits than signed long.
> 
> Yikes! Thanks for catching this, Dan.  Really tricky.  I guess you used
> some static analysis tool (smatch?) to highlight this issue for you,
> right?

Yes.  I've pushed this check but you need the cross function DB to know
which functions return error codes so most people won't see the warning.

regards,
dan carpenter

Re: [PATCH] ceph: fix type promotion bug on 32bit systems

[Dan Carpenter]

On Sun, Oct 08, 2023 at 08:21:45AM +0800, Xiubo Li wrote:
> 
> On 10/7/23 16:52, Dan Carpenter wrote:
> > In this code "ret" is type long and "src_objlen" is unsigned int.  The
> > problem is that on 32bit systems, when we do the comparison signed longs
> > are type promoted to unsigned int.  So negative error codes from
> > do_splice_direct() are treated as success instead of failure.
> > 
> > Fixes: 1b0c3b9f91f0 ("ceph: re-org copy_file_range and fix some error paths")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > 32bit is so weird and ancient.  It's strange to think that unsigned int
> > has more positive bits than signed long.
> > 
> >   fs/ceph/file.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > index b1da02f5dbe3..b5f8038065d7 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> > @@ -2969,7 +2969,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> >   		ret = do_splice_direct(src_file, &src_off, dst_file,
> >   				       &dst_off, src_objlen, flags);
> >   		/* Abort on short copies or on error */
> > -		if (ret < src_objlen) {
> > +		if (ret < (long)src_objlen) {
> >   			dout("Failed partial copy (%zd)\n", ret);
> >   			goto out;
> >   		}
> 
> Good catch and makes sense to me.
> 

Thanks.

> I also ran a test in 64bit system, the output is the same too:
> 
> int x = -1
> unsigned int y = 2
> x > y

Here none of the types are int.  It's long and unsigned int.  So how
type promotion works (normally, there are also weird exceptions like ?:
and <<) is when you have two variables then you by default at least type
promote both sides to int.  But if one side is larger than int, then you
type promote it to which ever has more positive bits.

regards,
dan carpenter