[PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

There is still a gcc-11 warning about stringop-overread in some
configurations, despite the earlier fix that was meant to address
this issue:

fs/qnx4/dir.c: In function 'qnx4_readdir':
fs/qnx4/dir.c:76:32: error: 'strnlen' specified bound [16, 48] exceeds source size 1 [-Werror=stringop-overread]
   76 |                         size = strnlen(name, size);
      |                                ^~~~~~~~~~~~~~~~~~~
fs/qnx4/dir.c:26:22: note: source object declared here
   26 |                 char de_name;
      |                      ^~~~~~~

The problem here is that we access the same pointer using two different
structure layouts, but gcc determines the object size based on
whatever it encounters first, in this case, the single-byte field.

Rework the union once more, to have a flexible-array member as the
thing that gets accessed first to deal with current gcc versions, but
leave everything else in place so that a future fixed compiler will
correctly see the struct member sizes.

Unfortunately, this makes the code really ugly again. In my original
patch, I just changed the if (!de->de_name) check to access the longer
of the two fields, which worked. Another option would be to make
de_name a longer array.

Fixes: b7213ffa0e58 ("qnx4: avoid stringop-overread errors")
Link: https://lore.kernel.org/all/20210322160253.4032422-6-arnd@kernel.org/
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: rebase on top of the earlier patch.
---
 fs/qnx4/dir.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index 2a66844b7ff8..d7eb5a9b79e4 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -23,8 +23,16 @@
  */
 union qnx4_directory_entry {
 	struct {
-		char de_name;
-		char de_pad[62];
+		/*
+		 * work around gcc-11.x using the first field it observes
+		 * to determing the actual length
+		 * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
+		 */
+		char __empty[0];
+		char de_name[];
+	};
+	struct {
+		char de_pad[63];
 		char de_status;
 	};
 	struct qnx4_inode_entry inode;
@@ -58,7 +66,7 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
 			offset = ix * QNX4_DIR_ENTRY_SIZE;
 			de = (union qnx4_directory_entry *) (bh->b_data + offset);
 
-			if (!de->de_name)
+			if (!de->de_name[0])
 				continue;
 			if (!(de->de_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
 				continue;
-- 
2.29.2

Re: [PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

On Mon, Sep 20, 2021 at 5:12 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> +               /*
> +                * work around gcc-11.x using the first field it observes
> +                * to determing the actual length
> +                * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> +                */
> +               char __empty[0];
> +               char de_name[];

Ugh. That looks _really_ hacky.

It sounds like we can avoid the gcc bug if we just always use
"de->de_name[]". Then we don't need to depend on magical behavior
about one particular gcc version and a strange empty array in front of
it.

IOW, something like the attached simpler thing that just does that
"always use de_name[]" and has a comment about why we don't do the
natural thing

Also, just what version of gcc is the broken one? You say "gcc-11",
but I certainly don't see it with _my_ version of gcc-11, so can we
(just for that comment) document more precisely what version you have
(or possibly what config you use to trigger it).

                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2210 bytes --]

 fs/qnx4/dir.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index 2a66844b7ff8..d60806efe090 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -20,12 +20,24 @@
  * depending on the status field in the last byte. The
  * first byte is where the name start either way, and a
  * zero means it's empty.
+ *
+ * Also, due to a bug in gcc, we don't want to use the
+ * real (differently sized) name arrays in the inode and
+ * link entries, but always the 'de_name[]' one in the
+ * fake struct entry.
+ *
+ * See
+ *
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c6
+ *
+ * for details - but basically gcc will take the size of
+ * the 'name' array from one of the used entries randomly.
  */
 union qnx4_directory_entry {
 	struct {
-		char de_name;
-		char de_pad[62];
-		char de_status;
+		const char de_name[48];
+		u8 de_pad[15];
+		u8 de_status;
 	};
 	struct qnx4_inode_entry inode;
 	struct qnx4_link_info link;
@@ -53,29 +65,26 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
 		ix = (ctx->pos >> QNX4_DIR_ENTRY_SIZE_BITS) % QNX4_INODES_PER_BLOCK;
 		for (; ix < QNX4_INODES_PER_BLOCK; ix++, ctx->pos += QNX4_DIR_ENTRY_SIZE) {
 			union qnx4_directory_entry *de;
-			const char *name;
 
 			offset = ix * QNX4_DIR_ENTRY_SIZE;
 			de = (union qnx4_directory_entry *) (bh->b_data + offset);
 
-			if (!de->de_name)
+			if (!de->de_name[0])
 				continue;
 			if (!(de->de_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
 				continue;
 			if (!(de->de_status & QNX4_FILE_LINK)) {
 				size = sizeof(de->inode.di_fname);
-				name = de->inode.di_fname;
 				ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1;
 			} else {
 				size = sizeof(de->link.dl_fname);
-				name = de->link.dl_fname;
 				ino = ( le32_to_cpu(de->link.dl_inode_blk) - 1 ) *
 					QNX4_INODES_PER_BLOCK +
 					de->link.dl_inode_ndx;
 			}
-			size = strnlen(name, size);
+			size = strnlen(de->de_name, size);
 			QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, name));
-			if (!dir_emit(ctx, name, size, ino, DT_UNKNOWN)) {
+			if (!dir_emit(ctx, de->de_name, size, ino, DT_UNKNOWN)) {
 				brelse(bh);
 				return 0;
 			}

Re: [PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[Arnd Bergmann]

On Mon, Sep 20, 2021 at 7:26 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Sep 20, 2021 at 5:12 AM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > +               /*
> > +                * work around gcc-11.x using the first field it observes
> > +                * to determing the actual length
> > +                * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> > +                */
> > +               char __empty[0];
> > +               char de_name[];
>
> Ugh. That looks _really_ hacky.
>
> It sounds like we can avoid the gcc bug if we just always use
> "de->de_name[]". Then we don't need to depend on magical behavior
> about one particular gcc version and a strange empty array in front of
> it.
>
> IOW, something like the attached simpler thing that just does that
> "always use de_name[]" and has a comment about why we don't do the
> natural thing

Yes, your patch is much nicer than my attempt. I checked it overnight
and it addresses all the randconfig issues I found.

> Also, just what version of gcc is the broken one? You say "gcc-11",
> but I certainly don't see it with _my_ version of gcc-11, so can we
> (just for that comment) document more precisely what version you have
> (or possibly what config you use to trigger it).

I'm using the gcc-11.1.0 that I uploaded to
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/

The gcc bug is still open and currently targets gcc-11.3. To make it
worse, this is definitely configuration dependent, and I do not see it
in an allmodconfig build, but only certain randconfig builds. I originally
had a simpler patch myself and also found that this did not fully
address all kernel configs.
gcc-10.x and early do not show the problem, I think the warning
was only introduced in 11.1.

https://pastebin.com/raw/fXmNiute is a .config for x86 that showed the
problem for me on 5.15-rc2 but not with your new patch.

       Arnd

Re: [PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[Anders Larsen]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

On Tuesday, 2021-09-21 10:18 Arnd Bergmann wrote:
> On Mon, Sep 20, 2021 at 7:26 PM Linus Torvalds 
<torvalds@linux-foundation.org> wrote:
> > It sounds like we can avoid the gcc bug if we just always use
> > "de->de_name[]". Then we don't need to depend on magical behavior
> > about one particular gcc version and a strange empty array in front of
> > it.
> >
> > IOW, something like the attached simpler thing that just does that
> > "always use de_name[]" and has a comment about why we don't do the
> > natural thing

well, the code in question actually does not use anything from struct 
qnx4_inode_entry except di_fname and di_status;
they are available at the same offsets in struct qnx4_link_info as well, so 
wouldn't it be even simpler to just always use the fields of the latter 
structure?

Like in the attached patch which replaces b7213ffa0e58?
($me feeling bad for reverting Linus' patch!)

That way, the compiler should never see any access to the (shorter) 
qnx4_inode_entry.di_fname

BTW, in the process I noticed that fs/qnx4/namei.c was missed by 663f4deca76 
back in 2013 and so is still calling strlen() on untrusted data; the second 
part of the patch takes care of that.

> > Also, just what version of gcc is the broken one? You say "gcc-11",
> > but I certainly don't see it with _my_ version of gcc-11, so can we
> > (just for that comment) document more precisely what version you have
> > (or possibly what config you use to trigger it).
> 
> I'm using the gcc-11.1.0 that I uploaded to
> https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/

I don't have that compiler version, so obviously I couldn't test if the patch 
solves the problem.

Cheers
Anders

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 4908 bytes --]

 fs/qnx4/dir.c   | 18 ++++++++----------
 fs/qnx4/namei.c | 34 +++++++++++++++-------------------
 2 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index a6ee23aadd28..45b0262c6fac 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -20,7 +20,6 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
 	struct inode *inode = file_inode(file);
 	unsigned int offset;
 	struct buffer_head *bh;
-	struct qnx4_inode_entry *de;
 	struct qnx4_link_info *le;
 	unsigned long blknum;
 	int ix, ino;
@@ -39,26 +38,25 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
 		ix = (ctx->pos >> QNX4_DIR_ENTRY_SIZE_BITS) % QNX4_INODES_PER_BLOCK;
 		for (; ix < QNX4_INODES_PER_BLOCK; ix++, ctx->pos += QNX4_DIR_ENTRY_SIZE) {
 			offset = ix * QNX4_DIR_ENTRY_SIZE;
-			de = (struct qnx4_inode_entry *) (bh->b_data + offset);
-			if (!de->di_fname[0])
+			le = (struct qnx4_link_info *) (bh->b_data + offset);
+			if (!le->dl_fname[0])
 				continue;
-			if (!(de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
+			if (!(le->dl_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
 				continue;
-			if (!(de->di_status & QNX4_FILE_LINK))
+			if (!(le->dl_status & QNX4_FILE_LINK))
 				size = QNX4_SHORT_NAME_MAX;
 			else
 				size = QNX4_NAME_MAX;
-			size = strnlen(de->di_fname, size);
-			QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, de->di_fname));
-			if (!(de->di_status & QNX4_FILE_LINK))
+			size = strnlen(le->dl_fname, size);
+			QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, le->dl_fname));
+			if (!(le->dl_status & QNX4_FILE_LINK))
 				ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1;
 			else {
-				le  = (struct qnx4_link_info*)de;
 				ino = ( le32_to_cpu(le->dl_inode_blk) - 1 ) *
 					QNX4_INODES_PER_BLOCK +
 					le->dl_inode_ndx;
 			}
-			if (!dir_emit(ctx, de->di_fname, size, ino, DT_UNKNOWN)) {
+			if (!dir_emit(ctx, le->dl_fname, size, ino, DT_UNKNOWN)) {
 				brelse(bh);
 				return 0;
 			}
diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..75ff330ce5e0 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -26,28 +26,26 @@
 static int qnx4_match(int len, const char *name,
 		      struct buffer_head *bh, unsigned long *offset)
 {
-	struct qnx4_inode_entry *de;
-	int namelen, thislen;
+	struct qnx4_link_info *le;
+	int namelen;
 
 	if (bh == NULL) {
 		printk(KERN_WARNING "qnx4: matching unassigned buffer !\n");
 		return 0;
 	}
-	de = (struct qnx4_inode_entry *) (bh->b_data + *offset);
+	le = (struct qnx4_link_info *) (bh->b_data + *offset);
 	*offset += QNX4_DIR_ENTRY_SIZE;
-	if ((de->di_status & QNX4_FILE_LINK) != 0) {
+	if ((le->dl_status & QNX4_FILE_LINK) != 0) {
 		namelen = QNX4_NAME_MAX;
 	} else {
 		namelen = QNX4_SHORT_NAME_MAX;
 	}
-	thislen = strlen( de->di_fname );
-	if ( thislen > namelen )
-		thislen = namelen;
-	if (len != thislen) {
+	namelen = strnlen( le->dl_fname, namelen );
+	if (len != namelen) {
 		return 0;
 	}
-	if (strncmp(name, de->di_fname, len) == 0) {
-		if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) != 0) {
+	if (strncmp(name, le->dl_fname, len) == 0) {
+		if ((le->dl_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) != 0) {
 			return 1;
 		}
 	}
@@ -55,7 +53,7 @@ static int qnx4_match(int len, const char *name,
 }
 
 static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
-	   const char *name, struct qnx4_inode_entry **res_dir, int *ino)
+	   const char *name, struct qnx4_link_info **res_dir, int *ino)
 {
 	unsigned long block, offset, blkofs;
 	struct buffer_head *bh;
@@ -73,7 +71,7 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
 				continue;
 			}
 		}
-		*res_dir = (struct qnx4_inode_entry *) (bh->b_data + offset);
+		*res_dir = (struct qnx4_link_info *) (bh->b_data + offset);
 		if (qnx4_match(len, name, bh, &offset)) {
 			*ino = block * QNX4_INODES_PER_BLOCK +
 			    (offset / QNX4_DIR_ENTRY_SIZE) - 1;
@@ -95,21 +93,19 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
 struct dentry * qnx4_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
 {
 	int ino;
-	struct qnx4_inode_entry *de;
-	struct qnx4_link_info *lnk;
+	struct qnx4_link_info *le;
 	struct buffer_head *bh;
 	const char *name = dentry->d_name.name;
 	int len = dentry->d_name.len;
 	struct inode *foundinode = NULL;
 
-	if (!(bh = qnx4_find_entry(len, dir, name, &de, &ino)))
+	if (!(bh = qnx4_find_entry(len, dir, name, &le, &ino)))
 		goto out;
 	/* The entry is linked, let's get the real info */
-	if ((de->di_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) {
-		lnk = (struct qnx4_link_info *) de;
-		ino = (le32_to_cpu(lnk->dl_inode_blk) - 1) *
+	if ((le->dl_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) {
+		ino = (le32_to_cpu(le->dl_inode_blk) - 1) *
                     QNX4_INODES_PER_BLOCK +
-		    lnk->dl_inode_ndx;
+		    le->dl_inode_ndx;
 	}
 	brelse(bh);
 

Re: [PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[Linus Torvalds]

On Tue, Sep 21, 2021 at 8:15 AM Anders Larsen <al@alarsen.net> wrote:
>
> they are available at the same offsets in struct qnx4_link_info as well, so
> wouldn't it be even simpler to just always use the fields of the latter
> structure?

I'd rather use that third "clearly neither" structure member, just to
clarify what is going on.

Yes, we could just always use the bigger structure, but then we'd
actually access a "link entry" even when it really isn't a link entry.

Now we can have that bogus entry and the big comment that says exactly
why we use the bogus entry, and it's clear that the "name" we use is
not necessarily a link entry or an inode entry, it's that special
union with a big comment about a gcc bug above it..

Anyway, I committed my patch that Arnd had tested, with a slightly
expanded comment. I'm sure yours would have compiled cleanly too.

           Linus

Re: [PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[Anders Larsen]

On Tuesday, 2021-09-21 17:40 Linus Torvalds wrote:
> I'd rather use that third "clearly neither" structure member, just to
> clarify what is going on.

I'm perfectly fine with that.

> Anyway, I committed my patch that Arnd had tested, with a slightly
> expanded comment. I'm sure yours would have compiled cleanly too.

When it turns up in the tree, I'll cook up a fix for the strlen() issue in
fs/qnx4/namei.c following your scheme.

Cheers
Anders

Re: [PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[Linus Torvalds]

On Tue, Sep 21, 2021 at 8:49 AM Anders Larsen <al@alarsen.net> wrote:
>
> When it turns up in the tree, I'll cook up a fix for the strlen() issue in
> fs/qnx4/namei.c following your scheme.

Now pushed out.

            Linus

Re: [PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

[Arnd Bergmann]

On Tue, Sep 21, 2021 at 5:16 PM Anders Larsen <al@alarsen.net> wrote:
> On Tuesday, 2021-09-21 10:18 Arnd Bergmann wrote:
> >
> > I'm using the gcc-11.1.0 that I uploaded to
> > https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/
>
> I don't have that compiler version, so obviously I couldn't test if the patch
> solves the problem.

To clarify, those cross-compilers are meant to be usable on any
x86/arm64/ppc64le distro from the past 5 years, and should allow
you to build kernels (but no user space) for all supported target
architectures using all supported gcc versions.

I'm not asking you to waste time reproducing the problem (it's already
solved now), just saying you probably could ;-)

       Arnd