[LTP] [PATCH] syscalls/keyctl06: Print TFAIL if keyring_read() returns wrong size

[LTP] [PATCH] syscalls/keyctl06: Print TFAIL if keyring_read() returns wrong size

[Xiao Yang]

According to keyctl06's message, the mentioned bug is introduced
by the following patch which is merged into kernel since v3.13:
'b2a4df200d57 ("KEYS: Expand the capacity of a keyring")'

However, we still got the following output before v3.13:
 tst_test.c:958: INFO: Timeout per run is 0h 05m 00s
 keyctl06.c:60: BROK: KEYCTL_READ returned 8 but expected 4

In old kernels, the output exposed that keyring_read() could not
return the size of data read into buffer, because it just returned
the size of a keyring.  So i think this issue should be targeted
as TFAIL.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 testcases/kernel/syscalls/keyctl/keyctl06.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/keyctl/keyctl06.c b/testcases/kernel/syscalls/keyctl/keyctl06.c
index 8873431..bf30fb6 100644
--- a/testcases/kernel/syscalls/keyctl/keyctl06.c
+++ b/testcases/kernel/syscalls/keyctl/keyctl06.c
@@ -56,7 +56,7 @@ static void do_test(void)
 		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
 
 	if (TEST_RETURN != sizeof(key_serial_t)) {
-		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
+		tst_brk(TFAIL, "KEYCTL_READ returned %ld but expected %zu",
 			TEST_RETURN, sizeof(key_serial_t));
 	}
 
-- 
1.8.3.1

[LTP] [PATCH] syscalls/keyctl06: Print TFAIL if keyring_read() returns wrong size

[Eric Biggers]

Hi Xiao,

On Tue, Oct 17, 2017 at 08:53:12PM +0800, Xiao Yang wrote:
> According to keyctl06's message, the mentioned bug is introduced
> by the following patch which is merged into kernel since v3.13:
> 'b2a4df200d57 ("KEYS: Expand the capacity of a keyring")'
> 
> However, we still got the following output before v3.13:
>  tst_test.c:958: INFO: Timeout per run is 0h 05m 00s
>  keyctl06.c:60: BROK: KEYCTL_READ returned 8 but expected 4
> 
> In old kernels, the output exposed that keyring_read() could not
> return the size of data read into buffer, because it just returned
> the size of a keyring.  So i think this issue should be targeted
> as TFAIL.
> 
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  testcases/kernel/syscalls/keyctl/keyctl06.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/keyctl/keyctl06.c b/testcases/kernel/syscalls/keyctl/keyctl06.c
> index 8873431..bf30fb6 100644
> --- a/testcases/kernel/syscalls/keyctl/keyctl06.c
> +++ b/testcases/kernel/syscalls/keyctl/keyctl06.c
> @@ -56,7 +56,7 @@ static void do_test(void)
>  		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
>  
>  	if (TEST_RETURN != sizeof(key_serial_t)) {
> -		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
> +		tst_brk(TFAIL, "KEYCTL_READ returned %ld but expected %zu",
>  			TEST_RETURN, sizeof(key_serial_t));
>  	}
>  

It was actually pointed out yesterday that the short return value is a bug in
the kernel patch.  The documented behavior of keyctl_read() (as well as the
actual behavior for the other key types that implement it) is to return the full
count on a short read, rather than a short count.  It's not really intuitive but
I'm going to have to fix it with another kernel patch.

For now we probably should just make the test accept both return values:

	if (TEST_RETURN != sizeof(key_serial_t) &&
	    TEST_RETURN != sizeof(key_ids)) {
		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu or %zu",
			TEST_RETURN, sizeof(key_serial_t), sizeof(key_ids));
	}

Then once there is another kernel patch, I'll update the test to reference that
commit too, and accept only TEST_RETURN == sizeof(key_ids).

There is also the question of whether anything should be read at all when the
buffer is too small.  Currently the test assumes that a short read is done.
Unfortunately, there is no simple answer to that question as the documentation
for keyctl_read() and implementations are all inconsistent.

Eric

[LTP] [PATCH] syscalls/keyctl06: Print TFAIL if keyring_read() returns wrong size

[Xiao Yang]

Hi Eric,

On 2017/10/18 0:12, Eric Biggers wrote:

