| Message ID | 5e0418d3-a31b-4231-80bf-99adca6bcbe5@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix type promotion bug on 32bit systems | expand |
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
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,
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
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
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; }
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(-)