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

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

[Richard Palethorpe]

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.

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.

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},
-- 
2.24.0

[LTP] [PATCH 2/2] add_key05: Correct formatting

[Richard Palethorpe]

Use checkpatch.pl

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---
 testcases/kernel/syscalls/add_key/add_key05.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
index 5691b8579..d83339048 100644
--- a/testcases/kernel/syscalls/add_key/add_key05.c
+++ b/testcases/kernel/syscalls/add_key/add_key05.c
@@ -118,8 +118,8 @@ static void verify_max_btyes(void)
 	plen = max_bytes - used_bytes - delta - strlen("test_xxx") - 1;
 	TEST(add_key("user", "test_max", buf, plen, KEY_SPEC_THREAD_KEYRING));
 	if (TST_RET == -1) {
-		 tst_res(TFAIL | TTERRNO, "add_key(test_max) failed unexpectedly");
-		 return;
+		tst_res(TFAIL | TTERRNO, "add_key(test_max) failed unexpectedly");
+		return;
 	}
 
 	tst_res(TPASS, "add_key(test_max) succeeded as expected");
-- 
2.24.0

[LTP] [PATCH 2/2] add_key05: Correct formatting

[Yang Xu]

Hi Richard

Sorry for this wrong format. Thanks for your fix.
Acked-by.

Best Regards
Yang Xu
> Use checkpatch.pl
> 
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
>   testcases/kernel/syscalls/add_key/add_key05.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
> index 5691b8579..d83339048 100644
> --- a/testcases/kernel/syscalls/add_key/add_key05.c
> +++ b/testcases/kernel/syscalls/add_key/add_key05.c
> @@ -118,8 +118,8 @@ static void verify_max_btyes(void)
>   	plen = max_bytes - used_bytes - delta - strlen("test_xxx") - 1;
>   	TEST(add_key("user", "test_max", buf, plen, KEY_SPEC_THREAD_KEYRING));
>   	if (TST_RET == -1) {
> -		 tst_res(TFAIL | TTERRNO, "add_key(test_max) failed unexpectedly");
> -		 return;
> +		tst_res(TFAIL | TTERRNO, "add_key(test_max) failed unexpectedly");
> +		return;
>   	}
>   
>   	tst_res(TPASS, "add_key(test_max) succeeded as expected");
> 

[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},
> 

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

[Jan Stancek]

----- Original Message -----
> 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.

Thanks for raising this problem Richard. This matches CKI failure
we seen recently. (CC Li and Rachel)

> 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 might be problem, but other than that I think it works, at least for default run.

Though I'm wondering, shouldn't the test delete keys it creates,
rather than relying on garbage collection?

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

[Richard Palethorpe]

Hello,

Yang Xu <xuyang2018.jy@cn.fujitsu.com> writes:

> 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 definition of struct key_user in security/keys/internal.h.

/*
 * Keep track of keys for a user.
 *
 * This needs to be separate to user_struct to avoid a refcount-loop
 * (user_struct pins some keyrings which pin this struct).
 *
 * We also keep track of keys under request from userspace for this UID here.
 */
struct key_user {
	struct rb_node		node;
	struct mutex		cons_lock;	/* construction initiation lock */
	spinlock_t		lock;
	refcount_t		usage;		/* for accessing qnkeys & qnbytes */
	atomic_t		nkeys;		/* number of keys */
	atomic_t		nikeys;		/* number of instantiated keys */
	kuid_t			uid;
	int			qnkeys;		/* number of keys allocated to this user */
	int			qnbytes;	/* number of bytes allocated to this user */
};

As you can see it just has the uid and, AFIACT, deleting a user will not
cause this structure to be cleaned up. However I am not familiar with
the key code.

>>
>> 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.

Good catch!

>
> 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?

I don't think it is necessary to delete the user on every iteration
because of the above problem with struct key_user.

Infact I suppose there is still a race condition with -i because the
keys are freed asynchronously. Even if we delete the users the UIDs will
get reused. Deleting the user does slow down the test which may stop the
race most of the time, but I don't think it is a good solution.

Possibly what we should do instead is retry test1 if EDQUOT is returned?
However the keyring and its keys are not all freed atomically, so this
will still result in errors when test1 passes because one key was freed,
but the next test fails because the other keys are still not freed.

Frankly I am not sure this is going to work with an arbitrary value for
-i unless there is some way of forcibly freeing the keys? However we
could create up to 10 users (0-9) and recycle them?


>
> --- 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},
>>


--
Thank you,
Richard.

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

[Richard Palethorpe]

Hello,

Jan Stancek <jstancek@redhat.com> writes:

> ----- Original Message -----
>> 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.
>
> Thanks for raising this problem Richard. This matches CKI failure
> we seen recently. (CC Li and Rachel)
>
>> 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 might be problem, but other than that I think it works, at least for default run.
>
> Though I'm wondering, shouldn't the test delete keys it creates,
> rather than relying on garbage collection?

