syscalls/add_key05: add maxbytes/maxkeys test under unprivileged user

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/add_key/.gitignore  |   1 +
 testcases/kernel/syscalls/add_key/add_key05.c | 180 ++++++++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 testcases/kernel/syscalls/add_key/add_key05.c

Hi!
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 577a4a527..df7bbcbf1 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -18,6 +18,7 @@ add_key01 add_key01
>  add_key02 add_key02
>  add_key03 add_key03
>  add_key04 add_key04
> +add_key05 add_key05
>  
>  adjtimex01 adjtimex01
>  adjtimex02 adjtimex02
> diff --git a/testcases/kernel/syscalls/add_key/.gitignore b/testcases/kernel/syscalls/add_key/.gitignore
> index b9a04214d..f57dc2228 100644
> --- a/testcases/kernel/syscalls/add_key/.gitignore
> +++ b/testcases/kernel/syscalls/add_key/.gitignore
> @@ -2,3 +2,4 @@
>  /add_key02
>  /add_key03
>  /add_key04
> +/add_key05
> diff --git a/testcases/kernel/syscalls/add_key/add_key05.c b/testcases/kernel/syscalls/add_key/add_key05.c
> new file mode 100644
> index 000000000..0d5e9db28
> --- /dev/null
> +++ b/testcases/kernel/syscalls/add_key/add_key05.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved.
> + * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
> + *
> + *Description:
> + * Test unprivileged user can support the number of keys and the
> + * number of bytes consumed in payloads of the keys.The defalut value
> + * is 200 and 20000.
> + * This is also a regresstion test for
> + * commit a08bf91ce28e ("KEYS: allow reaching the keys quotas exact")
> + *
> + */
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <pwd.h>
> +#include "tst_test.h"
> +#include "lapi/keyctl.h"
> +
> +static char *user_buf, *user_buf1, *keyring_buf;
> +static const char *username = "ltp_add_key05";
> +static int user_added;
> +struct passwd *ltpuser;
> +static unsigned int used_bytes, max_bytes, used_key, max_key, data_len;
> +char fmt[1024];
> +int flag[2] = {1, 0};
> +
> +void add_user(void)
> +{
> +	if (user_added)
> +		return;
> +
> +	const char *const cmd_useradd[] = {"useradd", username, NULL};
> +	int rc;
> +
> +	switch ((rc = tst_run_cmd(cmd_useradd, NULL, NULL, 1))) {
> +	case 0:
> +		user_added = 1;
> +		ltpuser = SAFE_GETPWNAM(username);
> +		break;
> +	case 1:
> +	case 255:
> +		break;
> +	default:
> +		tst_brk(TBROK, "Useradd failed (%d)", rc);
> +	}
> +	sprintf(fmt, "%5u: %%*5d %%*d/%%*d %%d/%%d %%d/%%d", ltpuser->pw_uid);
> +}
> +
> +void clean_user(void)
> +{
> +	if (!user_added)
> +		return;
> +
> +	const char *const cmd_userdel[] = {"userdel", "-r", username, NULL};
> +
> +	if (tst_run_cmd(cmd_userdel, NULL, NULL, 1))
> +		tst_res(TWARN | TERRNO, "'userdel -r %s' failed", username);
> +	else
> +		user_added = 0;
> +}
> +
> +void verify_max_btyes(void)
> +{
> +	char *buf, *invalid_buf;
> +	int plen, invalid_plen;
> +
> +	tst_res(TINFO, "test max bytes under unprivileged user");
> +	invalid_plen = max_bytes - used_bytes - data_len - 8;

What is the -8 for, strlen("test_inv")?

I guess that it will be more readable if we used the strlen() here as
well.

> +	plen = invalid_plen - 1;
> +	buf = tst_alloc(plen);
> +	invalid_buf = tst_alloc(invalid_plen);
> +
> +	TEST(add_key("user", "test_inv", invalid_buf, invalid_plen, KEY_SPEC_THREAD_KEYRING));
> +	if (TST_RET != -1)
> +		tst_res(TFAIL, "add_key(test_inv) succeeded unexpectedltly");
> +	else {
> +		if (TST_ERR == EDQUOT)
> +			tst_res(TPASS | TTERRNO, "add_key(test_inv) failed as expected");
> +		else
> +			tst_res(TFAIL | TTERRNO, "add_key(test_inv) failed expected EDQUOT got");
> +	}
> +
> +	TEST(add_key("user", "test_max", buf, plen, KEY_SPEC_THREAD_KEYRING));
> +	if (TST_RET != -1) {
> +		tst_res(TPASS, "add_key(test_max) succeeded as expected");
> +		SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, &max_key, &used_bytes, &max_bytes);
> +		if (used_bytes == max_bytes)
> +			tst_res(TPASS, "allow reaching the max bytes exactly");
> +		else
> +			tst_res(TFAIL, "max used bytes %u, key allow max bytes %u", used_bytes, max_bytes);
> +	} else
> +		tst_res(TFAIL | TTERRNO, "add_key(test_max) failed unexpectedly");
> +}
> +
> +void verify_max_keys(void)
> +{
> +	unsigned int i;
> +	char desc[10];
> +
> +	tst_res(TINFO, "test max keys under unprivileged user");
> +	for (i = used_key + 1; i < max_key; i++) {
> +		sprintf(desc, "abc%d", i);
> +		TEST(add_key("keyring", desc, keyring_buf, 0, KEY_SPEC_THREAD_KEYRING));
> +		if (TST_RET == -1)
> +			tst_res(TFAIL | TTERRNO, "add keyring key(%s) failed", desc);

Why we are using the "keyring" ring here instead? I doubt that it
matters for the quota, but it just seems strange.

Also we should stop the loop on a first failure, I guess that goto
count: would suffice.

> +	}
> +
> +	TEST(add_key("keyring", "abc200", keyring_buf, 0, KEY_SPEC_THREAD_KEYRING));
> +
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "add keyring key(abc200) failed");
> +		goto count;
> +	} else
> +		tst_res(TPASS, "add keyring key(abc200) succedd");

