[PATCH 0/1] s390/dasd: fix string length handling

[PATCH 0/1] s390/dasd: fix string length handling

[Heiko Carstens]

Hi Jens,

since both Stefan and Jan are not available, I'm sending a simple fix
to address a valid clang finding. Since I expect more reports and
patches for this, I'm sending this now in order to avoid that more
people spend time on this.

Please apply.

Thanks,
Heiko

Heiko Carstens (1):
  s390/dasd: fix string length handling

 drivers/s390/block/dasd_devmap.c |  6 +-----
 drivers/s390/block/dasd_eckd.c   | 10 +++++-----
 drivers/s390/block/dasd_int.h    |  4 ++++
 3 files changed, 10 insertions(+), 10 deletions(-)

-- 
2.39.2

[PATCH 1/1] s390/dasd: fix string length handling

[Heiko Carstens]

Building dasd_eckd.o with latest clang reveals this bug:

    CC      drivers/s390/block/dasd_eckd.o
      drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated;
      specified size is 1, but format string expands to at least 11 [-Wfortify-source]
       1082 |                 snprintf(print_uid, sizeof(*print_uid),
            |                 ^
      drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated;
      specified size is 1, but format string expands to at least 10 [-Wfortify-source]
       1087 |                 snprintf(print_uid, sizeof(*print_uid),
            |                 ^

Fix this by moving and using the existing UID_STRLEN for the arrays
that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN
to clarify its scope.

Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf")
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 drivers/s390/block/dasd_devmap.c |  6 +-----
 drivers/s390/block/dasd_eckd.c   | 10 +++++-----
 drivers/s390/block/dasd_int.h    |  4 ++++
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index 620fab01b710..c4e36650c426 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -1378,16 +1378,12 @@ static ssize_t dasd_vendor_show(struct device *dev,
 
 static DEVICE_ATTR(vendor, 0444, dasd_vendor_show, NULL);
 
-#define UID_STRLEN ( /* vendor */ 3 + 1 + /* serial    */ 14 + 1 +\
-		     /* SSID   */ 4 + 1 + /* unit addr */ 2 + 1 +\
-		     /* vduit */ 32 + 1)
-
 static ssize_t
 dasd_uid_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
+	char uid_string[DASD_UID_STRLEN];
 	struct dasd_device *device;
 	struct dasd_uid uid;
-	char uid_string[UID_STRLEN];
 	char ua_string[3];
 
 	device = dasd_device_from_cdev(to_ccwdev(dev));
diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 8587e423169e..bd89b032968a 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -1079,12 +1079,12 @@ static void dasd_eckd_get_uid_string(struct dasd_conf *conf,
 
 	create_uid(conf, &uid);
 	if (strlen(uid.vduit) > 0)
-		snprintf(print_uid, sizeof(*print_uid),
+		snprintf(print_uid, DASD_UID_STRLEN,
 			 "%s.%s.%04x.%02x.%s",
 			 uid.vendor, uid.serial, uid.ssid,
 			 uid.real_unit_addr, uid.vduit);
 	else
-		snprintf(print_uid, sizeof(*print_uid),
+		snprintf(print_uid, DASD_UID_STRLEN,
 			 "%s.%s.%04x.%02x",
 			 uid.vendor, uid.serial, uid.ssid,
 			 uid.real_unit_addr);
@@ -1093,8 +1093,8 @@ static void dasd_eckd_get_uid_string(struct dasd_conf *conf,
 static int dasd_eckd_check_cabling(struct dasd_device *device,
 				   void *conf_data, __u8 lpm)
 {
+	char print_path_uid[DASD_UID_STRLEN], print_device_uid[DASD_UID_STRLEN];
 	struct dasd_eckd_private *private = device->private;
-	char print_path_uid[60], print_device_uid[60];
 	struct dasd_conf path_conf;
 
 	path_conf.data = conf_data;
@@ -1293,9 +1293,9 @@ static void dasd_eckd_path_available_action(struct dasd_device *device,
 	__u8 path_rcd_buf[DASD_ECKD_RCD_DATA_SIZE];
 	__u8 lpm, opm, npm, ppm, epm, hpfpm, cablepm;
 	struct dasd_conf_data *conf_data;
+	char print_uid[DASD_UID_STRLEN];
 	struct dasd_conf path_conf;
 	unsigned long flags;
-	char print_uid[60];
 	int rc, pos;
 
 	opm = 0;
@@ -5855,8 +5855,8 @@ static void dasd_eckd_dump_sense(struct dasd_device *device,
 static int dasd_eckd_reload_device(struct dasd_device *device)
 {
 	struct dasd_eckd_private *private = device->private;
+	char print_uid[DASD_UID_STRLEN];
 	int rc, old_base;
-	char print_uid[60];
 	struct dasd_uid uid;
 	unsigned long flags;
 
diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index 0aa56351da72..8a4dbe9d7741 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -259,6 +259,10 @@ struct dasd_uid {
 	char vduit[33];
 };
 
+#define DASD_UID_STRLEN ( /* vendor */ 3 + 1 + /* serial    */ 14 + 1 +	\
+			  /* SSID   */ 4 + 1 + /* unit addr */ 2 + 1 +	\
+			  /* vduit */ 32 + 1)
+
 /*
  * PPRC Status data
  */
-- 
2.39.2

RE: [PATCH 1/1] s390/dasd: fix string length handling

[David Laight]

From: Heiko Carstens
> Sent: 28 August 2023 16:32
> 
> Building dasd_eckd.o with latest clang reveals this bug:
> 
>     CC      drivers/s390/block/dasd_eckd.o
>       drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated;
>       specified size is 1, but format string expands to at least 11 [-Wfortify-source]
>        1082 |                 snprintf(print_uid, sizeof(*print_uid),
>             |                 ^
>       drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated;
>       specified size is 1, but format string expands to at least 10 [-Wfortify-source]
>        1087 |                 snprintf(print_uid, sizeof(*print_uid),
>             |                 ^
> 
> Fix this by moving and using the existing UID_STRLEN for the arrays
> that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN
> to clarify its scope.

If you embed that char[] in a struct and pass the address of the
struct then sizeof will return the correct value and you get the
size checked properly.

...
>  	if (strlen(uid.vduit) > 0)

Does the compiler know enough to optimise that brain-dead test?

> -		snprintf(print_uid, sizeof(*print_uid),
> +		snprintf(print_uid, DASD_UID_STRLEN,
>  			 "%s.%s.%04x.%02x.%s",
>  			 uid.vendor, uid.serial, uid.ssid,
>  			 uid.real_unit_addr, uid.vduit);
>  	else
> -		snprintf(print_uid, sizeof(*print_uid),
> +		snprintf(print_uid, DASD_UID_STRLEN,
>  			 "%s.%s.%04x.%02x",
>  			 uid.vendor, uid.serial, uid.ssid,
>  			 uid.real_unit_addr);

or:
	snprintf(print_uid, DASD_UID_STRLEN,
		"%s.%s.%04x.%02x%s%s",
		uid.vendor, uid.serial, uid.ssid,
		uid.real_unit_addr,
		uid.vduit[0] ? "." : "", uid.vduit);

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Nick Desaulniers]

On Mon, Aug 28, 2023 at 05:18:37PM +0000, David Laight wrote:
> From: Heiko Carstens
> > Sent: 28 August 2023 16:32
> >  	if (strlen(uid.vduit) > 0)
> 
> Does the compiler know enough to optimise that brain-dead test?
> 

For the purposes of skipping diagnostics, no; clang performs semantic
analysis BEFORE optimization (which is handled by LLVM). As such, clang
will produce diagnostics on dead code.

Partly because LLVM isn't very ergonomic at emitting diagnostics from
the backend, partly because Clang code owner and developers don't want
clang to emit diagnostics dependent on optimization level.

I disagree with my compatriots, and you can read more thoughts here:
https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261?u=nickdesaulniers

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Heiko Carstens]

On Mon, Aug 28, 2023 at 03:51:00PM -0700, Nick Desaulniers wrote:
> On Mon, Aug 28, 2023 at 05:18:37PM +0000, David Laight wrote:
> > From: Heiko Carstens
> > > Sent: 28 August 2023 16:32
> > >  	if (strlen(uid.vduit) > 0)
> > 
> > Does the compiler know enough to optimise that brain-dead test?
> > 
> 
> For the purposes of skipping diagnostics, no; clang performs semantic
> analysis BEFORE optimization (which is handled by LLVM). As such, clang
> will produce diagnostics on dead code.
> 
> Partly because LLVM isn't very ergonomic at emitting diagnostics from
> the backend, partly because Clang code owner and developers don't want
> clang to emit diagnostics dependent on optimization level.
> 
> I disagree with my compatriots, and you can read more thoughts here:
> https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261?u=nickdesaulniers

Maybe I misunderstand what you write above, however clang (latest+greatest)
does indeed optimize the strlen() away and generates code which only tests
if uid.vduit[0] is zero or not.

Unlike gcc, which does not optimize this away and which uses the strlen()
inline assembly provided via string.h...

RE: [PATCH 1/1] s390/dasd: fix string length handling

[David Laight]

From: Heiko Carstens
> Sent: 29 August 2023 08:49
> 
> On Mon, Aug 28, 2023 at 03:51:00PM -0700, Nick Desaulniers wrote:
> > On Mon, Aug 28, 2023 at 05:18:37PM +0000, David Laight wrote:
> > > From: Heiko Carstens
> > > > Sent: 28 August 2023 16:32
> > > >  	if (strlen(uid.vduit) > 0)
> > >
> > > Does the compiler know enough to optimise that brain-dead test?
> > >
> >
> > For the purposes of skipping diagnostics, no; clang performs semantic
> > analysis BEFORE optimization (which is handled by LLVM). As such, clang
> > will produce diagnostics on dead code.
> >
> > Partly because LLVM isn't very ergonomic at emitting diagnostics from
> > the backend, partly because Clang code owner and developers don't want
> > clang to emit diagnostics dependent on optimization level.
> >
> > I disagree with my compatriots, and you can read more thoughts here:
> > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-
> diagnostics/69261?u=nickdesaulniers
> 
> Maybe I misunderstand what you write above, however clang (latest+greatest)
> does indeed optimize the strlen() away and generates code which only tests
> if uid.vduit[0] is zero or not.
> 
> Unlike gcc, which does not optimize this away and which uses the strlen()
> inline assembly provided via string.h...

And, if -ffreestanding is set (as in some kernel builds), the compiler
can't assume what strlen() does.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Nick Desaulniers]

On Tue, Aug 29, 2023 at 1:32 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Heiko Carstens
> > Sent: 29 August 2023 08:49
> >
> > On Mon, Aug 28, 2023 at 03:51:00PM -0700, Nick Desaulniers wrote:
> > > On Mon, Aug 28, 2023 at 05:18:37PM +0000, David Laight wrote:
> > > > From: Heiko Carstens
> > > > > Sent: 28 August 2023 16:32
> > > > >         if (strlen(uid.vduit) > 0)
> > > >
> > > > Does the compiler know enough to optimise that brain-dead test?
> > > >
> > >
> > > For the purposes of skipping diagnostics, no; clang performs semantic
> > > analysis BEFORE optimization (which is handled by LLVM). As such, clang
> > > will produce diagnostics on dead code.
> > >
> > > Partly because LLVM isn't very ergonomic at emitting diagnostics from
> > > the backend, partly because Clang code owner and developers don't want
> > > clang to emit diagnostics dependent on optimization level.
> > >
> > > I disagree with my compatriots, and you can read more thoughts here:
> > > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-
> > diagnostics/69261?u=nickdesaulniers
> >
> > Maybe I misunderstand what you write above, however clang (latest+greatest)
> > does indeed optimize the strlen() away and generates code which only tests
> > if uid.vduit[0] is zero or not.
> >
> > Unlike gcc, which does not optimize this away and which uses the strlen()
> > inline assembly provided via string.h...
>
> And, if -ffreestanding is set (as in some kernel builds), the compiler
> can't assume what strlen() does.

Exactly.

But triple checking if -ffreestanding is being used in arch/s390/ I only see:

arch/s390/purgatory/Makefile
26:KBUILD_CFLAGS += -fno-zero-initialized-in-bss -fno-builtin -ffreestanding

arch/s390/Makefile
28:KBUILD_CFLAGS_DECOMPRESSOR += -ffreestanding

---
So I don't think -ffreestanding is at play here.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Nick Desaulniers]

On Tue, Aug 29, 2023 at 12:49 AM Heiko Carstens <hca@linux.ibm.com> wrote:
>
> On Mon, Aug 28, 2023 at 03:51:00PM -0700, Nick Desaulniers wrote:
> > On Mon, Aug 28, 2023 at 05:18:37PM +0000, David Laight wrote:
> > > From: Heiko Carstens
> > > > Sent: 28 August 2023 16:32
> > > >   if (strlen(uid.vduit) > 0)
> > >
> > > Does the compiler know enough to optimise that brain-dead test?
> > >
> >
> > For the purposes of skipping diagnostics, no; clang performs semantic
> > analysis BEFORE optimization (which is handled by LLVM). As such, clang
> > will produce diagnostics on dead code.
> >
> > Partly because LLVM isn't very ergonomic at emitting diagnostics from
> > the backend, partly because Clang code owner and developers don't want
> > clang to emit diagnostics dependent on optimization level.
> >
> > I disagree with my compatriots, and you can read more thoughts here:
> > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261?u=nickdesaulniers
>
> Maybe I misunderstand what you write above, however clang (latest+greatest)
> does indeed optimize the strlen() away and generates code which only tests
> if uid.vduit[0] is zero or not.

Oh, yeah, sorry I was talking about something else. Nevermind my point.

>
> Unlike gcc, which does not optimize this away and which uses the strlen()
> inline assembly provided via string.h...

heh, I feel like I was just having a conversation yesterday with
someone about pessimizing compile-time calculations...

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Nick Desaulniers]

On Mon, Aug 28, 2023 at 05:31:42PM +0200, Heiko Carstens wrote:
> Building dasd_eckd.o with latest clang reveals this bug:
> 
>     CC      drivers/s390/block/dasd_eckd.o
>       drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated;
>       specified size is 1, but format string expands to at least 11 [-Wfortify-source]
>        1082 |                 snprintf(print_uid, sizeof(*print_uid),
>             |                 ^
>       drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated;
>       specified size is 1, but format string expands to at least 10 [-Wfortify-source]
>        1087 |                 snprintf(print_uid, sizeof(*print_uid),
>             |                 ^
> 
> Fix this by moving and using the existing UID_STRLEN for the arrays
> that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN
> to clarify its scope.
> 
> Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf")
> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>

Thanks for the patch! Nathan just reported a bunch of these. I took a
look at these two and thought "yeah that's clearly a bug in the kernel
sources." Fix LGTM.

Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://github.com/ClangBuiltLinux/linux/issues/1923
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I also like David's idea of passing `char ident [DASD_UID_STRLEN]`, too,
but I don't feel strongly either way.

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Nick Desaulniers]

On Mon, Aug 28, 2023 at 3:46 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Aug 28, 2023 at 05:31:42PM +0200, Heiko Carstens wrote:
> > Building dasd_eckd.o with latest clang reveals this bug:
> >
> >     CC      drivers/s390/block/dasd_eckd.o
> >       drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated;
> >       specified size is 1, but format string expands to at least 11 [-Wfortify-source]
> >        1082 |                 snprintf(print_uid, sizeof(*print_uid),
> >             |                 ^
> >       drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated;
> >       specified size is 1, but format string expands to at least 10 [-Wfortify-source]
> >        1087 |                 snprintf(print_uid, sizeof(*print_uid),
> >             |                 ^
> >
> > Fix this by moving and using the existing UID_STRLEN for the arrays
> > that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN
> > to clarify its scope.
> >
> > Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf")
> > Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> > Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
>
> Thanks for the patch! Nathan just reported a bunch of these. I took a
> look at these two and thought "yeah that's clearly a bug in the kernel
> sources." Fix LGTM.
>
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1923
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Meant to add:

Tested-by: Nick Desaulniers <ndesaulniers@google.com> # build

>
> I also like David's idea of passing `char ident [DASD_UID_STRLEN]`, too,
> but I don't feel strongly either way.



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Heiko Carstens]

On Mon, Aug 28, 2023 at 03:46:52PM -0700, Nick Desaulniers wrote:
> On Mon, Aug 28, 2023 at 05:31:42PM +0200, Heiko Carstens wrote:
> > Building dasd_eckd.o with latest clang reveals this bug:
> > 
> >     CC      drivers/s390/block/dasd_eckd.o
> >       drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated;
> >       specified size is 1, but format string expands to at least 11 [-Wfortify-source]
> >        1082 |                 snprintf(print_uid, sizeof(*print_uid),
> >             |                 ^
> >       drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated;
> >       specified size is 1, but format string expands to at least 10 [-Wfortify-source]
> >        1087 |                 snprintf(print_uid, sizeof(*print_uid),
> >             |                 ^
> > 
> > Fix this by moving and using the existing UID_STRLEN for the arrays
> > that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN
> > to clarify its scope.
> > 
> > Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf")
> > Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> > Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> 
> Thanks for the patch! Nathan just reported a bunch of these. I took a
> look at these two and thought "yeah that's clearly a bug in the kernel
> sources." Fix LGTM.
> 
> Reported-by: Nathan Chancellor <nathan@kernel.org>
> Closes: https://github.com/ClangBuiltLinux/linux/issues/1923
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> I also like David's idea of passing `char ident [DASD_UID_STRLEN]`, too,
> but I don't feel strongly either way.

Well, this is supposed to be the "minimal" fix. I consider everything else
additional cleanup work, which can and should be done by Stefan and Jan who
maintain this device driver.

For example there is more or less identical code within dasd_devmap.c
(dasd_uid_show()), where it would make sense to de-deduplicate the
code. And then of course there is the already mentioned rather pointless
strlen() invocation; plus there are many other string operations / format
strings, which also should be addressed.
E.g. there are quite a couple of "%p" printk format specifiers which are
pointless, since pointer values get hashed since years - so a more or less
random value will be printed, etc.

However all of this is up to Stefan and Jan.

So I consider this current fix as good enough and final.

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Nick Desaulniers]

On Tue, Aug 29, 2023 at 1:02 AM Heiko Carstens <hca@linux.ibm.com> wrote:
>
> On Mon, Aug 28, 2023 at 03:46:52PM -0700, Nick Desaulniers wrote:
> > On Mon, Aug 28, 2023 at 05:31:42PM +0200, Heiko Carstens wrote:
> > > Building dasd_eckd.o with latest clang reveals this bug:
> > >
> > >     CC      drivers/s390/block/dasd_eckd.o
> > >       drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated;
> > >       specified size is 1, but format string expands to at least 11 [-Wfortify-source]
> > >        1082 |                 snprintf(print_uid, sizeof(*print_uid),
> > >             |                 ^
> > >       drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated;
> > >       specified size is 1, but format string expands to at least 10 [-Wfortify-source]
> > >        1087 |                 snprintf(print_uid, sizeof(*print_uid),
> > >             |                 ^
> > >
> > > Fix this by moving and using the existing UID_STRLEN for the arrays
> > > that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN
> > > to clarify its scope.
> > >
> > > Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf")
> > > Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> > > Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> >
> > Thanks for the patch! Nathan just reported a bunch of these. I took a
> > look at these two and thought "yeah that's clearly a bug in the kernel
> > sources." Fix LGTM.
> >
> > Reported-by: Nathan Chancellor <nathan@kernel.org>
> > Closes: https://github.com/ClangBuiltLinux/linux/issues/1923
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > I also like David's idea of passing `char ident [DASD_UID_STRLEN]`, too,
> > but I don't feel strongly either way.
>
> Well, this is supposed to be the "minimal" fix. I consider everything else
> additional cleanup work, which can and should be done by Stefan and Jan who
> maintain this device driver.

Sure, like I said, I don't care either way.

>
> For example there is more or less identical code within dasd_devmap.c
> (dasd_uid_show()), where it would make sense to de-deduplicate the
> code. And then of course there is the already mentioned rather pointless
> strlen() invocation; plus there are many other string operations / format
> strings, which also should be addressed.
> E.g. there are quite a couple of "%p" printk format specifiers which are
> pointless, since pointer values get hashed since years - so a more or less
> random value will be printed, etc.

kptr_restrict can be disabled at runtime though, so it's not useless
to print pointer values (IMO).

>
> However all of this is up to Stefan and Jan.
>
> So I consider this current fix as good enough and final.

Thanks for the patch.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 1/1] s390/dasd: fix string length handling

[Heiko Carstens]

Hi Jens,

On Mon, Aug 28, 2023 at 05:31:42PM +0200, Heiko Carstens wrote:
> Building dasd_eckd.o with latest clang reveals this bug:
> 
>     CC      drivers/s390/block/dasd_eckd.o
>       drivers/s390/block/dasd_eckd.c:1082:3: warning: 'snprintf' will always be truncated;
>       specified size is 1, but format string expands to at least 11 [-Wfortify-source]
>        1082 |                 snprintf(print_uid, sizeof(*print_uid),
>             |                 ^
>       drivers/s390/block/dasd_eckd.c:1087:3: warning: 'snprintf' will always be truncated;
>       specified size is 1, but format string expands to at least 10 [-Wfortify-source]
>        1087 |                 snprintf(print_uid, sizeof(*print_uid),
>             |                 ^
> 
> Fix this by moving and using the existing UID_STRLEN for the arrays
> that are being written to. Also rename UID_STRLEN to DASD_UID_STRLEN
> to clarify its scope.
> 
> Fixes: 23596961b437 ("s390/dasd: split up dasd_eckd_read_conf")
> Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  drivers/s390/block/dasd_devmap.c |  6 +-----
>  drivers/s390/block/dasd_eckd.c   | 10 +++++-----
>  drivers/s390/block/dasd_int.h    |  4 ++++
>  3 files changed, 10 insertions(+), 10 deletions(-)

Just wondering, will you pick this one up, or should I route this via
the s390 tree?

Re: [PATCH 0/1] s390/dasd: fix string length handling

[Jens Axboe]

On Mon, 28 Aug 2023 17:31:41 +0200, Heiko Carstens wrote:
> since both Stefan and Jan are not available, I'm sending a simple fix
> to address a valid clang finding. Since I expect more reports and
> patches for this, I'm sending this now in order to avoid that more
> people spend time on this.
> 
> Please apply.
> 
> [...]

Applied, thanks!

[1/1] s390/dasd: fix string length handling
      commit: f7cf22424665043787a96a66a048ff6b2cfd473c

Best regards,
-- 
Jens Axboe