syscalls/keyctl06: Print TFAIL if keyring_read() returns wrong size

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 <[email protected]>

---
 testcases/kernel/syscalls/keyctl/keyctl06.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

testcases/kernel/syscalls/keyctl/keyctl06.c +0 -0

index 8873431..bf30fb6 100644

Hi Xiao,

Quoted 25 lines · 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<[email protected]>

---
  testcases/kernel/syscalls/keyctl/keyctl06.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

testcases/kernel/syscalls/keyctl/keyctl06.c +0 -0

index 8873431..bf30fb6 100644
@@ -56,7 +56,7 @@ static void do_test(void)
  		tst_brk(TBROK, "KEYCTL_READ didn't return correct key ID");
 

testcases/kernel/syscalls/keyctl/keyctl06.c static void do_test(void) · line 62

Quoted 6 lines

testcases/kernel/syscalls/keyctl/keyctl06.c +1 -1

  	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

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 34 lines · Xiao Yang

Quoted 5 lines

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.

Quoted 10 lines

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));
}

Quoted 2 lines

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