Why don't we just start the loop above at used_key? There is no point in
adding the last key here outside of the loop.

> +	TEST(add_key("keyring", "abc201", keyring_buf, 0, KEY_SPEC_THREAD_KEYRING));
> +	if (TST_RET != -1) {
> +		tst_res(TFAIL, "add keyring key(abc201) succeeded unexpectedly");
> +		goto count;

We should add a key with a different name than the previous "abc%d"
pattern here, if the upper limit was over 200 we would just replace a
key here instread which will not fail at all.

> +	} else {
> +		if (TST_ERR == EDQUOT)
> +			tst_res(TPASS | TTERRNO, "add keyring key(abc201) failed as expected over max key");
> +		else
> +			tst_res(TFAIL | TTERRNO, "add_keyring failed expected EDQUOT got");
> +	}
> +count:
> +	SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, &max_key, &used_bytes, &max_bytes);
> +	if (used_key == max_key)
> +		tst_res(TPASS, "allow reaching the max key exactly");
> +	else
> +		tst_res(TFAIL, "max used key %u, key allow max key %u", used_key, max_key);
> +}
> +
> +static void do_test(unsigned int n)
> +{
> +	add_user();
> +	int f_used_bytes = 0;
> +
> +	if (!SAFE_FORK()) {
> +		SAFE_SETUID(ltpuser->pw_uid);
> +
> +		TEST(add_key("user", "test1", user_buf, 64, KEY_SPEC_THREAD_KEYRING));
> +		if (TST_RET == -1)
> +			tst_brk(TFAIL | TTERRNO, "add key test1 failed");
> +		SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, &max_key, &used_bytes, &max_bytes);
> +		f_used_bytes = used_bytes;
> +
> +		TEST(add_key("user", "test2", user_buf1, 64, KEY_SPEC_THREAD_KEYRING));
> +		if (TST_RET == -1)
> +			tst_brk(TFAIL | TTERRNO, "add key test2 failed");

Do we really need to pass a different users_buf to each call?

> +		SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, &max_key, &used_bytes, &max_bytes);
> +
> +		data_len = used_bytes - f_used_bytes - strlen("test1") - 1 - 64;

So this code here is used to determine how much data will consume the
key structure in kernel itself? I guess that this is useless to run in
the case of testing for maximal number of keys, right? Can we put this
code into a function something as get_kernel_key_data_size() and call it
from the verify_max_bytes() only?