> Hi Xiao,
>
> On Tue, Oct 17, 2017 at 08:53:12PM +0800, Xiao Yang wrote:
>> According to keyctl06's message, the mentioned bug is introduced
>> by the following patch which is merged into kernel since v3.13:
>> 'b2a4df200d57 ("KEYS: Expand the capacity of a keyring")'
>>
>> However, we still got the following output before v3.13:
>>   tst_test.c:958: INFO: Timeout per run is 0h 05m 00s
>>   keyctl06.c:60: BROK: KEYCTL_READ returned 8 but expected 4
>>
>> In old kernels, the output exposed that keyring_read() could not
>> return the size of data read into buffer, because it just returned
>> the size of a keyring.  So i think this issue should be targeted
>> as TFAIL.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   testcases/kernel/syscalls/keyctl/keyctl06.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/testcases/kernel/syscalls/keyctl/keyctl06.c b/testcases/kernel/syscalls/keyctl/keyctl06.c
>> index 8873431..bf30fb6 100644
>> --- a/testcases/kernel/syscalls/keyctl/keyctl06.c
>> +++ b/testcases/kernel/syscalls/keyctl/keyctl06.c
>> @@ -56,7 +56,7 @@ static void do_test(void)
>>   		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
>>
>>   	if (TEST_RETURN != sizeof(key_serial_t)) {
>> -		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
>> +		tst_brk(TFAIL, "KEYCTL_READ returned %ld but expected %zu",
>>   			TEST_RETURN, sizeof(key_serial_t));
>>   	}
>>
> It was actually pointed out yesterday that the short return value is a bug in
> the kernel patch.  The documented behavior of keyctl_read() (as well as the
> actual behavior for the other key types that implement it) is to return the full
> count on a short read, rather than a short count.  It's not really intuitive but
> I'm going to have to fix it with another kernel patch.
Thanks for your explanation.
Sorry, i misunderstood the expected return value before.


> For now we probably should just make the test accept both return values:
>
> 	if (TEST_RETURN != sizeof(key_serial_t)&&
> 	TEST_RETURN != sizeof(key_ids)) {
> 		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu or %zu",
> 			TEST_RETURN, sizeof(key_serial_t), sizeof(key_ids));
> 	}
>
> Then once there is another kernel patch, I'll update the test to reference that
> commit too, and accept only TEST_RETURN == sizeof(key_ids).
Could we update the test to check both return values? as below:
if (TEST_RETURN != sizeof(key_ids)) {
	/* keyctl_read() should return the size of buffer required, rather than the size
	 * of data read into buffer. This bug was introduced by the commit:
	 * e645016abc80 ("KEYS: fix writing past end of user-supplied buffer in keyring_read()")
	 */
	if (TEST_RETURN == sizeof(key_serial_t)) {
		tst_brk(TFAIL, "KEYCTL_READ returned %ld but expected %zu",
			TEST_RETURN, sizeof(key_ids));
	}

	tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
		TEST_RETURN, sizeof(key_ids));
}

We probably should expose the short return value as a bug, rather than ignore it.


> There is also the question of whether anything should be read at all when the
> buffer is too small.  Currently the test assumes that a short read is done.
> Unfortunately, there is no simple answer to that question as the documentation
> for keyctl_read() and implementations are all inconsistent.
I also find the documentation and implementations are inconsistent, and either of
them may need to update.

Thanks，
Xiao yang


> Eric
>
>
>

[LTP] [PATCH] syscalls/keyctl06: Print TFAIL if keyring_read() returns wrong size

[Eric Biggers]

On Wed, Oct 18, 2017 at 02:25:25PM +0800, Xiao Yang wrote:
> >It was actually pointed out yesterday that the short return value is a bug in
> >the kernel patch.  The documented behavior of keyctl_read() (as well as the
> >actual behavior for the other key types that implement it) is to return the full
> >count on a short read, rather than a short count.  It's not really intuitive but
> >I'm going to have to fix it with another kernel patch.
> Thanks for your explanation.
> Sorry, i misunderstood the expected return value before.
> 
> 
> >For now we probably should just make the test accept both return values:
> >
> >	if (TEST_RETURN != sizeof(key_serial_t)&&
> >	TEST_RETURN != sizeof(key_ids)) {
> >		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu or %zu",
> >			TEST_RETURN, sizeof(key_serial_t), sizeof(key_ids));
> >	}
> >
> >Then once there is another kernel patch, I'll update the test to reference that
> >commit too, and accept only TEST_RETURN == sizeof(key_ids).
> Could we update the test to check both return values? as below:
> if (TEST_RETURN != sizeof(key_ids)) {
> 	/* keyctl_read() should return the size of buffer required, rather than the size
> 	 * of data read into buffer. This bug was introduced by the commit:
> 	 * e645016abc80 ("KEYS: fix writing past end of user-supplied buffer in keyring_read()")
> 	 */
> 	if (TEST_RETURN == sizeof(key_serial_t)) {
> 		tst_brk(TFAIL, "KEYCTL_READ returned %ld but expected %zu",
> 			TEST_RETURN, sizeof(key_ids));
> 	}
> 
> 	tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
> 		TEST_RETURN, sizeof(key_ids));
> }
> 
> We probably should expose the short return value as a bug, rather than ignore it.
> 

This will confuse people when the comment at the top of the file says
"regression test for commit e645016abc80", so people will waste time trying to
figure out why the test is still failing.  I'd prefer to wait to require the
full return value until I get a second fix applied, which can then be explicitly
referenced from the test.

Eric

[LTP] [PATCH] syscalls/keyctl06: Print TFAIL if keyring_read() returns wrong size

[Xiao Yang]

