| Message ID | 20231221133928.49824-1-dmantipov@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | rbd: use check_sub_overflow() to limit the number of snapshots | expand |
On Thu, Dec 21, 2023 at 2:40 PM Dmitry Antipov <dmantipov@yandex.ru> wrote: > > When compiling with clang-18 and W=1, I've noticed the following > warning: > > drivers/block/rbd.c:6093:17: warning: result of comparison of constant > 2305843009213693948 with expression of type 'u32' (aka 'unsigned int') > is always false [-Wtautological-constant-out-of-range-compare] > 6093 | if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context)) > | ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 6094 | / sizeof (u64)) { > | ~~~~~~~~~~~~~~ > > Since the plain check with '>' makes sense only if U32_MAX == SIZE_MAX > which is not true for the 64-bit kernel, prefer 'check_sub_overflow()' > in 'rbd_dev_v2_snap_context()' and 'rbd_dev_ondisk_valid()' as well. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > drivers/block/rbd.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index a999b698b131..ef8e6fbc9a79 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -933,7 +933,7 @@ static bool rbd_image_format_valid(u32 image_format) > > static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk) > { > - size_t size; > + size_t size, result; > u32 snap_count; > > /* The header has to start with the magic rbd header text */ > @@ -956,7 +956,7 @@ static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk) > */ > snap_count = le32_to_cpu(ondisk->snap_count); > size = SIZE_MAX - sizeof (struct ceph_snap_context); > - if (snap_count > size / sizeof (__le64)) > + if (check_sub_overflow(size / sizeof(__le64), snap_count, &result)) Hi Dmitry, There is a limit on the number of snapshots: #define RBD_MAX_SNAP_COUNT 510 /* allows max snapc to fit in 4KB */ It's not direct, but it's a hard limit, at least in the current implementation. Let's just replace these with direct comparisons for RBD_MAX_SNAP_COUNT. Thanks, Ilya
On 12/22/23 8:23 AM, Ilya Dryomov wrote: > On Thu, Dec 21, 2023 at 2:40 PM Dmitry Antipov <dmantipov@yandex.ru> wrote: >> >> When compiling with clang-18 and W=1, I've noticed the following >> warning: >> >> drivers/block/rbd.c:6093:17: warning: result of comparison of constant >> 2305843009213693948 with expression of type 'u32' (aka 'unsigned int') >> is always false [-Wtautological-constant-out-of-range-compare] >> 6093 | if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context)) >> | ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 6094 | / sizeof (u64)) { >> | ~~~~~~~~~~~~~~ >> >> Since the plain check with '>' makes sense only if U32_MAX == SIZE_MAX >> which is not true for the 64-bit kernel, prefer 'check_sub_overflow()' >> in 'rbd_dev_v2_snap_context()' and 'rbd_dev_ondisk_valid()' as well. >> >> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> >> --- >> drivers/block/rbd.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index a999b698b131..ef8e6fbc9a79 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -933,7 +933,7 @@ static bool rbd_image_format_valid(u32 image_format) >> >> static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk) >> { >> - size_t size; >> + size_t size, result; >> u32 snap_count; >> >> /* The header has to start with the magic rbd header text */ >> @@ -956,7 +956,7 @@ static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk) >> */ >> snap_count = le32_to_cpu(ondisk->snap_count); >> size = SIZE_MAX - sizeof (struct ceph_snap_context); >> - if (snap_count > size / sizeof (__le64)) >> + if (check_sub_overflow(size / sizeof(__le64), snap_count, &result)) > > Hi Dmitry, > > There is a limit on the number of snapshots: > > #define RBD_MAX_SNAP_COUNT 510 /* allows max snapc to fit in 4KB */ > > It's not direct, but it's a hard limit, at least in the current > implementation. Let's just replace these with direct comparisons for > RBD_MAX_SNAP_COUNT. The point of the original code was to ensure the on-disk snap_count value was sane, but there's no point in checking for a 64-bit size_t. Comparing against RBD_MAX_SNAP_COUNT is better. I think it's safe to assume RBD_MAX_SNAP_COUNT (or rather, the size required to hold that many snapshot IDs) can always be represented in a size_t. -Alex > > Thanks, > > Ilya >
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index a999b698b131..ef8e6fbc9a79 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -933,7 +933,7 @@ static bool rbd_image_format_valid(u32 image_format) static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk) { - size_t size; + size_t size, result; u32 snap_count; /* The header has to start with the magic rbd header text */ @@ -956,7 +956,7 @@ static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk) */ snap_count = le32_to_cpu(ondisk->snap_count); size = SIZE_MAX - sizeof (struct ceph_snap_context); - if (snap_count > size / sizeof (__le64)) + if (check_sub_overflow(size / sizeof(__le64), snap_count, &result)) return false; /* @@ -6090,8 +6090,8 @@ static int rbd_dev_v2_snap_context(struct rbd_device *rbd_dev, * make sure the computed size of the snapshot context we * allocate is representable in a size_t. */ - if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context)) - / sizeof (u64)) { + if (check_sub_overflow((SIZE_MAX - sizeof(struct ceph_snap_context)) + / sizeof(u64), snap_count, &size)) { ret = -EINVAL; goto out; }
When compiling with clang-18 and W=1, I've noticed the following warning: drivers/block/rbd.c:6093:17: warning: result of comparison of constant 2305843009213693948 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare] 6093 | if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context)) | ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 6094 | / sizeof (u64)) { | ~~~~~~~~~~~~~~ Since the plain check with '>' makes sense only if U32_MAX == SIZE_MAX which is not true for the 64-bit kernel, prefer 'check_sub_overflow()' in 'rbd_dev_v2_snap_context()' and 'rbd_dev_ondisk_valid()' as well. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- drivers/block/rbd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)