I guess that the SAFE_FILE_LINES_SCANF() intializes the max_key for the
max_keys test. Having a global variables that are initalized in random
places makes the code really confusing, can we avoid that if poissible
please?

I guess that we can at least create:

parse_proc_key_users(int *used_keys, int *max_key, int *used_bytes, *int max_bytes)

function that would read the values into a local variables and copy the
results only if non-NULL pointers were passed.

That way the verify_max_keys() would call:
parse_proc_key_users(NULL, &max_keys, NULL, NULL);
and use the result for the loop that adds the keys.

> +		if (flag[n])

Huh, why not just if (n)?

> +			verify_max_btyes();
> +		else
> +			verify_max_keys();
> +		exit(0);
> +	}
> +	tst_reap_children();
> +	clean_user();

What is the reason to add and remove a user for each iteration?

Aare you cleaning the keys that way?

> +}
> +
> +static struct tst_test test = {
> +	.test = do_test,
> +	.tcnt = 2,
> +	.needs_root = 1,
> +	.forks_child = 1,
> +	.cleanup = clean_user,
> +	.bufs = (struct tst_buffers []) {
> +		{&keyring_buf, .size = 1},
> +		{&user_buf, .size = 64},
> +		{&user_buf1, .size = 64},
> +		{}
> +	},
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "a08bf91ce28"},
> +		{}
> +	}
> +};
> -- 
> 2.18.0
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

Hi!

