| Message ID | 20241112161019.4154616-2-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | md W=1 build fixes | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_13-PR | success | PR summary |
| mdraidci/vmtest-md-6_13-VM_Test-0 | success | Logs for per-patch-testing |
On Tue, Nov 12, 2024 at 8:10 AM John Garry <john.g.garry@oracle.com> wrote: > > For compiling with W=1, the following warning can be seen: > > drivers/md/raid5.c: In function ‘setup_conf’: > drivers/md/raid5.c:2423:12: error: ‘%s’ directive output may be truncated writing up to 31 bytes into a region of size between 16 and 26 [-Werror=format-truncation=] > "raid%d-%s", conf->level, mdname(conf->mddev)); > ^~ > drivers/md/raid5.c:2422:3: note: ‘snprintf’ output between 7 and 48 bytes into a destination of size 32 This is a bit confusing. Does this mean we actually need 48 bytes? I played with it myself, and 38 bytes is indeed enough to silent the warning. With 38 bytes, we have 4 bytes hole right behind cache_name, so I am thinking we should just use 40. WDYT? Thanks, Song > snprintf(conf->cache_name[0], namelen, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > "raid%d-%s", conf->level, mdname(conf->mddev)); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > Increase the array size to avoid this warning. > > Signed-off-by: John Garry <john.g.garry@oracle.com> [...]
On 12/11/2024 17:42, Song Liu wrote: > On Tue, Nov 12, 2024 at 8:10 AM John Garry<john.g.garry@oracle.com> wrote: >> For compiling with W=1, the following warning can be seen: >> >> drivers/md/raid5.c: In function ‘setup_conf’: >> drivers/md/raid5.c:2423:12: error: ‘%s’ directive output may be truncated writing up to 31 bytes into a region of size between 16 and 26 [-Werror=format-truncation=] >> "raid%d-%s", conf->level, mdname(conf->mddev)); >> ^~ >> drivers/md/raid5.c:2422:3: note: ‘snprintf’ output between 7 and 48 bytes into a destination of size 32 > This is a bit confusing. Does this mean we actually need 48 bytes? > I played with it myself, and 38 bytes is indeed enough to silent the > warning. With 38 bytes, we have 4 bytes hole right behind > cache_name, so I am thinking we should just use 40. > > WDYT? Indeed it is confusing... So the string is "raid%d-%s", which is 4B for "raid" 10B for max int (right?) 1B for '-' 32B for DISK_NAME_LEN 1B for NUL which totals 48 So I don't know why/how 38 is ok. Maybe there is some auto-padding going on, like you hint at. Maybe just using 48 is better. Thanks, John
On Tue, Nov 12, 2024 at 10:09 AM John Garry <john.g.garry@oracle.com> wrote: > > On 12/11/2024 17:42, Song Liu wrote: > > On Tue, Nov 12, 2024 at 8:10 AM John Garry<john.g.garry@oracle.com> wrote: > >> For compiling with W=1, the following warning can be seen: > >> > >> drivers/md/raid5.c: In function ‘setup_conf’: > >> drivers/md/raid5.c:2423:12: error: ‘%s’ directive output may be truncated writing up to 31 bytes into a region of size between 16 and 26 [-Werror=format-truncation=] > >> "raid%d-%s", conf->level, mdname(conf->mddev)); > >> ^~ > >> drivers/md/raid5.c:2422:3: note: ‘snprintf’ output between 7 and 48 bytes into a destination of size 32 > > This is a bit confusing. Does this mean we actually need 48 bytes? > > I played with it myself, and 38 bytes is indeed enough to silent the > > warning. With 38 bytes, we have 4 bytes hole right behind > > cache_name, so I am thinking we should just use 40. > > > > WDYT? > > Indeed it is confusing... > So the string is "raid%d-%s", which is > 4B for "raid" > 10B for max int (right?) > 1B for '-' > 32B for DISK_NAME_LEN > 1B for NUL > > which totals 48 > > So I don't know why/how 38 is ok. Maybe there is some auto-padding going > on, like you hint at. > > Maybe just using 48 is better. Makes sense. I will update the patch to use 48, and apply it to md-6.13. Thanks, Song
On 12/11/2024 23:05, Song Liu wrote: >>> WDYT? >> Indeed it is confusing... >> So the string is "raid%d-%s", which is >> 4B for "raid" >> 10B for max int (right?) >> 1B for '-' >> 32B for DISK_NAME_LEN >> 1B for NUL >> >> which totals 48 >> >> So I don't know why/how 38 is ok. Maybe there is some auto-padding going >> on, like you hint at. >> >> Maybe just using 48 is better. > Makes sense. I will update the patch to use 48, and apply it to md-6.13. ok, thanks. And let me know what you think of 2/2, I am even less happy about the solution there. Cheers
On Wed, Nov 13, 2024 at 2:18 AM John Garry <john.g.garry@oracle.com> wrote: > > On 12/11/2024 23:05, Song Liu wrote: > >>> WDYT? > >> Indeed it is confusing... > >> So the string is "raid%d-%s", which is > >> 4B for "raid" > >> 10B for max int (right?) > >> 1B for '-' > >> 32B for DISK_NAME_LEN > >> 1B for NUL > >> > >> which totals 48 > >> > >> So I don't know why/how 38 is ok. Maybe there is some auto-padding going > >> on, like you hint at. > >> > >> Maybe just using 48 is better. > > Makes sense. I will update the patch to use 48, and apply it to md-6.13. > > ok, thanks. > > And let me know what you think of 2/2, I am even less happy about the > solution there. 2/2 will go through the device mapper side. Please refer to the following (from MAINTAINERS). DEVICE-MAPPER (LVM) M: Alasdair Kergon <agk@redhat.com> M: Mike Snitzer <snitzer@kernel.org> M: Mikulas Patocka <mpatocka@redhat.com> L: dm-devel@lists.linux.dev S: Maintained Q: http://patchwork.kernel.org/project/dm-devel/list/ T: git git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git Thanks, Song
On 13/11/2024 18:15, Song Liu wrote: >> And let me know what you think of 2/2, I am even less happy about the >> solution there. > 2/2 will go through the device mapper side. Please refer to the following > (from MAINTAINERS). > > DEVICE-MAPPER (LVM) > M: Alasdair Kergon<agk@redhat.com> > M: Mike Snitzer<snitzer@kernel.org> > M: Mikulas Patocka<mpatocka@redhat.com> > L:dm-devel@lists.linux.dev > S: Maintained > Q:https://urldefense.com/v3/__http://patchwork.kernel.org/project/dm- > devel/list/__;!!ACWV5N9M2RV99hQ! > KdbGHn2DKjuTy6kVmIAkHuremU7VP_2xxwX0e0r8X-3bOEa3FJiA6YnmzXBUbvRTA51J9UGUubVTFnw$ > T: git git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git ah, ok. I'll resend. Thanks
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h index 896ecfc4afa6..3da5b412190b 100644 --- a/drivers/md/raid5.h +++ b/drivers/md/raid5.h @@ -633,7 +633,7 @@ struct r5conf { * two caches. */ int active_name; - char cache_name[2][32]; + char cache_name[2][38]; struct kmem_cache *slab_cache; /* for allocating stripes */ struct mutex cache_size_mutex; /* Protect changes to cache size */
For compiling with W=1, the following warning can be seen: drivers/md/raid5.c: In function ‘setup_conf’: drivers/md/raid5.c:2423:12: error: ‘%s’ directive output may be truncated writing up to 31 bytes into a region of size between 16 and 26 [-Werror=format-truncation=] "raid%d-%s", conf->level, mdname(conf->mddev)); ^~ drivers/md/raid5.c:2422:3: note: ‘snprintf’ output between 7 and 48 bytes into a destination of size 32 snprintf(conf->cache_name[0], namelen, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ "raid%d-%s", conf->level, mdname(conf->mddev)); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors Increase the array size to avoid this warning. Signed-off-by: John Garry <john.g.garry@oracle.com> --- drivers/md/raid5.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)