gcc-8: mm/swapfile.c:863:40: error: array subscript 1 is above array bounds of 'struct list_head[1]' [-Werror=array-bounds]

gcc-8: mm/swapfile.c:863:40: error: array subscript 1 is above array bounds of 'struct list_head[1]' [-Werror=array-bounds]

[Naresh Kamboju]

The x86 builds failed with gcc-8 due to following build warnings / errors on
Linux next-20240802 to next-20240819.

x86_64 defconfig gcc-8 build failed
x86_64 defconfig gcc-13 build pass

First seen on the next-20240802 tag.

  GOOD: next-20240730
  BAD:  next-20240802

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>

Build errors:
--------
mm/swapfile.c: In function 'scan_swap_map_slots.constprop':
mm/swapfile.c:863:40: error: array subscript 1 is above array bounds
of 'struct list_head[1]' [-Werror=array-bounds]
   while (!list_empty(&si->frag_clusters[o])) {
                       ~~~~~~~~~~~~~~~~~^~~
mm/swapfile.c:872:43: error: array subscript 1 is above array bounds
of 'struct list_head[1]' [-Werror=array-bounds]
   while (!list_empty(&si->nonfull_clusters[o])) {
                       ~~~~~~~~~~~~~~~~~~~~^~~
In file included from include/linux/list.h:5,
                 from include/linux/wait.h:7,
                 from include/linux/wait_bit.h:8,
                 from include/linux/fs.h:6,
                 from include/linux/highmem.h:5,
                 from include/linux/bvec.h:10,
                 from include/linux/blk_types.h:10,
                 from include/linux/blkdev.h:9,
                 from mm/swapfile.c:9:
include/linux/list.h:612:18: error: array subscript 1 is above array
bounds of 'struct list_head[1]' [-Werror=array-bounds]
  list_entry((ptr)->next, type, member)
                  ^~
include/linux/container_of.h:19:26: note: in definition of macro 'container_of'
  void *__mptr = (void *)(ptr);     \
                          ^~~
include/linux/list.h:612:2: note: in expansion of macro 'list_entry'
  list_entry((ptr)->next, type, member)
  ^~~~~~~~~~
mm/swapfile.c:873:9: note: in expansion of macro 'list_first_entry'
    ci = list_first_entry(&si->nonfull_clusters[o],
         ^~~~~~~~~~~~~~~~
include/linux/list.h:612:18: error: array subscript 1 is above array
bounds of 'struct list_head[1]' [-Werror=array-bounds]
  list_entry((ptr)->next, type, member)
                  ^~
include/linux/container_of.h:19:26: note: in definition of macro 'container_of'
  void *__mptr = (void *)(ptr);     \
                          ^~~
include/linux/list.h:612:2: note: in expansion of macro 'list_entry'
  list_entry((ptr)->next, type, member)
  ^~~~~~~~~~
mm/swapfile.c:864:9: note: in expansion of macro 'list_first_entry'
    ci = list_first_entry(&si->frag_clusters[o],
         ^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Steps to reproduce:
-------
# tuxmake --runtime podman --target-arch x86_64 --toolchain gcc-8
--kconfig x86_64_defconfig

Build log link,
------
 - https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20240819/testrun/24918869/suite/build/test/gcc-8-x86_64_defconfig/log

metadata:
-----
  git describe: next-20240802
  git repo: https://gitlab.com/Linaro/lkft/mirrors/next/linux-next
  git short_log: f524a5e4dfb7 ("Add linux-next specific files for 20240802")
  config: https://storage.tuxsuite.com/public/linaro/lkft/builds/2k6wLr4UFODrd3snUDTPPevXpPY/config
  download_url:
https://storage.tuxsuite.com/public/linaro/lkft/builds/2k6wLr4UFODrd3snUDTPPevXpPY/
  toolchain: gcc-8
  arch: x86_64

--
Linaro LKFT
https://lkft.linaro.org

Re: gcc-8: mm/swapfile.c:863:40: error: array subscript 1 is above array bounds of 'struct list_head[1]' [-Werror=array-bounds]

[Naresh Kamboju]

On Mon, 19 Aug 2024 at 13:58, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>
> The x86 builds failed with gcc-8 due to following build warnings / errors on
> Linux next-20240802 to next-20240819.
>
> x86_64 defconfig gcc-8 build failed
> x86_64 defconfig gcc-13 build pass
>
> First seen on the next-20240802 tag.
>
>   GOOD: next-20240730
>   BAD:  next-20240802

Anders bisected this and found the first bad commit id as,
aded4352f648 ("mm: swap: separate SSD allocation from
scan_swap_map_slots()") first faulty one

- Naresh

Re: gcc-8: mm/swapfile.c:863:40: error: array subscript 1 is above array bounds of 'struct list_head[1]' [-Werror=array-bounds]

[Kairui Song]

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

On Mon, Aug 19, 2024 at 6:16 PM Naresh Kamboju
<naresh.kamboju@linaro.org> wrote:
>
> On Mon, 19 Aug 2024 at 13:58, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
> >
> > The x86 builds failed with gcc-8 due to following build warnings / errors on
> > Linux next-20240802 to next-20240819.
> >
> > x86_64 defconfig gcc-8 build failed
> > x86_64 defconfig gcc-13 build pass
> >
> > First seen on the next-20240802 tag.
> >
> >   GOOD: next-20240730
> >   BAD:  next-20240802
>
> Anders bisected this and found the first bad commit id as,
> aded4352f648 ("mm: swap: separate SSD allocation from
> scan_swap_map_slots()") first faulty one
>
> - Naresh
>

Hi Naresh,

Thanks for the report, the problem will occur when CONFIG_THP_SWAP is
disabled. Can you try the following patch? I can confirm it's fixed
with my test.

[-- Attachment #2: 0001-mm-swap-fix-array-bounds-error-with-CONFIG_THP_SWAP-.patch --]
[-- Type: application/octet-stream, Size: 880 bytes --]

From 89f0aafa03ad5878e8902e07e12ec0c020dba2df Mon Sep 17 00:00:00 2001
From: Kairui Song <kasong@tencent.com>
Date: Mon, 19 Aug 2024 19:31:55 +0800
Subject: [PATCH] mm: swap: fix array-bounds error with CONFIG_THP_SWAP=n

Signed-off-by: Kairui Song <kasong@tencent.com>
---
 mm/swapfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index d9cf31b04db3..16ec6b7df198 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -836,7 +836,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
 		goto done;
 
 	/* Order 0 stealing from higher order */
-	for (int o = 1; o < PMD_ORDER; o++) {
+	for (int o = 1; o < SWAP_NR_ORDERS; o++) {
 		/*
 		 * Clusters here have at least one usable slots and can't fail order 0
 		 * allocation, but reclaim may drop si->lock and race with another user.
-- 
2.45.2

Re: gcc-8: mm/swapfile.c:863:40: error: array subscript 1 is above array bounds of 'struct list_head[1]' [-Werror=array-bounds]

[Naresh Kamboju]

On Mon, 19 Aug 2024 at 17:14, Kairui Song <ryncsn@gmail.com> wrote:
>
> On Mon, Aug 19, 2024 at 6:16 PM Naresh Kamboju
> <naresh.kamboju@linaro.org> wrote:
> >
> > On Mon, 19 Aug 2024 at 13:58, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
> > >
> > > The x86 builds failed with gcc-8 due to following build warnings / errors on
> > > Linux next-20240802 to next-20240819.
> > >
> > > x86_64 defconfig gcc-8 build failed
> > > x86_64 defconfig gcc-13 build pass
> > >
> > > First seen on the next-20240802 tag.
> > >
> > >   GOOD: next-20240730
> > >   BAD:  next-20240802
> >
> > Anders bisected this and found the first bad commit id as,
> > aded4352f648 ("mm: swap: separate SSD allocation from
> > scan_swap_map_slots()") first faulty one
> >
> > - Naresh
> >
>
> Hi Naresh,
>
> Thanks for the report, the problem will occur when CONFIG_THP_SWAP is
> disabled. Can you try the following patch? I can confirm it's fixed
> with my test.

Thanks for the patch.
I have tested your patch and it fixes the reported problem.

Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>

- Naresh

Re: gcc-8: mm/swapfile.c:863:40: error: array subscript 1 is above array bounds of 'struct list_head[1]' [-Werror=array-bounds]

[Andrew Morton]

On Mon, 19 Aug 2024 19:44:25 +0800 Kairui Song <ryncsn@gmail.com> wrote:

> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -836,7 +836,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
>  		goto done;
>  
>  	/* Order 0 stealing from higher order */
> -	for (int o = 1; o < PMD_ORDER; o++) {
> +	for (int o = 1; o < SWAP_NR_ORDERS; o++) {
>  		/*
>  		 * Clusters here have at least one usable slots and can't fail order 0
>  		 * allocation, but reclaim may drop si->lock and race with another user.

OK, I got that landed in the right place, but...

The definition of `o' within the for statement isn't typical kernel
style - I'm surprised we didn't get a warning for this - maybe things
have changed when I wasn't looking.

Also, this code makes no attempt to honor our "The preferred limit on
the length of a single line is 80 columns" objective.  There's just no
reason for comment blocks to violate this.

So Chris, please attend to such things when preparing v6, which I
assume is in the works.

Re: gcc-8: mm/swapfile.c:863:40: error: array subscript 1 is above array bounds of 'struct list_head[1]' [-Werror=array-bounds]

[Chris Li]

On Mon, Aug 19, 2024 at 10:05 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Mon, 19 Aug 2024 19:44:25 +0800 Kairui Song <ryncsn@gmail.com> wrote:
>
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -836,7 +836,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
> >               goto done;
> >
> >       /* Order 0 stealing from higher order */
> > -     for (int o = 1; o < PMD_ORDER; o++) {
> > +     for (int o = 1; o < SWAP_NR_ORDERS; o++) {
> >               /*
> >                * Clusters here have at least one usable slots and can't fail order 0
> >                * allocation, but reclaim may drop si->lock and race with another user.
>
> OK, I got that landed in the right place, but...
>
> The definition of `o' within the for statement isn't typical kernel
> style - I'm surprised we didn't get a warning for this - maybe things
> have changed when I wasn't looking.

Noted.

I did use the checkpatch.pl and fixed all the warnings before I sent
the patch out.
The checkpatch.pl script did not complain about this. Sure I can stay
away from it.
BTW, I did  a search on the kernel tree:
$ rg 'for \(int' | wc -l
970
$
It seems pretty common in the kernel tree now.

>
> Also, this code makes no attempt to honor our "The preferred limit on
> the length of a single line is 80 columns" objective.  There's just no
> reason for comment blocks to violate this.

I was wondering why the checkpatch.pl did not catch this, is there any
config for checkpatch.pl I should apply?

I typically invoke:

./scripts/checkpatch.pl -g HEAD

Let me know if there is a better way to invoke checkpatch.pl to give
more warnings.

Chris

Re: gcc-8: mm/swapfile.c:863:40: error: array subscript 1 is above array bounds of 'struct list_head[1]' [-Werror=array-bounds]

[Kairui Song]

On Tue, Aug 20, 2024 at 4:51 PM Chris Li <chrisl@kernel.org> wrote:
>
> On Mon, Aug 19, 2024 at 10:05 PM Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> > On Mon, 19 Aug 2024 19:44:25 +0800 Kairui Song <ryncsn@gmail.com> wrote:
> >
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -836,7 +836,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
> > >               goto done;
> > >
> > >       /* Order 0 stealing from higher order */
> > > -     for (int o = 1; o < PMD_ORDER; o++) {
> > > +     for (int o = 1; o < SWAP_NR_ORDERS; o++) {
> > >               /*
> > >                * Clusters here have at least one usable slots and can't fail order 0
> > >                * allocation, but reclaim may drop si->lock and race with another user.
> >
> > OK, I got that landed in the right place, but...
> >
> > The definition of `o' within the for statement isn't typical kernel
> > style - I'm surprised we didn't get a warning for this - maybe things
> > have changed when I wasn't looking.
>
> Noted.
>
> I did use the checkpatch.pl and fixed all the warnings before I sent
> the patch out.
> The checkpatch.pl script did not complain about this. Sure I can stay
> away from it.
> BTW, I did  a search on the kernel tree:
> $ rg 'for \(int' | wc -l
> 970
> $
> It seems pretty common in the kernel tree now.

Might be off topic from the issue...

I believe this issue it's not an upstream problem nowadays after
e8c07082a810 ("Kbuild: move to -std=gnu11"), I did notice a GCC error
after backporting these commits to an older kernel which still used
c89, but for upstream this should be OK?

>
> >
> > Also, this code makes no attempt to honor our "The preferred limit on
> > the length of a single line is 80 columns" objective.  There's just no
> > reason for comment blocks to violate this.
>
> I was wondering why the checkpatch.pl did not catch this, is there any
> config for checkpatch.pl I should apply?
>
> I typically invoke:
>
> ./scripts/checkpatch.pl -g HEAD

I found checkpatch.pl stopped checking for 80 columns limit after
commit bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column
warning") 4 years ago. But the 80 column limit seems still preferred?