linux-kernel - RE: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8de7721da35000ff0f41b0b024a998dfecf9ef90.camel@ibm.com>
Date: Mon, 10 Nov 2025 20:03:26 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: Gregory Farnum <gfarnum@...hat.com>
CC: Xiubo Li <xiubli@...hat.com>,
        "justinstitt@...gle.com"
	<justinstitt@...gle.com>,
        "andriy.shevchenko@...ux.intel.com"
	<andriy.shevchenko@...ux.intel.com>,
        "llvm@...ts.linux.dev"
	<llvm@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "nathan@...nel.org" <nathan@...nel.org>,
        "morbo@...gle.com" <morbo@...gle.com>,
        "idryomov@...il.com"
	<idryomov@...il.com>,
        "nick.desaulniers+lkml@...il.com"
	<nick.desaulniers+lkml@...il.com>,
        "ceph-devel@...r.kernel.org"
	<ceph-devel@...r.kernel.org>
Subject: RE: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build
 breakage

On Mon, 2025-11-10 at 12:00 -0800, Gregory Farnum wrote:
> On Mon, Nov 10, 2025 at 11:57 AM Viacheslav Dubeyko
> <Slava.Dubeyko@....com> wrote:
> > 
> > On Mon, 2025-11-10 at 11:48 -0800, Gregory Farnum wrote:
> > > On Mon, Nov 10, 2025 at 11:42 AM Viacheslav Dubeyko
> > > <Slava.Dubeyko@....com> wrote:
> > > > 
> > > > On Mon, 2025-11-10 at 15:44 +0100, Andy Shevchenko wrote:
> > > > > In a few cases the code compares 32-bit value to a SIZE_MAX derived
> > > > > constant which is much higher than that value on 64-bit platforms,
> > > > > Clang, in particular, is not happy about this
> > > > > 
> > > > > fs/ceph/snap.c:377:10: error: result of comparison of constant 2305843009213693948 with expression of type 'u32' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> > > > >   377 |         if (num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
> > > > >       |             ~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > 
> > > > > Fix this by casting to size_t. Note, that possible replacement of SIZE_MAX
> > > > > by U32_MAX may lead to the behaviour changes on the corner cases.
> > > > > 
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > > > > ---
> > > > >  fs/ceph/snap.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> > > > > index c65f2b202b2b..521507ea8260 100644
> > > > > --- a/fs/ceph/snap.c
> > > > > +++ b/fs/ceph/snap.c
> > > > > @@ -374,7 +374,7 @@ static int build_snap_context(struct ceph_mds_client *mdsc,
> > > > > 
> > > > >       /* alloc new snap context */
> > > > >       err = -ENOMEM;
> > > > > -     if (num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
> > > > > +     if ((size_t)num > (SIZE_MAX - sizeof(*snapc)) / sizeof(u64))
> > > > 
> > > > The same question is here. Does it makes sense to declare num as size_t? Could
> > > > it be more clean solution? Or could it introduce another warnings/errors?
> > > 
> > > Given that the number of snaps is constrained over the wire as a
> > > 32-bit integer, you probably want to keep that mapping...(Though I
> > > guess it's the sum of two 32-bit integers which technically could
> > > overflow, and I'm not sure what happens if you actually hit those
> > > boundaries on the server — but nobody generates snapshots on the same
> > > file in that quantity).
> > > 
> > > All that said, it'd be kind of nice if we could just annotate for
> > > clang that we are perfectly happy for the evaluation to always be true
> > > on a 64-bit architecture (as snapids are 64 bits, we will always be
> > > able to count them).
> > 
> > So, are you suggesting to declare num as u64 here? Am I correct?
> 
> What? No, the whole point of this block is checking that it can track
> all the snapshots and allocate them (on a <32-bit architecture, which
> are the only ones at risk), isn't it?

OK. So, what is your point? Are we taking the patch as it is? Or do we need to
make some modifications?

Thanks,
Slava.