> Hi!
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index 577a4a527..df7bbcbf1 100644
>> --
>> +void verify_max_btyes(void)
>> +{
>> +	char *buf, *invalid_buf;
>> +	int plen, invalid_plen;
>> +
>> +	tst_res(TINFO, "test max bytes under unprivileged user");
>> +	invalid_plen = max_bytes - used_bytes - data_len - 8;
> 
> What is the -8 for, strlen("test_inv")?
Yes.
> 
> I guess that it will be more readable if we used the strlen() here as
> well.
OK. I will use strlen.
> 
>> +	plen = invalid_plen - 1;
>> +	buf = tst_alloc(plen);
>> +	invalid_buf = tst_alloc(invalid_plen);
>> +
>> +	TEST(add_key("user", "test_inv", invalid_buf, invalid_plen, KEY_SPEC_THREAD_KEYRING));
>> +	if (TST_RET != -1)
>> +		tst_res(TFAIL, "add_key(test_inv) succeeded unexpectedltly");
>> +	else {
>> +		if (TST_ERR == EDQUOT)
>> +			tst_res(TPASS | TTERRNO, "add_key(test_inv) failed as expected");
>> +		else
>> +			tst_res(TFAIL | TTERRNO, "add_key(test_inv) failed expected EDQUOT got");
>> +	}
>> +
>> +	TEST(add_key("user", "test_max", buf, plen, KEY_SPEC_THREAD_KEYRING));
>> +	if (TST_RET != -1) {
>> +		tst_res(TPASS, "add_key(test_max) succeeded as expected");
>> +		SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, &max_key, &used_bytes, &max_bytes);
>> +		if (used_bytes == max_bytes)
>> +			tst_res(TPASS, "allow reaching the max bytes exactly");
>> +		else
>> +			tst_res(TFAIL, "max used bytes %u, key allow max bytes %u", used_bytes, max_bytes);
>> +	} else
>> +		tst_res(TFAIL | TTERRNO, "add_key(test_max) failed unexpectedly");
>> +}
>> +
>> +void verify_max_keys(void)
>> +{
>> +	unsigned int i;
>> +	char desc[10];
>> +
>> +	tst_res(TINFO, "test max keys under unprivileged user");
>> +	for (i = used_key + 1; i < max_key; i++) {
>> +		sprintf(desc, "abc%d", i);
>> +		TEST(add_key("keyring", desc, keyring_buf, 0, KEY_SPEC_THREAD_KEYRING));
>> +		if (TST_RET == -1)
>> +			tst_res(TFAIL | TTERRNO, "add keyring key(%s) failed", desc);
> 
> Why we are using the "keyring" ring here instead? I doubt that it
> matters for the quota, but it just seems strange.
It doesn't matter quota, only because "keyrings" plen is 0. I will use 
"user" type for this.
  >
> Also we should stop the loop on a first failure, I guess that goto
> count: would suffice.
I ignored this before, thanks.
> 
>> +	}
>> +
>> +	TEST(add_key("keyring", "abc200", keyring_buf, 0, KEY_SPEC_THREAD_KEYRING));
>> +
>> +	if (TST_RET == -1) {
>> +		tst_res(TFAIL | TTERRNO, "add keyring key(abc200) failed");
>> +		goto count;
>> +	} else
>> +		tst_res(TPASS, "add keyring key(abc200) succedd");
> 
> Why don't we just start the loop above at used_key? There is no point in
> adding the last key here outside of the loop.
> 
Using start + 1 more clean：
abc100 >> the 100th key,
We can change code (including last key)as below:
for (i = used_key + 1; i <= max_key; i ++)

>> +	TEST(add_key("keyring", "abc201", keyring_buf, 0, KEY_SPEC_THREAD_KEYRING));
>> +	if (TST_RET != -1) {
>> +		tst_res(TFAIL, "add keyring key(abc201) succeeded unexpectedly");
>> +		goto count;
> 
> We should add a key with a different name than the previous "abc%d"
> pattern here, if the upper limit was over 200 we would just replace a
> key here instread which will not fail at all.
Do you mean to use  "invalid_num_key" to avoid upper limit over 200?

> 
>> +	} else {
>> +		if (TST_ERR == EDQUOT)
>> +			tst_res(TPASS | TTERRNO, "add keyring key(abc201) failed as expected over max key");
>> +		else
>> +			tst_res(TFAIL | TTERRNO, "add_keyring failed expected EDQUOT got");
>> +	}
>> +count:
>> +	SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, &max_key, &used_bytes, &max_bytes);
>> +	if (used_key == max_key)
>> +		tst_res(TPASS, "allow reaching the max key exactly");
>> +	else
>> +		tst_res(TFAIL, "max used key %u, key allow max key %u", used_key, max_key);
>> +}
>> +
>> +static void do_test(unsigned int n)
>> +{
>> +	add_user();
>> +	int f_used_bytes = 0;
>> +
>> +	if (!SAFE_FORK()) {
>> +		SAFE_SETUID(ltpuser->pw_uid);
>> +
>> +		TEST(add_key("user", "test1", user_buf, 64, KEY_SPEC_THREAD_KEYRING));
>> +		if (TST_RET == -1)
>> +			tst_brk(TFAIL | TTERRNO, "add key test1 failed");
>> +		SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, &max_key, &used_bytes, &max_bytes);
>> +		f_used_bytes = used_bytes;
>> +
>> +		TEST(add_key("user", "test2", user_buf1, 64, KEY_SPEC_THREAD_KEYRING));
>> +		if (TST_RET == -1)
>> +			tst_brk(TFAIL | TTERRNO, "add key test2 failed");
> 
> Do we really need to pass a different users_buf to each call?
I have seen kernel code, payload is used to instantiate or update the 
key, I think it is no problme to use the same buf because we don't get 
it again from kernel space.
> 
>> +		SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, &max_key, &used_bytes, &max_bytes);
>> +
>> +		data_len = used_bytes - f_used_bytes - strlen("test1") - 1 - 64;
> 
> So this code here is used to determine how much data will consume the
> key structure in kernel itself? 
I debug this code after adding test1 key, show in /proc/keys
add_key05.c:154: INFO: 1535d151 I--Q---    24 perm 3f030000     0     0 
keyring   _ses: 1

add_key05.c:154: INFO: 15d7df09 I--Q---     6 perm 1f3f0000     0 65534 
keyring   _uid.0: empty

add_key05.c:154: INFO: 1a300c72 I--Q---     1 perm 3f010000  1003     0 
user      test1: 64

add_key05.c:154: INFO: 2e57738c I--Q---     1 perm 3f010000  1003     0 
keyring   _tid: 1

