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

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

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