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

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

[Andy Shevchenko]

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@linux.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))
 		goto fail;
 	snapc = ceph_create_snap_context(num, GFP_NOFS);
 	if (!snapc)
-- 
2.50.1

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[Viacheslav Dubeyko]

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@linux.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?

Thanks,
Slava.

>  		goto fail;
>  	snapc = ceph_create_snap_context(num, GFP_NOFS);
>  	if (!snapc)

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[andriy.shevchenko]

On Mon, Nov 10, 2025 at 07:37:13PM +0000, Viacheslav Dubeyko 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.

...

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

Maybe. Or even maybe the U32_MAX is the way to go: Does anybody check those
corner cases? Are those never tested? Potential (security) bug?

...

Whatever you find, in case if it will be not the proposed solution as is,
consider these patches as Reported-by.

And thanks for the reviews!

-- 
With Best Regards,
Andy Shevchenko

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

[Viacheslav Dubeyko]

On Mon, 2025-11-10 at 21:43 +0200, andriy.shevchenko@linux.intel.com wrote:
> On Mon, Nov 10, 2025 at 07:37:13PM +0000, Viacheslav Dubeyko 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.
> 
> ...
> 
> > > -	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?
> 
> Maybe. Or even maybe the U32_MAX is the way to go: Does anybody check those
> corner cases? Are those never tested? Potential (security) bug?
> 
> ...
> 
> Whatever you find, in case if it will be not the proposed solution as is,
> consider these patches as Reported-by.
> 
> And thanks for the reviews!

I think we can take the patch as it. It looks good. Probably, it makes sense to
take a deeper look in the code on our side.

Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[andriy.shevchenko]

On Mon, Nov 10, 2025 at 08:42:13PM +0000, Viacheslav Dubeyko wrote:
> On Mon, 2025-11-10 at 21:43 +0200, andriy.shevchenko@linux.intel.com wrote:
> > On Mon, Nov 10, 2025 at 07:37:13PM +0000, Viacheslav Dubeyko 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.
> > 
> > ...
> > 
> > > > -	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?
> > 
> > Maybe. Or even maybe the U32_MAX is the way to go: Does anybody check those
> > corner cases? Are those never tested? Potential (security) bug?
> > 
> > ...
> > 
> > Whatever you find, in case if it will be not the proposed solution as is,
> > consider these patches as Reported-by.
> > 
> > And thanks for the reviews!
> 
> I think we can take the patch as it. It looks good. Probably, it makes sense to
> take a deeper look in the code on our side.
> 
> Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks, can this be applied? My builds are still broken.

-- 
With Best Regards,
Andy Shevchenko

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

[Viacheslav Dubeyko]

On Mon, 2025-11-24 at 17:09 +0200, andriy.shevchenko@linux.intel.com wrote:
> On Mon, Nov 10, 2025 at 08:42:13PM +0000, Viacheslav Dubeyko wrote:
> > On Mon, 2025-11-10 at 21:43 +0200, andriy.shevchenko@linux.intel.com wrote:
> > > On Mon, Nov 10, 2025 at 07:37:13PM +0000, Viacheslav Dubeyko 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.
> > > 
> > > ...
> > > 
> > > > > -	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?
> > > 
> > > Maybe. Or even maybe the U32_MAX is the way to go: Does anybody check those
> > > corner cases? Are those never tested? Potential (security) bug?
> > > 
> > > ...
> > > 
> > > Whatever you find, in case if it will be not the proposed solution as is,
> > > consider these patches as Reported-by.
> > > 
> > > And thanks for the reviews!
> > 
> > I think we can take the patch as it. It looks good. Probably, it makes sense to
> > take a deeper look in the code on our side.
> > 
> > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> 
> Thanks, can this be applied? My builds are still broken.

The patchset has been applied on testing branch already [1].

Ilya, when are we planning to send this patchset upstream?

Thanks,
Slava.

[1] https://github.com/ceph/ceph-client.git

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[andriy.shevchenko]

On Mon, Nov 24, 2025 at 05:47:04PM +0000, Viacheslav Dubeyko wrote:
> On Mon, 2025-11-24 at 17:09 +0200, andriy.shevchenko@linux.intel.com wrote:
> > On Mon, Nov 10, 2025 at 08:42:13PM +0000, Viacheslav Dubeyko wrote:
> > > On Mon, 2025-11-10 at 21:43 +0200, andriy.shevchenko@linux.intel.com wrote:

...