the key num is 2 in /proc/key-users, I use this code to figure keyring 
  _tid: 1(I don't know why has it) consumed data bytes.
I guess that this is useless to run in
> the case of testing for maximal number of keys, right? Can we put this
> code into a function something as get_kernel_key_data_size() and call it
> from the verify_max_bytes() only?
Yes, it was only used in max_bytes.
> 
> I guess that the SAFE_FILE_LINES_SCANF() intializes the max_key for the
> max_keys test. Having a global variables that are initalized in random
> places makes the code really confusing, can we avoid that if poissible
> please?
Ok. I will avoid this and make code clean.
> 
> I guess that we can at least create:
> 
> parse_proc_key_users(int *used_keys, int *max_key, int *used_bytes, *int max_bytes)
> 
> function that would read the values into a local variables and copy the
> results only if non-NULL pointers were passed.
> 
> That way the verify_max_keys() would call:
> parse_proc_key_users(NULL, &max_keys, NULL, NULL);
> and use the result for the loop that adds the keys.
Sound good, I will use it.
> 
>> +		if (flag[n])
> 
> Huh, why not just if (n)?
> 
OK.
>> +			verify_max_btyes();
>> +		else
>> +			verify_max_keys();
>> +		exit(0);
>> +	}
>> +	tst_reap_children();
>> +	clean_user();
> 
> What is the reason to add and remove a user for each iteration?
> 
> Aare you cleaning the keys that way?
Yes, I planed to look for a wise (try KEYCTL_CLEAR
  or KEYCTL_INVALIDATE)way but failed. Can you give me some
suggestion?
> 
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test = do_test,
>> +	.tcnt = 2,
>> +	.needs_root = 1,
>> +	.forks_child = 1,
>> +	.cleanup = clean_user,
>> +	.bufs = (struct tst_buffers []) {
>> +		{&keyring_buf, .size = 1},
>> +		{&user_buf, .size = 64},
>> +		{&user_buf1, .size = 64},
>> +		{}
>> +	},
>> +	.tags = (const struct tst_tag[]) {
>> +		{"linux-git", "a08bf91ce28"},
>> +		{}
>> +	}
>> +};
>> -- 
>> 2.18.0
>>
>>
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>

Hi!

