Re: [LTP] [PATCH] fanotify24: Verify expected count/offset info in pre content events

[Petr Vorel <pvorel@suse.cz>]

Hi Amir, all,

> To test fix commit 28bba2c2935e2 ("fsnotify: Pass correct offset to
> fsnotify_mmap_perm()"), diversify the offsets and count used for mmap()
> write() and read() and verify that the FAN_PRE_ACCESS events report the
> expected count/offset.

> For the FAN_PRE_ACCESS events generated by execve(), we cannot
> anticipate the exact ranges, so we set 0 count to skip this verification.

> Also add prints of path of the fd passed with the event (not verified
> against expected path).

> Make sure that we take the expected error value for an operation
> (e.g. read) from a matching event type (e.g. FAN_PRE_ACCESS).

Thanks for the update.  LGTM, but it'd be great if some of kernel developers
also had look into it. Few minor notes below.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

I also restarted failing jobs in github actions:
https://github.com/linux-test-project/ltp/actions/runs/18599812482

Unfortunately due patchwork API limitation, the failing jobs aren't replaced
with successful ones, instead the fixes are appended:
https://patchwork.ozlabs.org/project/ltp/patch/20251017161639.2088158-1-amir73il@gmail.com/

> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  .../kernel/syscalls/fanotify/fanotify24.c     | 167 +++++++++++-------
>  1 file changed, 107 insertions(+), 60 deletions(-)

> diff --git a/testcases/kernel/syscalls/fanotify/fanotify24.c b/testcases/kernel/syscalls/fanotify/fanotify24.c
> index 27f0663ce..8f2dee55b 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify24.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify24.c
> @@ -9,6 +9,8 @@
>  /*\
>   * - Test fanotify pre-content events
>   * - Test respond to permission/pre-content events with cutsom error code
> + * - Test count/offset info bug that was fixed by commit
> + *   28bba2c2935e2 "fsnotify: Pass correct offset to fsnotify_mmap_perm()"
>   */

>  #define _GNU_SOURCE
> @@ -44,6 +46,8 @@
>  #define FILE_EXEC_PATH MOUNT_PATH"/"TEST_APP

>  static char fname[BUF_SIZE];
> +static char symlnk[BUF_SIZE];
> +static char fdpath[BUF_SIZE];
>  static char buf[BUF_SIZE];
>  static volatile int fd_notify;
>  static size_t page_sz;
> @@ -55,6 +59,8 @@ static char event_buf[EVENT_BUF_LEN];
>  struct event {
>  	unsigned long long mask;
>  	unsigned int response;
> +	unsigned long pgcount;
> +	unsigned long pgoff;
>  };

>  static struct tcase {
> @@ -68,11 +74,11 @@ static struct tcase {
>  		INIT_FANOTIFY_MARK_TYPE(INODE),
>  		FAN_OPEN_PERM | FAN_PRE_ACCESS,
>  		{
> -			{FAN_OPEN_PERM, FAN_ALLOW},
> -			{FAN_PRE_ACCESS, FAN_ALLOW},
> -			{FAN_PRE_ACCESS, FAN_ALLOW},
> -			{FAN_PRE_ACCESS, FAN_DENY_ERRNO(EIO)},
> -			{FAN_OPEN_PERM, FAN_DENY_ERRNO(EBUSY)}
> +			{FAN_OPEN_PERM, FAN_ALLOW,0 ,0},
nit: space is usually before comma, not after. I'll fix it before merge.
I also prefer to use designated initializers when there are more struct members
and some of them are zero. But it's up to you, or I can change it later in a
separate patch.

> +			{FAN_PRE_ACCESS, FAN_ALLOW, 2, 100},
> +			{FAN_PRE_ACCESS, FAN_ALLOW,0 ,0},
> +			{FAN_PRE_ACCESS, FAN_DENY_ERRNO(EIO),0 ,0},
> +			{FAN_OPEN_PERM, FAN_DENY_ERRNO(EBUSY),0 ,0}

...
> @@ -190,17 +196,19 @@ static void generate_events(struct tcase *tc)
>  	 */
>  	fd = SAFE_OPEN(fname, O_RDWR | O_CREAT, 0700);

> -	exp_errno = expected_errno(event->response);
> -	event++;
> -
> +	exp_errno = 0;
> +	if (event->mask & FAN_PRE_ACCESS) {
> +		exp_errno = expected_errno(event->response);
> +		event++;
> +	}
>  	exp_ret = exp_errno ? -1 : 1;
> -	errno = 0;
>  	/*
>  	 * FAN_PRE_ACCESS events are reported on map() and write(), but should
>  	 * not be reported when faulting in the user page at offset page_sz*100
>  	 * that is used as an input buffer to the write() syscall.
>  	 */
> -	addr = SAFE_MMAP(NULL, page_sz, PROT_READ, MAP_PRIVATE, fd, page_sz*100);
> +	errno = 0;
> +	addr = SAFE_MMAP(NULL, page_sz*2, PROT_READ, MAP_PRIVATE, fd, page_sz*100);
>  	if (!addr || errno != exp_errno) {
nit (unrelated to this change): I wonder if "!addr" is really needed (addr !=
MAP_FAILED in safe_mmap() should be enough, right?

>  		tst_res(TFAIL, "mmap() got errno %d (expected %d)", errno, exp_errno);
>  		exit(3);
> @@ -208,12 +216,14 @@ static void generate_events(struct tcase *tc)
>  		tst_res(TINFO, "mmap() got errno %d as expected", errno);
>  	}
...

Kind regards,
Petr

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