I'm assuming the keys are 'deleted' when the thread keyring is destroyed
when the child process exits. However they are not freed until later by
garbage collection (maybe I am confusing deferred freeing with 'garbage
collection'?).

We could explicitly delete/revoke the individual keys, but AFAICT there
would still be a race because freeing is still asynchronous. Ofcourse
there might be a reliable way to force freeing?

--
Thank you,
Richard.

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

[Jan Stancek]

----- Original Message -----
> Hello,
> 
> Jan Stancek <jstancek@redhat.com> writes:
> 
> > ----- Original Message -----
> >> 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.
> >
> > Thanks for raising this problem Richard. This matches CKI failure
> > we seen recently. (CC Li and Rachel)
> >
> >> 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 might be problem, but other than that I think it works, at least for
> > default run.
> >
> > Though I'm wondering, shouldn't the test delete keys it creates,
> > rather than relying on garbage collection?
> 
> I'm assuming the keys are 'deleted' when the thread keyring is destroyed
> when the child process exits. However they are not freed until later by
> garbage collection (maybe I am confusing deferred freeing with 'garbage
> collection'?).

Do you know how large is the race window?

Default /proc/sys/kernel/keys/gc_delay is 300, so if it's tied to this
garbage collect, I'd expect it to fail almost all the time.

> 
> We could explicitly delete/revoke the individual keys, but AFAICT there
> would still be a race because freeing is still asynchronous. Ofcourse
> there might be a reliable way to force freeing?

gc_delay is only one I recall.

If it's tied to process being around, I can try similar approach from 
e747d0456adc ("syscalls/tgkill03: wait for defunct tid to get detached")
where we wait for /proc/<pid>/task/<tid> to disappear.

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

[Richard Palethorpe]

Hello,

Jan Stancek <jstancek@redhat.com> writes:

> ----- Original Message -----
>> Hello,
>> 
>> Jan Stancek <jstancek@redhat.com> writes:
>> 
>> > ----- Original Message -----
>> >> 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.
>> >
>> > Thanks for raising this problem Richard. This matches CKI failure
>> > we seen recently. (CC Li and Rachel)
>> >
>> >> 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 might be problem, but other than that I think it works, at least for
>> > default run.
>> >
>> > Though I'm wondering, shouldn't the test delete keys it creates,
>> > rather than relying on garbage collection?
>> 
>> I'm assuming the keys are 'deleted' when the thread keyring is destroyed
>> when the child process exits. However they are not freed until later by
>> garbage collection (maybe I am confusing deferred freeing with 'garbage
>> collection'?).
>
> Do you know how large is the race window?
>
> Default /proc/sys/kernel/keys/gc_delay is 300, so if it's tied to this
> garbage collect, I'd expect it to fail almost all the time.

It doesn't appear to be tied to that.

>
>> 
>> We could explicitly delete/revoke the individual keys, but AFAICT there
>> would still be a race because freeing is still asynchronous. Ofcourse
>> there might be a reliable way to force freeing?
>
> gc_delay is only one I recall.
>
> If it's tied to process being around, I can try similar approach from 
> e747d0456adc ("syscalls/tgkill03: wait for defunct tid to get detached")
> where we wait for /proc/<pid>/task/<tid> to disappear.


This might work as the work is scheduled to be done in process context,
so the task may remain until the keys have been freed.

-- 
Thank you,
Richard.

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

[Richard Palethorpe]

Hello,

Richard Palethorpe <rpalethorpe@suse.de> writes:

>>> I'm assuming the keys are 'deleted' when the thread keyring is destroyed
>>> when the child process exits. However they are not freed until later by
>>> garbage collection (maybe I am confusing deferred freeing with 'garbage
>>> collection'?).
>>
>> Do you know how large is the race window?
>>
>> Default /proc/sys/kernel/keys/gc_delay is 300, so if it's tied to this
>> garbage collect, I'd expect it to fail almost all the time.
>
> It doesn't appear to be tied to that.
>
>>
>>> 
>>> We could explicitly delete/revoke the individual keys, but AFAICT there
>>> would still be a race because freeing is still asynchronous. Ofcourse
>>> there might be a reliable way to force freeing?
>>
>> gc_delay is only one I recall.
>>
>> If it's tied to process being around, I can try similar approach from 
>> e747d0456adc ("syscalls/tgkill03: wait for defunct tid to get detached")
>> where we wait for /proc/<pid>/task/<tid> to disappear.
>
>
> This might work as the work is scheduled to be done in process context,
> so the task may remain until the keys have been freed.

This doesn't seem to work.

-- 
Thank you,
Richard.

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

[Jan Stancek]

----- Original Message -----
> Hello,
> 
> Richard Palethorpe <rpalethorpe@suse.de> writes:
> 
> >>> I'm assuming the keys are 'deleted' when the thread keyring is destroyed
> >>> when the child process exits. However they are not freed until later by
> >>> garbage collection (maybe I am confusing deferred freeing with 'garbage
> >>> collection'?).
> >>
> >> Do you know how large is the race window?
> >>
> >> Default /proc/sys/kernel/keys/gc_delay is 300, so if it's tied to this
> >> garbage collect, I'd expect it to fail almost all the time.
> >
> > It doesn't appear to be tied to that.
> >
> >>
> >>> 
> >>> We could explicitly delete/revoke the individual keys, but AFAICT there
> >>> would still be a race because freeing is still asynchronous. Ofcourse
> >>> there might be a reliable way to force freeing?
> >>
> >> gc_delay is only one I recall.
> >>
> >> If it's tied to process being around, I can try similar approach from
> >> e747d0456adc ("syscalls/tgkill03: wait for defunct tid to get detached")
> >> where we wait for /proc/<pid>/task/<tid> to disappear.
> >
> >
> > This might work as the work is scheduled to be done in process context,
> > so the task may remain until the keys have been freed.
> 
> This doesn't seem to work.

We should probably go with your approach for now: create 2 users ahead,
so at least default case works.

I have difficulty reproducing failure on demand.