> Hi!
> 
>> Hi!
>>> diff --git a/runtest/syscalls b/runtest/syscalls
>>> index 577a4a527..df7bbcbf1 100644
>>> -- 
>>> +void verify_max_btyes(void)
>>> +{
>>> +    char *buf, *invalid_buf;
>>> +    int plen, invalid_plen;
>>> +
>>> +    tst_res(TINFO, "test max bytes under unprivileged user");
>>> +    invalid_plen = max_bytes - used_bytes - data_len - 8;
>>
>> What is the -8 for, strlen("test_inv")?
> Yes.
>>
>> I guess that it will be more readable if we used the strlen() here as
>> well.
> OK. I will use strlen.
>>
>>> +    plen = invalid_plen - 1;
>>> +    buf = tst_alloc(plen);
>>> +    invalid_buf = tst_alloc(invalid_plen);
>>> +
>>> +    TEST(add_key("user", "test_inv", invalid_buf, invalid_plen, 
>>> KEY_SPEC_THREAD_KEYRING));
>>> +    if (TST_RET != -1)
>>> +        tst_res(TFAIL, "add_key(test_inv) succeeded unexpectedltly");
>>> +    else {
>>> +        if (TST_ERR == EDQUOT)
>>> +            tst_res(TPASS | TTERRNO, "add_key(test_inv) failed as 
>>> expected");
>>> +        else
>>> +            tst_res(TFAIL | TTERRNO, "add_key(test_inv) failed 
>>> expected EDQUOT got");
>>> +    }
>>> +
>>> +    TEST(add_key("user", "test_max", buf, plen, 
>>> KEY_SPEC_THREAD_KEYRING));
>>> +    if (TST_RET != -1) {
>>> +        tst_res(TPASS, "add_key(test_max) succeeded as expected");
>>> +        SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, 
>>> &max_key, &used_bytes, &max_bytes);
>>> +        if (used_bytes == max_bytes)
>>> +            tst_res(TPASS, "allow reaching the max bytes exactly");
>>> +        else
>>> +            tst_res(TFAIL, "max used bytes %u, key allow max bytes 
>>> %u", used_bytes, max_bytes);
>>> +    } else
>>> +        tst_res(TFAIL | TTERRNO, "add_key(test_max) failed 
>>> unexpectedly");
>>> +}
>>> +
>>> +void verify_max_keys(void)
>>> +{
>>> +    unsigned int i;
>>> +    char desc[10];
>>> +
>>> +    tst_res(TINFO, "test max keys under unprivileged user");
>>> +    for (i = used_key + 1; i < max_key; i++) {
>>> +        sprintf(desc, "abc%d", i);
>>> +        TEST(add_key("keyring", desc, keyring_buf, 0, 
>>> KEY_SPEC_THREAD_KEYRING));
>>> +        if (TST_RET == -1)
>>> +            tst_res(TFAIL | TTERRNO, "add keyring key(%s) failed", 
>>> desc);
>>
>> Why we are using the "keyring" ring here instead? I doubt that it
>> matters for the quota, but it just seems strange.
> It doesn't matter quota, only because "keyrings" plen is 0. I will use 
> "user" type for this.
>   >
>> Also we should stop the loop on a first failure, I guess that goto
>> count: would suffice.
> I ignored this before, thanks.
>>
>>> +    }
>>> +
>>> +    TEST(add_key("keyring", "abc200", keyring_buf, 0, 
>>> KEY_SPEC_THREAD_KEYRING));
>>> +
>>> +    if (TST_RET == -1) {
>>> +        tst_res(TFAIL | TTERRNO, "add keyring key(abc200) failed");
>>> +        goto count;
>>> +    } else
>>> +        tst_res(TPASS, "add keyring key(abc200) succedd");
>>
>> Why don't we just start the loop above at used_key? There is no point in
>> adding the last key here outside of the loop.
>>
> Using start + 1 more clean：
> abc100 >> the 100th key,
> We can change code (including last key)as below:
> for (i = used_key + 1; i <= max_key; i ++)
> 
>>> +    TEST(add_key("keyring", "abc201", keyring_buf, 0, 
>>> KEY_SPEC_THREAD_KEYRING));
>>> +    if (TST_RET != -1) {
>>> +        tst_res(TFAIL, "add keyring key(abc201) succeeded 
>>> unexpectedly");
>>> +        goto count;
>>
>> We should add a key with a different name than the previous "abc%d"
>> pattern here, if the upper limit was over 200 we would just replace a
>> key here instread which will not fail at all.
> Do you mean to use  "invalid_num_key" to avoid upper limit over 200?
> 
>>
>>> +    } else {
>>> +        if (TST_ERR == EDQUOT)
>>> +            tst_res(TPASS | TTERRNO, "add keyring key(abc201) failed 
>>> as expected over max key");
>>> +        else
>>> +            tst_res(TFAIL | TTERRNO, "add_keyring failed expected 
>>> EDQUOT got");
>>> +    }
>>> +count:
>>> +    SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, 
>>> &max_key, &used_bytes, &max_bytes);
>>> +    if (used_key == max_key)
>>> +        tst_res(TPASS, "allow reaching the max key exactly");
>>> +    else
>>> +        tst_res(TFAIL, "max used key %u, key allow max key %u", 
>>> used_key, max_key);
>>> +}
>>> +
>>> +static void do_test(unsigned int n)
>>> +{
>>> +    add_user();
>>> +    int f_used_bytes = 0;
>>> +
>>> +    if (!SAFE_FORK()) {
>>> +        SAFE_SETUID(ltpuser->pw_uid);
>>> +
>>> +        TEST(add_key("user", "test1", user_buf, 64, 
>>> KEY_SPEC_THREAD_KEYRING));
>>> +        if (TST_RET == -1)
>>> +            tst_brk(TFAIL | TTERRNO, "add key test1 failed");
>>> +        SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, 
>>> &max_key, &used_bytes, &max_bytes);
>>> +        f_used_bytes = used_bytes;
>>> +
>>> +        TEST(add_key("user", "test2", user_buf1, 64, 
>>> KEY_SPEC_THREAD_KEYRING));
>>> +        if (TST_RET == -1)
>>> +            tst_brk(TFAIL | TTERRNO, "add key test2 failed");
>>
>> Do we really need to pass a different users_buf to each call?
> I have seen kernel code, payload is used to instantiate or update the 
> key, I think it is no problme to use the same buf because we don't get 
> it again from kernel space.
>>
>>> +        SAFE_FILE_LINES_SCANF("/proc/key-users", fmt, &used_key, 
>>> &max_key, &used_bytes, &max_bytes);
>>> +
>>> +        data_len = used_bytes - f_used_bytes - strlen("test1") - 1 - 
>>> 64;
>>
>> So this code here is used to determine how much data will consume the
>> key structure in kernel itself? 

Sorry for the wrong info. This code is designed to compute delta, I have 
seen kernel code yestorday:
add_key calltrace as below:
add_key()
   key_create_or_update()
     key_alloc()
     __key_instantiate_and_link
       generic_key_instantiate
         key_payload_reserve
           ......

In key_alloc, we use type->def_datalen to alloc space, but when 
instantiating key, we using delta(quota_len -type->def_datalen) to 
adjust reserver space. So I compute this variable(delta, add_key05.c 
uses data_len) to avoid  that we can reach max bytes in key_alloc but 
got EDQOUT in key_payload_reserve.

more info:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/security/keys/core.rst#n1353

Also, using my v2 patch(send later), this case also fails on lastest 
kernel because of incomplete kernel commit.

more info:
https://patchwork.kernel.org/patch/11411507/

Best Regards
Yang Xu
> I debug this code after adding test1 key, show in /proc/keys
> add_key05.c:154: INFO: 1535d151 I--Q---    24 perm 3f030000     0     0 
> keyring   _ses: 1
> 
> add_key05.c:154: INFO: 15d7df09 I--Q---     6 perm 1f3f0000     0 65534 
> keyring   _uid.0: empty
> 
> add_key05.c:154: INFO: 1a300c72 I--Q---     1 perm 3f010000  1003     0 
> user      test1: 64
> 
> add_key05.c:154: INFO: 2e57738c I--Q---     1 perm 3f010000  1003     0 
> keyring   _tid: 1
> 
> the key num is 2 in /proc/key-users, I use this code to figure keyring 
>   _tid: 1(I don't know why has it) consumed data bytes.
> I guess that this is useless to run in
>> the case of testing for maximal number of keys, right? Can we put this
>> code into a function something as get_kernel_key_data_size() and call it
>> from the verify_max_bytes() only?
> Yes, it was only used in max_bytes.
>>
>> I guess that the SAFE_FILE_LINES_SCANF() intializes the max_key for the
>> max_keys test. Having a global variables that are initalized in random
>> places makes the code really confusing, can we avoid that if poissible
>> please?
> Ok. I will avoid this and make code clean.
>>
>> I guess that we can at least create:
>>
>> parse_proc_key_users(int *used_keys, int *max_key, int *used_bytes, 
>> *int max_bytes)
>>
>> function that would read the values into a local variables and copy the
>> results only if non-NULL pointers were passed.
>>
>> That way the verify_max_keys() would call:
>> parse_proc_key_users(NULL, &max_keys, NULL, NULL);
>> and use the result for the loop that adds the keys.
> Sound good, I will use it.
>>
>>> +        if (flag[n])
>>
>> Huh, why not just if (n)?
>>
> OK.
>>> +            verify_max_btyes();
>>> +        else
>>> +            verify_max_keys();
>>> +        exit(0);
>>> +    }
>>> +    tst_reap_children();
>>> +    clean_user();
>>
>> What is the reason to add and remove a user for each iteration?
>>
>> Aare you cleaning the keys that way?
> Yes, I planed to look for a wise (try KEYCTL_CLEAR
>   or KEYCTL_INVALIDATE)way but failed. Can you give me some
> suggestion?
>>
>>> +}
>>> +
>>> +static struct tst_test test = {
>>> +    .test = do_test,
>>> +    .tcnt = 2,
>>> +    .needs_root = 1,
>>> +    .forks_child = 1,
>>> +    .cleanup = clean_user,
>>> +    .bufs = (struct tst_buffers []) {
>>> +        {&keyring_buf, .size = 1},
>>> +        {&user_buf, .size = 64},
>>> +        {&user_buf1, .size = 64},
>>> +        {}
>>> +    },
>>> +    .tags = (const struct tst_tag[]) {
>>> +        {"linux-git", "a08bf91ce28"},
>>> +        {}
>>> +    }
>>> +};
>>> -- 
>>> 2.18.0
>>>
>>>
>>>
>>>
>>> -- 
>>> Mailing list info: https://lists.linux.it/listinfo/ltp
>>