> > > I think we can take the patch as it. It looks good. Probably, it makes sense to
> > > take a deeper look in the code on our side.
> > > 
> > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > 
> > Thanks, can this be applied? My builds are still broken.
> 
> The patchset has been applied on testing branch already [1].

Thanks for the information. The Linux Next still has no that branch in.
I recommend to write Stephen a message to include your testing branch or
something like that into Linux Next, so it will get more test coverage.

> Ilya, when are we planning to send this patchset upstream?

> [1] https://github.com/ceph/ceph-client.git

-- 
With Best Regards,
Andy Shevchenko

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

[Viacheslav Dubeyko]

On Mon, 2025-11-24 at 20:43 +0200, andriy.shevchenko@linux.intel.com wrote:
> On Mon, Nov 24, 2025 at 05:47:04PM +0000, Viacheslav Dubeyko wrote:
> > On Mon, 2025-11-24 at 17:09 +0200, andriy.shevchenko@linux.intel.com wrote:
> > > On Mon, Nov 10, 2025 at 08:42:13PM +0000, Viacheslav Dubeyko wrote:
> > > > On Mon, 2025-11-10 at 21:43 +0200, andriy.shevchenko@linux.intel.com wrote:
> 
> ...
> 
> > > > I think we can take the patch as it. It looks good. Probably, it makes sense to
> > > > take a deeper look in the code on our side.
> > > > 
> > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > 
> > > Thanks, can this be applied? My builds are still broken.
> > 
> > The patchset has been applied on testing branch already [1].
> 
> Thanks for the information. The Linux Next still has no that branch in.
> I recommend to write Stephen a message to include your testing branch or
> something like that into Linux Next, so it will get more test coverage.

It's not my personal branch. :) It's the branch that receives all new patches
for CephFS kernel client and, usually, it is used for internal testing by Ceph
team. Ilya gathers all patches from there and sends it upstream. So, we need to
ping Ilya. :)

Thanks,
Slava.

> 
> > Ilya, when are we planning to send this patchset upstream?
> 
> > [1] https://github.com/ceph/ceph-client.git  

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[andriy.shevchenko]

On Mon, Nov 24, 2025 at 06:49:26PM +0000, Viacheslav Dubeyko wrote:
> On Mon, 2025-11-24 at 20:43 +0200, andriy.shevchenko@linux.intel.com wrote:
> > On Mon, Nov 24, 2025 at 05:47:04PM +0000, Viacheslav Dubeyko wrote:
> > > On Mon, 2025-11-24 at 17:09 +0200, andriy.shevchenko@linux.intel.com wrote:
> > > > On Mon, Nov 10, 2025 at 08:42:13PM +0000, Viacheslav Dubeyko wrote:
> > > > > On Mon, 2025-11-10 at 21:43 +0200, andriy.shevchenko@linux.intel.com wrote:

...

> > > > > I think we can take the patch as it. It looks good. Probably, it makes sense to
> > > > > take a deeper look in the code on our side.
> > > > > 
> > > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > 
> > > > Thanks, can this be applied? My builds are still broken.
> > > 
> > > The patchset has been applied on testing branch already [1].
> > 
> > Thanks for the information. The Linux Next still has no that branch in.
> > I recommend to write Stephen a message to include your testing branch or
> > something like that into Linux Next, so it will get more test coverage.
> 
> It's not my personal branch. :) It's the branch that receives all new patches
> for CephFS kernel client and, usually, it is used for internal testing by Ceph
> team.

Exactly, that's why my proposal. I would not recommend to do the same for
a (semi-)private branches.

> Ilya gathers all patches from there and sends it upstream. So, we need to
> ping Ilya. :)

> > > Ilya, when are we planning to send this patchset upstream?
> > 
> > > [1] https://github.com/ceph/ceph-client.git  

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[Gregory Farnum]

On Mon, Nov 10, 2025 at 11:42 AM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.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@linux.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).
-Greg

>
> Thanks,
> Slava.
>
> >               goto fail;
> >       snapc = ceph_create_snap_context(num, GFP_NOFS);
> >       if (!snapc)

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

[Viacheslav Dubeyko]

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@ibm.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@linux.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?

Thanks,
Slava.

> > 
> > >               goto fail;
> > >       snapc = ceph_create_snap_context(num, GFP_NOFS);
> > >       if (!snapc)

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[Gregory Farnum]

On Mon, Nov 10, 2025 at 11:57 AM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.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@ibm.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@linux.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?

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

[Viacheslav Dubeyko]

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@ibm.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@ibm.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@linux.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.

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[david laight]

