From: Dan Carpenter <dan.carpenter@linaro.org>
To: Xiubo Li <xiubli@redhat.com>
Cc: Luis Henriques <lhenriques@suse.com>,
Ilya Dryomov <idryomov@gmail.com>,
Jeff Layton <jlayton@kernel.org>,
ceph-devel@vger.kernel.org, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] ceph: fix type promotion bug on 32bit systems
Date: Tue, 10 Oct 2023 09:54:42 +0300 [thread overview]
Message-ID: <6381caf2-f5e1-4279-bf62-a87bc8c5f023@kadam.mountain> (raw)
In-Reply-To: <a86080f4-3a40-8ae2-9bc5-9859298b7cbb@redhat.com>
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
next prev parent reply other threads:[~2023-10-10 6:54 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-07 8:52 [PATCH] ceph: fix type promotion bug on 32bit systems Dan Carpenter
2023-10-08 0:21 ` Xiubo Li
2023-10-10 6:54 ` Dan Carpenter [this message]
2023-10-09 13:39 ` Luis Henriques
2023-10-10 6:27 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6381caf2-f5e1-4279-bf62-a87bc8c5f023@kadam.mountain \
--to=dan.carpenter@linaro.org \
--cc=ceph-devel@vger.kernel.org \
--cc=idryomov@gmail.com \
--cc=jlayton@kernel.org \
--cc=kernel-janitors@vger.kernel.org \
--cc=lhenriques@suse.com \
--cc=xiubli@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).