Re: [LTP] [PATCH 3/5] syscalls/mmap13: Rewrite the test using new API

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Avinesh Kumar <akumar@suse.de> writes:

> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> ---
>  testcases/kernel/syscalls/mmap/mmap13.c | 174 +++++++++---------------
>  1 file changed, 65 insertions(+), 109 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/mmap/mmap13.c b/testcases/kernel/syscalls/mmap/mmap13.c
> index c5a2058e9..5e18f24fe 100644
> --- a/testcases/kernel/syscalls/mmap/mmap13.c
> +++ b/testcases/kernel/syscalls/mmap/mmap13.c
> @@ -1,142 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) 2013 FNST, DAN LI <li.dan@cn.fujitsu.com>
> + * Copyright (c) 2023 SUSE LLC Avinesh Kumar <avinesh.kumar@suse.com>
> + */
> +
> +/*\
> + * [Description]
>   *
> - * This program is free software;  you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> + * Verify that, mmap() call succeeds to create a file mapping with length
> + * argument greater than the file size but any attempt to reference the
> + * memory region which does not correspond to the file causes SIGBUS signal.
>   *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY;  without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> - * the GNU General Public License for more details.
>   *
> - * You should have received a copy of the GNU General Public License
> - * along with this program;  if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - */
> -
> -/*
> - * Test Description:
> - *  Verify error signal SIGBUS.
> - *  "Attempted access to a portion of the buffer that does not correspond
> - *   to the file."
> + *                  mmap(0, 8192, prot, MAP_FILE | MAP_SHARED, fd, 0);
>   *
> - * Expected Result:
> - *  mmap() should succeed returning the address of the mapped region,
> - *  and an attempt to access the memory which does not correspond to the file
> - *  should rise the signal SIGBUS.
> + *  byte offset:    0        2047 2048    4095 4096                8191 8192
> + *                  +------------+------------+------------------------+
> + *  Memory          |            |  remainder |                        |
> + *  region          |            | of page(0s)|                        |
> + *                  +------------+------------+------------------------+
> + *                  |<---------->|<---------->|<---------------------->|<--------------->
> + *                  | accessible,| accessible,        references            references
> + *                  |   mapped   | not mapped        yield SIGBUS          yield SIGSEGV
> + *                  |  to file   |  to file
> + *                  |            |
> + *                  +------------+
> + *  Mapped file     |            |
> + * (2048 bytes)     |            |
> + *                  +------------+
> + *  file offset:    0         2047

Nice diagram! The byte offsets are only valid for 4Kb pages though.

>   */
> -#include <stdio.h>
> +
>  #include <stdlib.h>
> -#include <sys/types.h>
> -#include <errno.h>
> -#include <unistd.h>
> -#include <fcntl.h>
> -#include <string.h>
> -#include <signal.h>
> -#include <sys/stat.h>
> -#include <sys/mman.h>
>  #include <setjmp.h>
> -
> -#include "test.h"
> +#include "tst_test.h"
>  
>  #define TEMPFILE	"mmapfile"
> -
> -char *TCID = "mmap13";
> -int TST_TOTAL = 1;
> -
>  static size_t page_sz;
>  static char *addr;
> -static int fildes;
> +static int fd;
>  static volatile sig_atomic_t pass;
>  static sigjmp_buf env;
>  
> -static void setup(void);
> -static void cleanup(void);
> -static void sig_handler(int sig);
> -
> -int main(int argc, char *argv[])
> +static void sig_handler(int sig)
>  {
> -	int lc;
> -	char *ch;
> -
> -	tst_parse_opts(argc, argv, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> -
> -		addr = mmap(NULL, page_sz * 2, PROT_READ | PROT_WRITE,
> -			    MAP_FILE | MAP_SHARED, fildes, 0);
> -
> -		if (addr == MAP_FAILED) {
> -			tst_resm(TFAIL | TERRNO, "mmap() failed on %s",
> -				 TEMPFILE);
> -			continue;
> -		}
> -
> -		if (sigsetjmp(env, 1) == 0) {
> -			ch = addr + page_sz + 1;
> -			*ch = 0;
> -		}
> -
> -		if (pass)
> -			tst_resm(TPASS, "Got SIGBUS "
> -					"as expected");
> -		else
> -			tst_resm(TFAIL, "Invalid access not "
> -						"rise SIGBUS");
> -
> -		if (munmap(addr, page_sz * 2) != 0)
> -			tst_brkm(TFAIL | TERRNO, cleanup,
> -				 "failed to unmap the mmapped pages");
> -
> -		pass = 0;
> -	}
> -
> -	cleanup();
> -	tst_exit();
> +	if (sig == SIGBUS) {
> +		pass = 1;
> +		siglongjmp(env, 1);
> +	} else
> +		tst_brk(TBROK, "received an unexpected signal");

To be safe we should exit the signal handler ASAP. It's better to save
the signal details and check them after returning.

The rest looks good, but setting to changes requested.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp