[LTP] [PATCH 1/2] add_key05: Avoid race with key garbage collection

[Yang Xu]

Hi Richard

> The key subsystem independently tracks user info against UID. If a user is
> deleted and the UID reused for a new user then the key subsystem will mistake
> the new user for the old one.
Does any documentation or kernel comment mentioned this? I didn't notice 
this before.
> 
> The keys/keyrings may not be accessible to the new user, but if they are not
> yet garbage collected (which happens asynchronously) then the new user may be
> exceeding its quota limits.
> 
> This results in a race condition where this test can fail because the old
> thread keyring is taking up the full quota. We should be able to avoid this by
> creating two users in parallel instead of sequentially so that they have
> different UIDs.
I guess you may want to creat two user, so next, the key subsystem 
think the new user is different from  the last deleting user. It can 
avoid race.

But you patch overrides ltpuser, in actually, we still use 
ltp_add_key05_1 in SAFE_SETUID.

Also, this patch doesn't handle delete user when we using -i parameters.

I think the right way should use ltp_add_key05_0 for the 1st case and 
use ltp_add_key05_1 for the 2nd case.  how about the following code?

--- a/testcases/kernel/syscalls/add_key/add_key05.c
+++ b/testcases/kernel/syscalls/add_key/add_key05.c
@@ -167,6 +167,7 @@ count:

  static void do_test(unsigned int n)
  {
+       add_user(n);
         if (!SAFE_FORK()) {
                 SAFE_SETUID(ltpuser->pw_uid);
                 TEST(add_key("user", "test1", user_buf, 64, 
KEY_SPEC_THREAD_KEYRING));
@@ -181,6 +182,7 @@ static void do_test(unsigned int n)
                 exit(0);
         }
         tst_reap_children();
+       clean_user(n);
  }

  static void setup(void)
@@ -188,14 +190,12 @@ static void setup(void)
         SAFE_FILE_PRINTF("/proc/sys/kernel/keys/maxkeys", "200");
         SAFE_FILE_PRINTF("/proc/sys/kernel/keys/maxbytes", "20000");

-       add_user(0);
-       add_user(1);
  }


> 
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>   testcases/kernel/syscalls/add_key/add_key05.c | 36 ++++++++++---------
>   1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
> index f64c359bb..5691b8579 100644
> --- a/testcases/kernel/syscalls/add_key/add_key05.c
> +++ b/testcases/kernel/syscalls/add_key/add_key05.c
> @@ -19,8 +19,6 @@
>   #include "lapi/keyctl.h"
>   
>   static char *user_buf;
> -static const char *username = "ltp_add_key05";
> -static int user_added;
>   struct passwd *ltpuser;
>   static char fmt[1024];
>   
> @@ -30,30 +28,29 @@ static const char *const save_restore[] = {
>   	NULL,
>   };
>   
> -static void add_user(void)
> +static void add_user(char n)
>   {
> -	if (user_added)
> -		return;
> -
> +	char username[] = "ltp_add_key05_n";
>   	const char *const cmd_useradd[] = {"useradd", username, NULL};
>   
> +	username[sizeof(username) - 2] = '0' + n;
> +
>   	SAFE_CMD(cmd_useradd, NULL, NULL);
> -	user_added = 1;
>   	ltpuser = SAFE_GETPWNAM(username);
>   	sprintf(fmt, "%5u: %%*5d %%*d/%%*d %%d/%%d %%d/%%d", ltpuser->pw_uid);
> +
> +	tst_res(TINFO, "Created user %s", ltpuser->pw_name);
>   }
>   
> -static void clean_user(void)
> +static void clean_user(char n)
>   {
> -	if (!user_added)
> -		return;
> -
> +	char username[] = "ltp_add_key05_n";
>   	const char *const cmd_userdel[] = {"userdel", "-r", username, NULL};
>   
> +	username[sizeof(username) - 2] = '0' + n;
> +
>   	if (tst_cmd(cmd_userdel, NULL, NULL, TST_CMD_PASS_RETVAL))
>   		tst_res(TWARN | TERRNO, "'userdel -r %s' failed", username);
> -	else
> -		user_added = 0;
>   }
>   
>   static inline void parse_proc_key_users(int *used_key, int *max_key, int *used_bytes, int *max_bytes)
> @@ -170,7 +167,6 @@ count:
>   
>   static void do_test(unsigned int n)
>   {
> -	add_user();
>   	if (!SAFE_FORK()) {
>   		SAFE_SETUID(ltpuser->pw_uid);
>   		TEST(add_key("user", "test1", user_buf, 64, KEY_SPEC_THREAD_KEYRING));
> @@ -185,13 +181,21 @@ static void do_test(unsigned int n)
>   		exit(0);
>   	}
>   	tst_reap_children();
> -	clean_user();
>   }
>   
>   static void setup(void)
>   {
>   	SAFE_FILE_PRINTF("/proc/sys/kernel/keys/maxkeys", "200");
>   	SAFE_FILE_PRINTF("/proc/sys/kernel/keys/maxbytes", "20000");
> +
> +	add_user(0);
> +	add_user(1);
> +}
> +
> +static void cleanup(void)
> +{
> +	clean_user(0);
> +	clean_user(1);
>   }
>   
>   static struct tst_test test = {
> @@ -200,7 +204,7 @@ static struct tst_test test = {
>   	.needs_root = 1,
>   	.forks_child = 1,
>   	.setup = setup,
> -	.cleanup = clean_user,
> +	.cleanup = cleanup,
>   	.save_restore = save_restore,
>   	.bufs = (struct tst_buffers []) {
>   		{&user_buf, .size = 64},
>