On 2017/10/19 1:19, Eric Biggers wrote:
> On Wed, Oct 18, 2017 at 02:25:25PM +0800, Xiao Yang wrote:
>>> It was actually pointed out yesterday that the short return value is a bug in
>>> the kernel patch.  The documented behavior of keyctl_read() (as well as the
>>> actual behavior for the other key types that implement it) is to return the full
>>> count on a short read, rather than a short count.  It's not really intuitive but
>>> I'm going to have to fix it with another kernel patch.
>> Thanks for your explanation.
>> Sorry, i misunderstood the expected return value before.
>>
>>
>>> For now we probably should just make the test accept both return values:
>>>
>>> 	if (TEST_RETURN != sizeof(key_serial_t)&&
>>> 	TEST_RETURN != sizeof(key_ids)) {
>>> 		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu or %zu",
>>> 			TEST_RETURN, sizeof(key_serial_t), sizeof(key_ids));
>>> 	}
>>>
>>> Then once there is another kernel patch, I'll update the test to reference that
>>> commit too, and accept only TEST_RETURN == sizeof(key_ids).
>> Could we update the test to check both return values? as below:
>> if (TEST_RETURN != sizeof(key_ids)) {
>> 	/* keyctl_read() should return the size of buffer required, rather than the size
>> 	 * of data read into buffer. This bug was introduced by the commit:
>> 	 * e645016abc80 ("KEYS: fix writing past end of user-supplied buffer in keyring_read()")
>> 	 */
>> 	if (TEST_RETURN == sizeof(key_serial_t)) {
>> 		tst_brk(TFAIL, "KEYCTL_READ returned %ld but expected %zu",
>> 			TEST_RETURN, sizeof(key_ids));
>> 	}
>>
>> 	tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
>> 		TEST_RETURN, sizeof(key_ids));
>> }
>>
>> We probably should expose the short return value as a bug, rather than ignore it.
>>
> This will confuse people when the comment at the top of the file says
> "regression test for commit e645016abc80", so people will waste time trying to
> figure out why the test is still failing.  I'd prefer to wait to require the
> full return value until I get a second fix applied, which can then be explicitly
> referenced from the test.
Hi Eric

OK, i got it.  I will send the v2 patch as you suggested.

Thanks,
Xiao Yang
> Eric
>
>
>

[LTP] [PATCH v2] syscalls/keyctl06: Accept two kinds of return values for the time being

[Xiao Yang]

The documented behavior of keyctl_read() is to return the full count
which is the size of buffer required.

After commit e645016abc80, keyctl_read() changs the behavior and returns
the short count which is the size of data read into buffer.

This issue is introduced by commit e645016abc80, and needs to be fixed.

For now we just accept both return values, and will accept only the full
return value as soon as the fix patch has been applied.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 testcases/kernel/syscalls/keyctl/keyctl06.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/keyctl/keyctl06.c b/testcases/kernel/syscalls/keyctl/keyctl06.c
index 8873431..0de70d2 100644
--- a/testcases/kernel/syscalls/keyctl/keyctl06.c
+++ b/testcases/kernel/syscalls/keyctl/keyctl06.c
@@ -55,7 +55,8 @@ static void do_test(void)
 	if (key_ids[0] != key_id_1 && key_ids[0] != key_id_2)
 		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
 
-	if (TEST_RETURN != sizeof(key_serial_t)) {
+	if (TEST_RETURN != sizeof(key_serial_t) &&
+	    TEST_RETURN != sizeof(key_ids)) {
 		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
 			TEST_RETURN, sizeof(key_serial_t));
 	}
-- 
1.8.3.1

[LTP] [PATCH v3] syscalls/keyctl06: Fix return value

[Xiao Yang]

If the user-supplied buffer is too small, the documented behavior of
keyctl_read() is to return the full count which is the size of buffer
required.  However, commit e645016abc80 ("KEYS: fix writing past end
of user-supplied buffer in keyring_read()") changs the behavior and
returns the short count which is the size of data read into buffer.

This issue was introduced by commit e645016abc80, and will be fixed by
the following patch:
http://www.spinics.net/lists/keyrings/msg03183.html

We should return the full count rather than the short count.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 testcases/kernel/syscalls/keyctl/keyctl06.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/keyctl/keyctl06.c b/testcases/kernel/syscalls/keyctl/keyctl06.c
index 8873431..c1b8d77 100644
--- a/testcases/kernel/syscalls/keyctl/keyctl06.c
+++ b/testcases/kernel/syscalls/keyctl/keyctl06.c
@@ -55,7 +55,7 @@ static void do_test(void)
 	if (key_ids[0] != key_id_1 && key_ids[0] != key_id_2)
 		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
 
-	if (TEST_RETURN != sizeof(key_serial_t)) {
+	if (TEST_RETURN != sizeof(key_ids)) {
 		tst_brk(TBROK, "KEYCTL_READ returned %ld but expected %zu",
 			TEST_RETURN, sizeof(key_serial_t));
 	}
-- 
1.8.3.1