Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200430213101.135134-1-arnd@arndb.de> <20200430213101.135134-10-arnd@arndb.de>
 <20200430215450.anfwm4zikvhy2bt5@pali>
In-Reply-To: <20200430215450.anfwm4zikvhy2bt5@pali>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 1 May 2020 22:30:27 +0200
Message-ID: <CAK8P3a0_BuKDK7X8dTHyx7t++CGjy3jhDVvL=Jo5=kcUxmtobg@mail.gmail.com>
Subject: Re: [PATCH 09/15] udf: avoid gcc-10 zero-length-bounds warnings
To:     =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jan Kara <jack@suse.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Steven J. Magnani" <steve@digidescorp.com>,
        Al Viro <viro@zeniv.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Apr 30, 2020 at 11:54 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Thursday 30 April 2020 23:30:51 Arnd Bergmann wrote:
> > gcc-10 warns about writes to the empty freeSpaceTable[] array, with
> > many instances like:
> >
> > fs/udf/balloc.c: In function 'udf_bitmap_new_block':
> > fs/udf/balloc.c:101:36: error: array subscript 65535 is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Werror=zero-length-bounds]
> >   101 |  le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
> >       |                ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > In file included from fs/udf/udfdecl.h:7,
> >                  from fs/udf/balloc.c:22:
> > fs/udf/ecma_167.h:363:11: note: while referencing 'freeSpaceTable'
> >   363 |  __le32   freeSpaceTable[0];
> >       |           ^~~~~~~~~~~~~~
>
> Hi Arnd! This looks like a false-positive warning.

Right, sorry for not making that clearer in the changelog.

> > These can all be avoided by using a flexible array member instead.
> >
> > Another warning is a bit more obscure:
> >
> > fs/udf/super.c: In function 'udf_count_free':
> > fs/udf/super.c:2521:26: warning: array subscript '(<unknown>) + 4294967295' is outside the bounds of an interior zero-length array '__le32[0]' {aka 'unsigned int[0]'} [-Wzero-length-bounds]
> >  2521 |      lvid->freeSpaceTable[part]);
> >
> > Work around this one by changing the array access to equivalent
> > pointer arithmetic, as there cannot be multiple flexible-array
> > members in a single struct.
>

> > @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
> >       uint8_t                 logicalVolContentsUse[32];
> >       __le32                  numOfPartitions;
> >       __le32                  lengthOfImpUse;
> > -     __le32                  freeSpaceTable[0];
> >       __le32                  sizeTable[0];
> >       uint8_t                 impUse[0];
> > +     __le32                  freeSpaceTable[];
>
> Please do not change order of members in these structures. Order is
> strictly defined by ECMA 167 standard and changing them you would just
> confuse reader. In LVID is free space table before size table.

Ok

> If you do not like GNU C extension for zero-length arrays then just
> replace it by standard C99 flexible arrays. I think that there is no
> reason to not use standard C99 language constructions, just nobody had
> motivation or time to change (working) code.

No, the problem is that only the last member can be a flexible array,
so when impUse[] is the last member, freeSpaceTable has to be a zero
length array.

[]> Also this file is semi-synchronized with udftools project in which I
> already replaced all GNU C zero-length arrays by C99 flexible arrays.
>
> You can take inspiration what I did with logicalVolIntegrityDesc:
> https://github.com/pali/udftools/commit/f851d84478ce881d516a76018745fa163f803880#diff-1e1a5b89f620d380f22b973f9449aeaeL381-R384

Right, this is likely the best workaround.

> Anyway, if you have a better idea what to do with such on-disk structure
> and how to represent it in C struct syntax, let me know as it could be
> updated also in udftools project.

The trick I used for impUse[] would also work for freeSpaceTable[] to avoid
the gcc warning, it's still not great, but maybe you like this better:

arnd@threadripper:~/arm-soc$ git diff
diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
index 02f03fadb75b..666d022eb00b 100644
--- a/fs/udf/balloc.c
+++ b/fs/udf/balloc.c
@@ -98,7 +98,7 @@ static void udf_add_free_space(struct super_block
*sb, u16 partition, u32 cnt)
                return;

        lvid = (struct logicalVolIntegrityDesc *)sbi->s_lvid_bh->b_data;
-       le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
+       le32_add_cpu(lvid->freeSpaceTable + partition, cnt);
        udf_updated_lvid(sb);
 }

diff --git a/fs/udf/ecma_167.h b/fs/udf/ecma_167.h
index 14ffe27342bc..215d97d7edc4 100644
--- a/fs/udf/ecma_167.h
+++ b/fs/udf/ecma_167.h
@@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
        uint8_t                 logicalVolContentsUse[32];
        __le32                  numOfPartitions;
        __le32                  lengthOfImpUse;
        __le32                  freeSpaceTable[0];
        __le32                  sizeTable[0];
-       uint8_t                 impUse[0];
+       uint8_t                 impUse[];
 } __packed;

 /* Integrity Type (ECMA 167r3 3/10.10.3) */
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 379867888c36..a1fc51c2261e 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2517,8 +2517,8 @@ static unsigned int udf_count_free(struct super_block *sb)
                        (struct logicalVolIntegrityDesc *)
                        sbi->s_lvid_bh->b_data;
                if (le32_to_cpu(lvid->numOfPartitions) > part) {
-                       accum = le32_to_cpu(
-                                       lvid->freeSpaceTable[part]);
+                       accum = le32_to_cpup(
+                                       (lvid->freeSpaceTable + part));
                        if (accum == 0xFFFFFFFF)
                                accum = 0;
                }


This version could easily be backported to stable kernels to let them be
compiled with gcc-10, and then synchronizing with the udftools version of
the header needs additional changes on top, which do not need to be
backported.

       Arnd