Re: [PATCH v2 5/5] fanotify21: Add FAN_REPORT_TID pidfd coverage

On Wed 17-06-26 05:04:48, AnonymeMeow wrote:
> Add a test case that generates an event from a thread. Without
> FAN_REPORT_TID, the reported pidfd is expected to refer to the thread
> group leader. With FAN_REPORT_TID, it is expected to refer to the
> triggering thread. Keep the event thread alive until the reported pidfd
> is verified, because pidfd fdinfo reports Pid: -1 after the thread
> exits.
> 
> Signed-off-by: AnonymeMeow <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
> 
> This patch is exactly the same as v1 except that the pthread link flag
> is added to fanotify21 target.
> 
> ---
>  testcases/kernel/syscalls/fanotify/Makefile   |  2 +-
>  .../kernel/syscalls/fanotify/fanotify21.c     | 47 ++++++++++++++++++-
>  2 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fanotify/Makefile b/testcases/kernel/syscalls/fanotify/Makefile
> index 3628094ba..b20bb50e9 100644
> --- a/testcases/kernel/syscalls/fanotify/Makefile
> +++ b/testcases/kernel/syscalls/fanotify/Makefile
> @@ -2,7 +2,7 @@
>  #  Copyright (c) Jan Kara <[email protected]>, 2013
>  
>  top_srcdir		?= ../../../..
> -fanotify11: CFLAGS+=-pthread
> +fanotify11 fanotify21: CFLAGS+=-pthread
>  include $(top_srcdir)/include/mk/testcases.mk
>  
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify21.c b/testcases/kernel/syscalls/fanotify/fanotify21.c
> index c05c69d58..c97ec4f92 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify21.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify21.c
> @@ -25,6 +25,7 @@
>  #include "tst_test.h"
>  #include "tst_safe_stdio.h"
>  #include "tst_safe_macros.h"
> +#include "tst_safe_pthread.h"
>  #include "lapi/pidfd.h"
>  
>  #ifdef HAVE_SYS_FANOTIFY_H
> @@ -47,6 +48,7 @@ struct pidfd_fdinfo_t {
>  
>  static struct test_case_t {
>  	char *name;
> +	int event_by_thread;
>  	int want_pidfd_err;
>  	int remount_ro;
>  } test_cases[] = {
> @@ -54,15 +56,24 @@ static struct test_case_t {
>  		"return a valid pidfd for event created by self",
>  		0,
>  		0,
> +		0,
> +	},
> +	{
> +		"return a valid pidfd for event created by thread",
> +		1,
> +		0,
> +		0,
>  	},
>  	{
>  		"return invalid pidfd for reader in descendant PID namespace",
> +		0,
>  		1,
>  		0,
>  	},
>  	{
>  		"fail to open rw fd for event created on read-only mount",
>  		0,
> +		0,
>  		1,
>  	},
>  };
> @@ -108,6 +119,26 @@ static void generate_event(void)
>  	SAFE_CLOSE(fd);
>  }
>  
> +static void *generate_event_pthread(void *args LTP_ATTRIBUTE_UNUSED)
> +{
> +	generate_event();
> +	TST_CHECKPOINT_WAIT(0);
> +	return NULL;
> +}
> +
> +static pthread_t event_thread_create(void)
> +{
> +	pthread_t pthread_id;
> +	SAFE_PTHREAD_CREATE(&pthread_id, NULL, generate_event_pthread, NULL);
> +	return pthread_id;
> +}
> +
> +static void event_thread_join(pthread_t pthread_id)
> +{
> +	TST_CHECKPOINT_WAKE(0);
> +	SAFE_PTHREAD_JOIN(pthread_id, NULL);
> +}
> +
>  static const char *test_variant_name(void)
>  {
>  	switch (tst_variant) {
> @@ -165,6 +196,7 @@ static void do_test(unsigned int num)
>  	int nopidfd_err = tc->want_pidfd_err ?
>  			  (TST_VARIANT_FD_ERROR ? -ESRCH : FAN_NOPIDFD) : 0;
>  	int fd_err = (tc->remount_ro && TST_VARIANT_FD_ERROR) ? -EROFS : 0;
> +	pthread_t event_thread;
>  	pid_t reader_pid;
>  	int reader_exit_status;
>  
> @@ -186,7 +218,16 @@ static void do_test(unsigned int num)
>  	SAFE_MOUNT("none", MOUNT_PATH, "none", MS_BIND | MS_REMOUNT |
>  		   (tc->remount_ro ? MS_RDONLY : 0), NULL);
>  
> -	generate_event();
> +	/*
> +	 * Generate the event by either self or a thread. Event generation in
> +	 * a thread is done so that in FAN_REPORT_TID mode the pidfd can be
> +	 * verified to refer to the tid, not the tgid. The thread waits until
> +	 * the pidfd is verified before exiting.
> +	 */
> +	if (tc->event_by_thread)
> +		event_thread = event_thread_create();
> +	else
> +		generate_event();
>  
>  	/*
>  	 * Read the event in either self or a child process in a descendant
> @@ -349,6 +390,9 @@ next_event:
>  
>  	if (tc->want_pidfd_err)
>  		exit(0);
> +
> +	if (tc->event_by_thread)
> +		event_thread_join(event_thread);
>  }
>  
>  static void do_cleanup(void)
> @@ -371,6 +415,7 @@ static struct tst_test test = {
>  	.mount_device = 1,
>  	.mntpoint = MOUNT_PATH,
>  	.forks_child = 1,
> +	.needs_checkpoints = 1,
>  };
>  
>  #else
> -- 
> 2.54.0
> 
-- 
Jan Kara <jack-IBi9RG/[email protected]>
SUSE Labs, CR

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