Message-ID: <5c4040b2-fdbb-1897-e6a8-1c990226586c@colorfullife.com>
Date:   Mon, 29 May 2023 18:16:29 +0200
From:   Manfred Spraul <manfred@...orfullife.com>
To:     Li Wang <liwang@...hat.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Naresh Kamboju <naresh.kamboju@...aro.org>,
        open list <linux-kernel@...r.kernel.org>,
        LTP List <ltp@...ts.linux.it>, lkft-triage@...ts.linaro.org,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Davidlohr Bueso <dbueso@...e.de>,
        Ard Biesheuvel <ardb@...nel.org>,
        Dan Carpenter <dan.carpenter@...aro.org>,
        Anders Roxell <anders.roxell@...aro.org>,
        chrubis <chrubis@...e.cz>, Petr Vorel <pvorel@...e.cz>,
        Martin Doucha <mdoucha@...e.cz>
Subject: Re: LTP: shmget02 fails on compat mode - 64-bit kernel and 32-bit
 userspace

Hi Li,

On 5/20/23 05:58, Li Wang wrote:
> Hi Manfred,
>
> On Sat, May 20, 2023 at 1:55 AM Manfred Spraul 
> <manfred@...orfullife.com> wrote:
>
>     Hi all,
>
>     On 5/19/23 12:57, Arnd Bergmann wrote:
>     > On Fri, May 19, 2023, at 11:17, Naresh Kamboju wrote:
>     >> LTP running on compat mode where the tests run on
>     >> 64-bit kernel and 32-bit userspace are noticed on a list of
>     failures.
>     >>
>     >> What would be the best way to handle this rare combination of
>     failures ?
>     >>
>     >> * arm64: juno-r2-compat, qemu_arm64-compat and qemu_x86_64-compat
>     >>      - shmget02
>     >>
>     >> Reported-by: Linux Kernel Functional Testing <lkft@...aro.org>
>     >>
>     >> tst_hugepage.c:83: TINFO: 0 hugepage(s) reserved
>     >> tst_test.c:1558: TINFO: Timeout per run is 0h 02m 30s
>     >> tst_kconfig.c:87: TINFO: Parsing kernel config '/proc/config.gz'
>     >> shmget02.c:95: TPASS: shmget(1644199826, 2048, 1024) : ENOENT (2)
>     >> shmget02.c:95: TPASS: shmget(1627422610, 2048, 1536) : EEXIST (17)
>     >> <4>[   84.678150] __vm_enough_memory: pid: 513, comm: shmget02, not
>     >> enough memory for the allocation
>     >> shmget02.c:95: TPASS: shmget(1644199826, 0, 1536) : EINVAL (22)
>     >> shmget02.c:95: TFAIL: shmget(1644199826, 4278190080, 1536) expected
>     >> EINVAL: ENOMEM (12)
>     > Adding Liam Howlett, Davidlohr Bueso and Manfred Spraul to Cc, they
>     > have worked on the shm code in the past few years.
>     >
>     > This is the line
>     >
>     >       {&shmkey1, SHMMAX + 1, IPC_CREAT | IPC_EXCL, 0, 0, EINVAL},
>     >
>     > from
>     >
>     >
>     https://github.com/linux-test-project/ltp/blob/04e8f2f4fd949/testcases/kernel/syscalls/ipc/shmget/shmget02.c#LL59C1-L59C61
>     >
>     > right?
>     >
>     > I think this is a result of SHMMAX being defined as
>     > #define SHMMAX (ULONG_MAX - (1UL << 24)), so the kernel would
>     > likely use a large 64-bit value here, while the 32-bit user
>     > space uses a much smaller limit.
>     >
>     > The expected return code likely comes from
>     >
>     > static int newseg(struct ipc_namespace *ns, struct ipc_params
>     *params)
>     > {
>     > ...
>     >          if (size < SHMMIN || size > ns->shm_ctlmax)
>     >                  return -EINVAL;
>     >
>     > but if ns->shm_ctlmax is probably set to the 64-bit value here.
>     > It would then trigger the accounting limit in __shmem_file_setup():
>     >
>     >          if (shmem_acct_size(flags, size))
>     >                  return ERR_PTR(-ENOMEM);
>     >
>     > My feeling is that the kernel in this case works as expected,
>     > and I wouldn't see this as a bug. On the other hand, this
>     > can probably be addressed in the kernel by adding a check for
>     > compat tasks like
>     >
>     > --- a/ipc/shm.c
>     > +++ b/ipc/shm.c
>     > @@ -714,7 +714,8 @@ static int newseg(struct ipc_namespace *ns,
>     struct ipc_params *params)
>     >          char name[13];
>     >          vm_flags_t acctflag = 0;
>     >
>     > -       if (size < SHMMIN || size > ns->shm_ctlmax)
>     > +       if (size < SHMMIN || size > ns->shm_ctlmax ||
>     > +          in_compat_syscall() && size > COMPAT_SHMMAX))
>     >                  return -EINVAL;
>     >
>     >          if (numpages << PAGE_SHIFT < size)
>     >
>     I would consider this as ugly: ns->shm_ctlmax can be configured by
>     writing to /proc/sys/kernel/shmmax.
>
>     You can break the test case on 64-bit as well, just by writing
>     SHMMAX+1
>     to /proc/sys/kernel/shmmax
>
>     Thus I think the test case is flawed:
>
>     It is testing the overflow behavior for a configurable value by
>     testing
>     with default+1. But sometimes the actual value is not the default.
>
>     Are the tests running as root?
>
>
> Yes.
>
>
>     What about intentionally setting the value to something useful?
>
>
>
> This suggest sounds reasonable, but I have a question:
> is there any upper limit for setting the /proc/sys/kernel/shmmax?
>
The real limit is 0x7fffffffffffffff. Even if the value of shmmax is 
higher, shmget() fails.

I think this is due to MAX_LFS_FILESIZE in __shmem_file_setup(). I 
didn't attach a debugger, thus I cannot rule out that there is another 
check that also rejects >= 0x800<...>0

The maximum useful size is probably even lower, shmat() would fail since 
the virtual memory size is even smaller.

>
> The test seems to try to test the bounder and as a
> corner case for covering that scenario.
But then just reduce shmmax:

- test that shmget(5000) works

- echo "4999" > /proc/sys/kernel/shmmax

- test that shmget(5000) fails

- echo "5000" > /proc/sys/kernel/shmmax

- test that shmget(5000) works again.



>
>     tmp=$(cat /proc/sys/kernel/shmmax)
>
>     echo "1234" > /proc/sys/kernel/shmmax
>
>     semget() based on {&shmkey1, 1234 + 1, IPC_CREAT | IPC_EXCL, 0, 0,
>     EINVAL},
>     echo $tmp >/proc/sys/kernel/shmmax
>
>     Or, alternatively: read /proc/sys/kernel/shmmax, and skip the test if
>     the value is larger than ULONG_MAX-1.
>
>     --
>             Manfred
>
>
>
>
> -- 
> Regards,
> Li Wang

Powered by blists - more mailing lists