On Mon, 10 Nov 2025 15:44:04 +0100
Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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.

Did you really read the code?
The test itself needs moving into ceph_create_snap_context().
Possibly by using kmalloc_array() to do the multiply.

But in any case are large values sane at all?
Allocating very large kernel memory blocks isn't a good idea at all.

In fact this does a kmalloc(... GFP_NOFS) which is pretty likely to
fail for even moderate sized requests. I bet it fails 64k (order 4?)
on a regular basis.

Perhaps all three value that get added to make 'num' need 'sanity limits'
that mean a large allocation just can't happen.

	David

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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))
>  		goto fail;
>  	snapc = ceph_create_snap_context(num, GFP_NOFS);
>  	if (!snapc)

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[Andy Shevchenko]

On Tue, Nov 25, 2025 at 09:55:16AM +0000, david laight wrote:
> On Mon, 10 Nov 2025 15:44:04 +0100
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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.
> 
> Did you really read the code?

I read the piece that prevents builds. The exercise on how to fix this properly
is delegated to the authors and maintainers.

> The test itself needs moving into ceph_create_snap_context().
> Possibly by using kmalloc_array() to do the multiply.
> 
> But in any case are large values sane at all?
> Allocating very large kernel memory blocks isn't a good idea at all.
> 
> In fact this does a kmalloc(... GFP_NOFS) which is pretty likely to
> fail for even moderate sized requests. I bet it fails 64k (order 4?)
> on a regular basis.
> 
> Perhaps all three value that get added to make 'num' need 'sanity limits'
> that mean a large allocation just can't happen.

Nice, can you send a followup to fix all that in a better way?
(I don't care about the fix as long as it doesn't break my builds)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] ceph: Amend checking to fix `make W=1` build breakage

[david laight]

On Tue, 25 Nov 2025 12:17:10 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Nov 25, 2025 at 09:55:16AM +0000, david laight wrote:
> > On Mon, 10 Nov 2025 15:44:04 +0100
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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.  
> > 
> > Did you really read the code?  
> 
> I read the piece that prevents builds. The exercise on how to fix this properly
> is delegated to the authors and maintainers.
> 
> > The test itself needs moving into ceph_create_snap_context().
> > Possibly by using kmalloc_array() to do the multiply.
> > 
> > But in any case are large values sane at all?
> > Allocating very large kernel memory blocks isn't a good idea at all.
> > 
> > In fact this does a kmalloc(... GFP_NOFS) which is pretty likely to
> > fail for even moderate sized requests. I bet it fails 64k (order 4?)
> > on a regular basis.
> > 
> > Perhaps all three value that get added to make 'num' need 'sanity limits'
> > that mean a large allocation just can't happen.  
> 
> Nice, can you send a followup to fix all that in a better way?
> (I don't care about the fix as long as it doesn't break my builds)
> 

Perhaps -Wtautological-constant-out-of-range-compare should just be delegated
to W=2 like (IIRC) -Wtype-bounds has been which is pretty much the same test.

	David

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

[Viacheslav Dubeyko]

On Tue, 2025-11-25 at 09:55 +0000, david laight wrote:
> On Mon, 10 Nov 2025 15:44:04 +0100
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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.
> 
> Did you really read the code?
> The test itself needs moving into ceph_create_snap_context().
> Possibly by using kmalloc_array() to do the multiply.
> 
> But in any case are large values sane at all?
> Allocating very large kernel memory blocks isn't a good idea at all.
> 
> In fact this does a kmalloc(... GFP_NOFS) which is pretty likely to
> fail for even moderate sized requests. I bet it fails 64k (order 4?)
> on a regular basis.
> 
> Perhaps all three value that get added to make 'num' need 'sanity limits'
> that mean a large allocation just can't happen.
> 
> 

Yeah, I also really dislike this pattern. The SIZE_MAX has been used in several
places of net/ceph and in fs/ceph. I believe that this particular code works
only because nobody creates crazy number of snapshots. But even if num value
will be crazy big, then ceph_create_snap_context() will fail to allocate memory.
So, functionally, this code is not so bad. But, logically this check of the num
value is not very reasonable because, anyway, it doesn't protect from the try to
allocate unreasonable amount of memory. So, probably, it makes sense to exchange
the SIZE_MAX on KMALLOC_MAX_SIZE. I believe it will make likewise check more
reasonable and functional.

Thanks,
Slava.

> 
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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))
> >  		goto fail;
> >  	snapc = ceph_create_snap_context(num, GFP_NOFS);
> >  	if (!snapc)
>