[linux-kselftest/test,v1] apparmor: add AppArmor KUnit tests for policy unpack

Commit Message

Brendan Higgins Oct. 18, 2019, 12:18 a.m. UTC

From: Mike Salvatore <[e-mail]>

Add KUnit tests to test AppArmor unpacking of userspace policies.
AppArmor uses a serialized binary format for loading policies. To find
policy format documentation see
Documentation/admin-guide/LSM/apparmor.rst.

In order to write the tests against the policy unpacking code, some
static functions needed to be exposed for testing purposes. One of the
goals of this patch is to establish a pattern for which testing these
kinds of functions should be done in the future.

Signed-off-by: Brendan Higgins <[e-mail]>
Signed-off-by: Mike Salvatore <[e-mail]>
---
Sorry in advanced for emailing so many people. I suspect that there are
quite a few people who will be interested in this discussion - not so
much because of the AppArmor test itself - but because I think this
patch offers a good place to discuss how we should/will need to handle
code visibility for the purpose of writing tests.

I fully expect there to be a great deal of discussion on this patch
especially on the topic of visibility and how to expose symbols for
testing. Based on conversations I had before sending this patch out, I
expect to see a number of different suggestions on how to test symbols
that are not normally exposed.
---
 security/apparmor/Kconfig                 |  17 +
 security/apparmor/Makefile                |   1 +
 security/apparmor/include/policy_unpack.h |  53 ++
 security/apparmor/policy_unpack.c         |  61 +--
 security/apparmor/policy_unpack_test.c    | 607 ++++++++++++++++++++++
 5 files changed, 687 insertions(+), 52 deletions(-)
 create mode 100644 security/apparmor/policy_unpack_test.c

Comments

Iurii Zaikin Oct. 18, 2019, 12:33 a.m. UTC | #1

On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins
<[e-mail]> wrote:

> +config SECURITY_APPARMOR_TEST
> +       bool "Build KUnit tests for policy_unpack.c"
> +       default n
> +       depends on KUNIT && SECURITY_APPARMOR
> +       help
>
select SECURITY_APPARMOR ?
> +       KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> +       KUNIT_EXPECT_TRUE(test,
> +               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
I think this must be  KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);,
otherwise there could be a buffer overflow in memcmp. All tests that
follow such pattern
are suspect. Also, not sure about your stylistic preference for
KUNIT_EXPECT_TRUE(test,
               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
vs
KUNIT_EXPECT_EQ(test,
               0,
               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));

Brendan Higgins Oct. 18, 2019, 12:43 a.m. UTC | #2

On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> From: Mike Salvatore <[e-mail]>
> 
> Add KUnit tests to test AppArmor unpacking of userspace policies.
> AppArmor uses a serialized binary format for loading policies. To find
> policy format documentation see
> Documentation/admin-guide/LSM/apparmor.rst.
> 
> In order to write the tests against the policy unpacking code, some
> static functions needed to be exposed for testing purposes. One of the
> goals of this patch is to establish a pattern for which testing these
> kinds of functions should be done in the future.
> 
> Signed-off-by: Brendan Higgins <[e-mail]>
> Signed-off-by: Mike Salvatore <[e-mail]>
> ---
> Sorry in advanced for emailing so many people. I suspect that there are
> quite a few people who will be interested in this discussion - not so
> much because of the AppArmor test itself - but because I think this
> patch offers a good place to discuss how we should/will need to handle
> code visibility for the purpose of writing tests.
> 
> I fully expect there to be a great deal of discussion on this patch
> especially on the topic of visibility and how to expose symbols for
> testing. Based on conversations I had before sending this patch out, I
> expect to see a number of different suggestions on how to test symbols
> that are not normally exposed.
> ---
>  security/apparmor/Kconfig                 |  17 +
>  security/apparmor/Makefile                |   1 +
>  security/apparmor/include/policy_unpack.h |  53 ++
>  security/apparmor/policy_unpack.c         |  61 +--
>  security/apparmor/policy_unpack_test.c    | 607 ++++++++++++++++++++++
>  5 files changed, 687 insertions(+), 52 deletions(-)
>  create mode 100644 security/apparmor/policy_unpack_test.c
> 
> diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> index d8b1a360a636..632fbb873389 100644
> --- a/security/apparmor/Kconfig
> +++ b/security/apparmor/Kconfig
> @@ -66,3 +66,20 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES
>  	  Set the default value of the apparmor.debug kernel parameter.
>  	  When enabled, various debug messages will be logged to
>  	  the kernel message buffer.
> +
> +config SECURITY_APPARMOR_TEST
> +	bool "Build KUnit tests for policy_unpack.c"
> +	default n
> +	depends on KUNIT && SECURITY_APPARMOR

Ted, here is an example where doing select on direct dependencies is
tricky because SECURITY_APPARMOR has a number of indirect dependencies.

> +	help
> +	  This builds the AppArmor KUnit tests.
> +
> +	  KUnit tests run during boot and output the results to the debug log
> +	  in TAP format (http://testanything.org/). Only useful for kernel devs
> +	  running KUnit test harness and are not for inclusion into a
> +	  production build.
> +
> +	  For more information on KUnit and unit tests in general please refer
> +	  to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +	  If unsure, say N.
> diff --git a/security/apparmor/Makefile b/security/apparmor/Makefile
> index ff23fcfefe19..7ac1fc2dc045 100644
> --- a/security/apparmor/Makefile
> +++ b/security/apparmor/Makefile
> @@ -2,6 +2,7 @@
>  # Makefile for AppArmor Linux Security Module
>  #
>  obj-$(CONFIG_SECURITY_APPARMOR) += apparmor.o
> +obj-$(CONFIG_SECURITY_APPARMOR_TEST) += policy_unpack_test.o
>  
>  apparmor-y := apparmorfs.o audit.o capability.o task.o ipc.o lib.o match.o \
>                path.o domain.o policy.o policy_unpack.o procattr.o lsm.o \
> diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h
> index 46aefae918f5..dc272bafee2b 100644
> --- a/security/apparmor/include/policy_unpack.h
> +++ b/security/apparmor/include/policy_unpack.h
> @@ -16,6 +16,59 @@
>  #include <linux/dcache.h>
>  #include <linux/workqueue.h>
>  
> +/*
> + * The AppArmor interface treats data as a type byte followed by the
> + * actual data.  The interface has the notion of a a named entry
> + * which has a name (AA_NAME typecode followed by name string) followed by
> + * the entries typecode and data.  Named types allow for optional
> + * elements and extensions to be added and tested for without breaking
> + * backwards compatibility.
> + */
> +
> +enum aa_code {
> +	AA_U8,
> +	AA_U16,
> +	AA_U32,
> +	AA_U64,
> +	AA_NAME,		/* same as string except it is items name */
> +	AA_STRING,
> +	AA_BLOB,
> +	AA_STRUCT,
> +	AA_STRUCTEND,
> +	AA_LIST,
> +	AA_LISTEND,
> +	AA_ARRAY,
> +	AA_ARRAYEND,
> +};
> +
> +/*
> + * aa_ext is the read of the buffer containing the serialized profile.  The
> + * data is copied into a kernel buffer in apparmorfs and then handed off to
> + * the unpack routines.
> + */
> +struct aa_ext {
> +	void *start;
> +	void *end;
> +	void *pos;		/* pointer to current position in the buffer */
> +	u32 version;
> +};
> +
> +/* test if read will be in packed data bounds */
> +static inline bool inbounds(struct aa_ext *e, size_t size)
> +{
> +	return (size <= e->end - e->pos);
> +}
> +
> +size_t unpack_u16_chunk(struct aa_ext *e, char **chunk);
> +bool unpack_X(struct aa_ext *e, enum aa_code code);
> +bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name);
> +bool unpack_u32(struct aa_ext *e, u32 *data, const char *name);
> +bool unpack_u64(struct aa_ext *e, u64 *data, const char *name);
> +size_t unpack_array(struct aa_ext *e, const char *name);
> +size_t unpack_blob(struct aa_ext *e, char **blob, const char *name);
> +int unpack_str(struct aa_ext *e, const char **string, const char *name);
> +int unpack_strdup(struct aa_ext *e, char **string, const char *name);
> +
>  struct aa_load_ent {
>  	struct list_head list;
>  	struct aa_profile *new;
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 8cfc9493eefc..7811e9b7a154 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -36,43 +36,6 @@
>  #define v7	7
>  #define v8	8	/* full network masking */
>  
> -/*
> - * The AppArmor interface treats data as a type byte followed by the
> - * actual data.  The interface has the notion of a a named entry
> - * which has a name (AA_NAME typecode followed by name string) followed by
> - * the entries typecode and data.  Named types allow for optional
> - * elements and extensions to be added and tested for without breaking
> - * backwards compatibility.
> - */
> -
> -enum aa_code {
> -	AA_U8,
> -	AA_U16,
> -	AA_U32,
> -	AA_U64,
> -	AA_NAME,		/* same as string except it is items name */
> -	AA_STRING,
> -	AA_BLOB,
> -	AA_STRUCT,
> -	AA_STRUCTEND,
> -	AA_LIST,
> -	AA_LISTEND,
> -	AA_ARRAY,
> -	AA_ARRAYEND,
> -};
> -
> -/*
> - * aa_ext is the read of the buffer containing the serialized profile.  The
> - * data is copied into a kernel buffer in apparmorfs and then handed off to
> - * the unpack routines.
> - */
> -struct aa_ext {
> -	void *start;
> -	void *end;
> -	void *pos;		/* pointer to current position in the buffer */
> -	u32 version;
> -};
> -
>  /* audit callback for unpack fields */
>  static void audit_cb(struct audit_buffer *ab, void *va)
>  {
> @@ -194,12 +157,6 @@ struct aa_loaddata *aa_loaddata_alloc(size_t size)
>  	return d;
>  }
>  
> -/* test if read will be in packed data bounds */
> -static bool inbounds(struct aa_ext *e, size_t size)
> -{
> -	return (size <= e->end - e->pos);
> -}
> -
>  static void *kvmemdup(const void *src, size_t len)
>  {
>  	void *p = kvmalloc(len, GFP_KERNEL);
> @@ -216,7 +173,7 @@ static void *kvmemdup(const void *src, size_t len)
>   *
>   * Returns: the size of chunk found with the read head at the end of the chunk.
>   */
> -static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
> +size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)

Here and below are a number of parsing functions that we would like to
test.

I know that if we decide to just make them not-static, as I have done,
we will also want to properly namespace them. Nevertheless, I didn't
bother doing that yet since I suspect people may not actually want to
make them non-static.

In Mike's original version of this test[1], he just put the test in the
policy_unpack.c file thereby sidestepping the visibility issue entirely.
However, I am not a huge fan of mixing code and tests.

Another alternative, is just testing through the aa_unpack()[2] function
which is already visible; this has the advantage of not changing the
code under test and follows the principle of only testing public
interfaces, but has the downside that tests become more complicated as
each one has to produce a complete policy object.

I added Alan to this discussion because he has an idea for a function
called kunit_find_symbol()[3] which allows symbols to be looked up by name
so that they can be used.

Another possibility is to make a macro that makes the symbol only
visible when building with CONFIG_KUNIT enabled. It might look something
like this:

#if IS_ENABLED(CONFIG_KUNIT)
/**
 * __visible_for_testing - Makes a static function visible when testing.
 *
 * A macro that replaces the `static` specifier on functions and global
 * variables that is static when compiled normally and visible when compiled for
 * tests.
 */
#define __visible_for_testing
#else
#define __visible_for_testing static
#endif

and would be used like this:

Instead of

static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
{...}

we would have

__visible_for_testing size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
{...}

in this case we would still probably want to properly namespace the
symbol to avoid the possibility of collisions when testing.

There were some other ideas that were mentioned; however, I will leave
it to those people to present their ideas.

>  {
>  	size_t size = 0;
>  	void *pos = e->pos;
> @@ -237,7 +194,7 @@ static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
>  }
>  
>  /* unpack control byte */
> -static bool unpack_X(struct aa_ext *e, enum aa_code code)
> +bool unpack_X(struct aa_ext *e, enum aa_code code)
>  {
>  	if (!inbounds(e, 1))
>  		return 0;
> @@ -263,7 +220,7 @@ static bool unpack_X(struct aa_ext *e, enum aa_code code)
>   *
>   * Returns: 0 if either match fails, the read head does not move
>   */
> -static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
> +bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
>  {
>  	/*
>  	 * May need to reset pos if name or type doesn't match
> @@ -311,7 +268,7 @@ static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name)
>  	return 0;
>  }
>  
> -static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
> +bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
>  {
>  	void *pos = e->pos;
>  
> @@ -329,7 +286,7 @@ static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
>  	return 0;
>  }
>  
> -static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
> +bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
>  {
>  	void *pos = e->pos;
>  
> @@ -347,7 +304,7 @@ static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
>  	return 0;
>  }
>  
> -static size_t unpack_array(struct aa_ext *e, const char *name)
> +size_t unpack_array(struct aa_ext *e, const char *name)
>  {
>  	void *pos = e->pos;
>  
> @@ -365,7 +322,7 @@ static size_t unpack_array(struct aa_ext *e, const char *name)
>  	return 0;
>  }
>  
> -static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
> +size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
>  {
>  	void *pos = e->pos;
>  
> @@ -387,7 +344,7 @@ static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
>  	return 0;
>  }
>  
> -static int unpack_str(struct aa_ext *e, const char **string, const char *name)
> +int unpack_str(struct aa_ext *e, const char **string, const char *name)
>  {
>  	char *src_str;
>  	size_t size = 0;
> @@ -410,7 +367,7 @@ static int unpack_str(struct aa_ext *e, const char **string, const char *name)
>  	return 0;
>  }
>  
> -static int unpack_strdup(struct aa_ext *e, char **string, const char *name)
> +int unpack_strdup(struct aa_ext *e, char **string, const char *name)
>  {
>  	const char *tmp;
>  	void *pos = e->pos;
> diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
> new file mode 100644
> index 000000000000..3907f7d642e6
> --- /dev/null
> +++ b/security/apparmor/policy_unpack_test.c
> @@ -0,0 +1,607 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * KUnit tests for AppArmor's policy unpack.
> + */
> +
> +#include <kunit/test.h>
> +
> +#include "include/policy.h"
> +#include "include/policy_unpack.h"
> +
> +#define TEST_STRING_NAME "TEST_STRING"
> +#define TEST_STRING_DATA "testing"
> +#define TEST_STRING_BUF_OFFSET \
> +	(3 + strlen(TEST_STRING_NAME) + 1)
> +
> +#define TEST_U32_NAME "U32_TEST"
> +#define TEST_U32_DATA ((u32)0x01020304)
> +#define TEST_NAMED_U32_BUF_OFFSET \
> +	(TEST_STRING_BUF_OFFSET + 3 + strlen(TEST_STRING_DATA) + 1)
> +#define TEST_U32_BUF_OFFSET \
> +	(TEST_NAMED_U32_BUF_OFFSET + 3 + strlen(TEST_U32_NAME) + 1)
> +
> +#define TEST_U16_OFFSET (TEST_U32_BUF_OFFSET + 3)
> +#define TEST_U16_DATA ((u16)(TEST_U32_DATA >> 16))
> +
> +#define TEST_U64_NAME "U64_TEST"
> +#define TEST_U64_DATA ((u64)0x0102030405060708)
> +#define TEST_NAMED_U64_BUF_OFFSET (TEST_U32_BUF_OFFSET + sizeof(u32) + 1)
> +#define TEST_U64_BUF_OFFSET \
> +	(TEST_NAMED_U64_BUF_OFFSET + 3 + strlen(TEST_U64_NAME) + 1)
> +
> +#define TEST_BLOB_NAME "BLOB_TEST"
> +#define TEST_BLOB_DATA "\xde\xad\x00\xbe\xef"
> +#define TEST_BLOB_DATA_SIZE (ARRAY_SIZE(TEST_BLOB_DATA))
> +#define TEST_NAMED_BLOB_BUF_OFFSET (TEST_U64_BUF_OFFSET + sizeof(u64) + 1)
> +#define TEST_BLOB_BUF_OFFSET \
> +	(TEST_NAMED_BLOB_BUF_OFFSET + 3 + strlen(TEST_BLOB_NAME) + 1)
> +
> +#define TEST_ARRAY_NAME "ARRAY_TEST"
> +#define TEST_ARRAY_SIZE 16
> +#define TEST_NAMED_ARRAY_BUF_OFFSET \
> +	(TEST_BLOB_BUF_OFFSET + 5 + TEST_BLOB_DATA_SIZE)
> +#define TEST_ARRAY_BUF_OFFSET \
> +	(TEST_NAMED_ARRAY_BUF_OFFSET + 3 + strlen(TEST_ARRAY_NAME) + 1)
> +
> +struct policy_unpack_fixture {
> +	struct aa_ext *e;
> +	size_t e_size;
> +};
> +
> +struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf,
> +				   struct kunit *test, size_t buf_size)
> +{
> +	char *buf;
> +	struct aa_ext *e;
> +
> +	buf = kunit_kzalloc(test, buf_size, GFP_USER);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, buf);
> +
> +	e = kunit_kmalloc(test, sizeof(*e), GFP_USER);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, e);
> +
> +	e->start = buf;
> +	e->end = e->start + buf_size;
> +	e->pos = e->start;
> +
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_STRING_NAME) + 1;
> +	strcpy(buf + 3, TEST_STRING_NAME);
> +
> +	buf = e->start + TEST_STRING_BUF_OFFSET;
> +	*buf = AA_STRING;
> +	*(buf + 1) = strlen(TEST_STRING_DATA) + 1;
> +	strcpy(buf + 3, TEST_STRING_DATA);
> +
> +	buf = e->start + TEST_NAMED_U32_BUF_OFFSET;
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_U32_NAME) + 1;
> +	strcpy(buf + 3, TEST_U32_NAME);
> +	*(buf + 3 + strlen(TEST_U32_NAME) + 1) = AA_U32;
> +	*((u32 *)(buf + 3 + strlen(TEST_U32_NAME) + 2)) = TEST_U32_DATA;
> +
> +	buf = e->start + TEST_NAMED_U64_BUF_OFFSET;
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_U64_NAME) + 1;
> +	strcpy(buf + 3, TEST_U64_NAME);
> +	*(buf + 3 + strlen(TEST_U64_NAME) + 1) = AA_U64;
> +	*((u64 *)(buf + 3 + strlen(TEST_U64_NAME) + 2)) = TEST_U64_DATA;
> +
> +	buf = e->start + TEST_NAMED_BLOB_BUF_OFFSET;
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_BLOB_NAME) + 1;
> +	strcpy(buf + 3, TEST_BLOB_NAME);
> +	*(buf + 3 + strlen(TEST_BLOB_NAME) + 1) = AA_BLOB;
> +	*(buf + 3 + strlen(TEST_BLOB_NAME) + 2) = TEST_BLOB_DATA_SIZE;
> +	memcpy(buf + 3 + strlen(TEST_BLOB_NAME) + 6,
> +		TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE);
> +
> +	buf = e->start + TEST_NAMED_ARRAY_BUF_OFFSET;
> +	*buf = AA_NAME;
> +	*(buf + 1) = strlen(TEST_ARRAY_NAME) + 1;
> +	strcpy(buf + 3, TEST_ARRAY_NAME);
> +	*(buf + 3 + strlen(TEST_ARRAY_NAME) + 1) = AA_ARRAY;
> +	*((u16 *)(buf + 3 + strlen(TEST_ARRAY_NAME) + 2)) = TEST_ARRAY_SIZE;
> +
> +	return e;
> +}
> +
> +static int policy_unpack_test_init(struct kunit *test)
> +{
> +	size_t e_size = TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1;
> +	struct policy_unpack_fixture *puf;
> +
> +	puf = kunit_kmalloc(test, sizeof(*puf), GFP_USER);
> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, puf);
> +
> +	puf->e_size = e_size;
> +	puf->e = build_aa_ext_struct(puf, test, e_size);
> +
> +	test->priv = puf;
> +	return 0;
> +}
> +
> +static void policy_unpack_test_inbounds_when_inbounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +
> +	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, 0));
> +	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size / 2));
> +	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size));
> +}
> +
> +static void policy_unpack_test_inbounds_when_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +
> +	KUNIT_EXPECT_FALSE(test, inbounds(puf->e, puf->e_size + 1));
> +}
> +
> +static void policy_unpack_test_unpack_array_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	u16 array_size;
> +
> +	puf->e->pos += TEST_ARRAY_BUF_OFFSET;
> +
> +	array_size = unpack_array(puf->e, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +		puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_array_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_ARRAY_NAME;
> +	u16 array_size;
> +
> +	puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
> +
> +	array_size = unpack_array(puf->e, name);
> +
> +	KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +		puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_array_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_ARRAY_NAME;
> +	u16 array_size;
> +
> +	puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
> +	puf->e->end = puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16);
> +
> +	array_size = unpack_array(puf->e, name);
> +
> +	KUNIT_EXPECT_EQ(test, array_size, (u16)0);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +		puf->e->start + TEST_NAMED_ARRAY_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_blob_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *blob = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_BLOB_BUF_OFFSET;
> +	size = unpack_blob(puf->e, &blob, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> +	KUNIT_EXPECT_TRUE(test,
> +		memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> +}
> +
> +static void policy_unpack_test_unpack_blob_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *blob = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
> +	size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> +	KUNIT_EXPECT_TRUE(test,
> +		memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> +}
> +
> +static void policy_unpack_test_unpack_blob_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *blob = NULL;
> +	void *start;
> +	int size;
> +
> +	puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
> +	start = puf->e->pos;
> +	puf->e->end = puf->e->start + TEST_BLOB_BUF_OFFSET
> +		+ TEST_BLOB_DATA_SIZE - 1;
> +
> +	size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, 0);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
> +}
> +
> +static void policy_unpack_test_unpack_str_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char *string = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_STRING_BUF_OFFSET;
> +	size = unpack_str(puf->e, &string, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
> +	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
> +}
> +
> +static void policy_unpack_test_unpack_str_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char *string = NULL;
> +	size_t size;
> +
> +	size = unpack_str(puf->e, &string, TEST_STRING_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
> +	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
> +}
> +
> +static void policy_unpack_test_unpack_str_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char *string = NULL;
> +	void *start = puf->e->pos;
> +	int size;
> +
> +	puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
> +		+ strlen(TEST_STRING_DATA) - 1;
> +
> +	size = unpack_str(puf->e, &string, TEST_STRING_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, 0);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
> +}
> +
> +static void policy_unpack_test_unpack_strdup_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *string = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_STRING_BUF_OFFSET;
> +	size = unpack_strdup(puf->e, &string, NULL);
> +
> +	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
> +	KUNIT_EXPECT_FALSE(test,
> +			   ((uintptr_t)puf->e->start <= (uintptr_t)string)
> +			   && ((uintptr_t)string <= (uintptr_t)puf->e->end));
> +	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
> +}
> +
> +static void policy_unpack_test_unpack_strdup_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *string = NULL;
> +	size_t size;
> +
> +	size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
> +	KUNIT_EXPECT_FALSE(test,
> +			   ((uintptr_t)puf->e->start <= (uintptr_t)string)
> +			   && ((uintptr_t)string <= (uintptr_t)puf->e->end));
> +	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
> +}
> +
> +static void policy_unpack_test_unpack_strdup_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	void *start = puf->e->pos;
> +	char *string = NULL;
> +	int size;
> +
> +	puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
> +		+ strlen(TEST_STRING_DATA) - 1;
> +
> +	size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
> +
> +	KUNIT_EXPECT_EQ(test, size, 0);
> +	KUNIT_EXPECT_PTR_EQ(test, string, (char *)NULL);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
> +}
> +
> +static void policy_unpack_test_unpack_nameX_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +
> +	puf->e->pos += TEST_U32_BUF_OFFSET;
> +
> +	success = unpack_nameX(puf->e, AA_U32, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			    puf->e->start + TEST_U32_BUF_OFFSET + 1);
> +}
> +
> +static void policy_unpack_test_unpack_nameX_with_wrong_code(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +
> +	puf->e->pos += TEST_U32_BUF_OFFSET;
> +
> +	success = unpack_nameX(puf->e, AA_BLOB, NULL);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			    puf->e->start + TEST_U32_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_nameX_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U32_NAME;
> +	bool success;
> +
> +	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
> +
> +	success = unpack_nameX(puf->e, AA_U32, name);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			    puf->e->start + TEST_U32_BUF_OFFSET + 1);
> +}
> +
> +static void policy_unpack_test_unpack_nameX_with_wrong_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	static const char name[] = "12345678";
> +	bool success;
> +
> +	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
> +
> +	success = unpack_nameX(puf->e, AA_U32, name);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			    puf->e->start + TEST_NAMED_U32_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_u16_chunk_basic(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *chunk = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_U16_OFFSET;
> +	/*
> +	 * WARNING: For unit testing purposes, we're pushing puf->e->end past
> +	 * the end of the allocated memory. Doing anything other than comparing
> +	 * memory addresses is dangerous.
> +	 */
> +	puf->e->end += TEST_U16_DATA;
> +
> +	size = unpack_u16_chunk(puf->e, &chunk);
> +
> +	KUNIT_EXPECT_PTR_EQ(test, (void *)chunk,
> +			    puf->e->start + TEST_U16_OFFSET + 2);
> +	KUNIT_EXPECT_EQ(test, size, (size_t)TEST_U16_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, (void *)(chunk + TEST_U16_DATA));
> +}
> +
> +static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_1(
> +		struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *chunk = NULL;
> +	size_t size;
> +
> +	puf->e->pos = puf->e->end - 1;
> +
> +	size = unpack_u16_chunk(puf->e, &chunk);
> +
> +	KUNIT_EXPECT_EQ(test, size, (size_t)0);
> +	KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->end - 1);
> +}
> +
> +static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_2(
> +		struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	char *chunk = NULL;
> +	size_t size;
> +
> +	puf->e->pos += TEST_U16_OFFSET;
> +	/*
> +	 * WARNING: For unit testing purposes, we're pushing puf->e->end past
> +	 * the end of the allocated memory. Doing anything other than comparing
> +	 * memory addresses is dangerous.
> +	 */
> +	puf->e->end = puf->e->pos + TEST_U16_DATA - 1;
> +
> +	size = unpack_u16_chunk(puf->e, &chunk);
> +
> +	KUNIT_EXPECT_EQ(test, size, (size_t)0);
> +	KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->start + TEST_U16_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_u32_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +	u32 data;
> +
> +	puf->e->pos += TEST_U32_BUF_OFFSET;
> +
> +	success = unpack_u32(puf->e, &data, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_u32_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U32_NAME;
> +	bool success;
> +	u32 data;
> +
> +	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
> +
> +	success = unpack_u32(puf->e, &data, name);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_u32_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U32_NAME;
> +	bool success;
> +	u32 data;
> +
> +	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
> +	puf->e->end = puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32);
> +
> +	success = unpack_u32(puf->e, &data, name);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_NAMED_U32_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_u64_with_null_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +	u64 data;
> +
> +	puf->e->pos += TEST_U64_BUF_OFFSET;
> +
> +	success = unpack_u64(puf->e, &data, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_u64_with_name(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U64_NAME;
> +	bool success;
> +	u64 data;
> +
> +	puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
> +
> +	success = unpack_u64(puf->e, &data, name);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);
> +}
> +
> +static void policy_unpack_test_unpack_u64_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	const char name[] = TEST_U64_NAME;
> +	bool success;
> +	u64 data;
> +
> +	puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
> +	puf->e->end = puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64);
> +
> +	success = unpack_u64(puf->e, &data, name);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
> +			puf->e->start + TEST_NAMED_U64_BUF_OFFSET);
> +}
> +
> +static void policy_unpack_test_unpack_X_code_match(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success = unpack_X(puf->e, AA_NAME);
> +
> +	KUNIT_EXPECT_TRUE(test, success);
> +	KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start + 1);
> +}
> +
> +static void policy_unpack_test_unpack_X_code_mismatch(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success = unpack_X(puf->e, AA_STRING);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +	KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start);
> +}
> +
> +static void policy_unpack_test_unpack_X_out_of_bounds(struct kunit *test)
> +{
> +	struct policy_unpack_fixture *puf = test->priv;
> +	bool success;
> +
> +	puf->e->pos = puf->e->end;
> +	success = unpack_X(puf->e, AA_NAME);
> +
> +	KUNIT_EXPECT_FALSE(test, success);
> +}
> +
> +static struct kunit_case apparmor_policy_unpack_test_cases[] = {
> +	KUNIT_CASE(policy_unpack_test_inbounds_when_inbounds),
> +	KUNIT_CASE(policy_unpack_test_inbounds_when_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_array_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_array_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_array_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_blob_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_blob_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_blob_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_code),
> +	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_str_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_str_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_str_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_strdup_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_strdup_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_strdup_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_basic),
> +	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_1),
> +	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_2),
> +	KUNIT_CASE(policy_unpack_test_unpack_u32_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_u32_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_u32_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_u64_with_null_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_u64_with_name),
> +	KUNIT_CASE(policy_unpack_test_unpack_u64_out_of_bounds),
> +	KUNIT_CASE(policy_unpack_test_unpack_X_code_match),
> +	KUNIT_CASE(policy_unpack_test_unpack_X_code_mismatch),
> +	KUNIT_CASE(policy_unpack_test_unpack_X_out_of_bounds),
> +	{},
> +};
> +
> +static struct kunit_suite apparmor_policy_unpack_test_module = {
> +	.name = "apparmor_policy_unpack",
> +	.init = policy_unpack_test_init,
> +	.test_cases = apparmor_policy_unpack_test_cases,
> +};
> +
> +kunit_test_suite(apparmor_policy_unpack_test_module);
> -- 
> 2.23.0.866.gb869b98d4c-goog

[1] https://gitlab.com/apparmor/apparmor-kernel/blob/apparmor-kunit/security/apparmor/policy_unpack.c#L1066
[2] https://kunit-review.googlesource.com/c/linux/+/2809/3/security/apparmor/policy_unpack.c#b1054
[3] https://lore.kernel.org/linux-kselftest/[e-mail]/

Luis Chamberlain Oct. 18, 2019, 12:29 p.m. UTC | #3

On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> From: Mike Salvatore <[e-mail]>
> 
> In order to write the tests against the policy unpacking code, some
> static functions needed to be exposed for testing purposes. One of the
> goals of this patch is to establish a pattern for which testing these
> kinds of functions should be done in the future.

And you'd run into the same situation expressed elsewhere with kunit of
an issue of the kunit test as built-in working but if built as a module
then it would not work, given the lack of exports. Symbols namespaces
should resolve this [0], and we'd be careful where a driver imports this
namespace.

[0] https://lwn.net/Articles/798254/

  Luis

Theodore Ts'o Oct. 18, 2019, 4:25 p.m. UTC | #4

On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
> > +config SECURITY_APPARMOR_TEST
> > +	bool "Build KUnit tests for policy_unpack.c"
> > +	default n
> > +	depends on KUNIT && SECURITY_APPARMOR
> 
> Ted, here is an example where doing select on direct dependencies is
> tricky because SECURITY_APPARMOR has a number of indirect dependencies.

Well, that could be solved by adding a select on all of the indirect
dependencies.  I did get your point about the fact that we could have
cases where the indirect dependencies might conflict with one another.
That's going to be a tough situation regardless of whether we have a
sat-solver or a human who has to struggle with that situation.

It's also going to be a bit sad because it means that we won't be able
to create a single config that could be used to run all the kunit
tests when a user pushes a change to a Gerrit server for review.  :-/

I suppose that if we use a strict definition of "unit tests", and we
assume that all of the tests impacted by a change in foo/bar/baz.c
will be found in foo/bar/baz-test.c, or maybe foo/bar/*-test.c, we can
automate the generation of the kunitconfig file, perhaps?

The other sad bit about having mutually exclusive config options is
that we can't easily "run all KUinit tests" for some kind of test
spinner or zero-day bot.

I'm not sure there's a good solution to that issue, though.

    	     	       	    	     - Ted

Brendan Higgins Oct. 18, 2019, 9:41 p.m. UTC | #5

On Fri, Oct 18, 2019 at 9:25 AM Theodore Y. Ts'o <[e-mail]> wrote:
>
> On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
> > > +config SECURITY_APPARMOR_TEST
> > > +   bool "Build KUnit tests for policy_unpack.c"
> > > +   default n
> > > +   depends on KUNIT && SECURITY_APPARMOR
> >
> > Ted, here is an example where doing select on direct dependencies is
> > tricky because SECURITY_APPARMOR has a number of indirect dependencies.
>
> Well, that could be solved by adding a select on all of the indirect
> dependencies.  I did get your point about the fact that we could have

In this particular case that would work.

> cases where the indirect dependencies might conflict with one another.
> That's going to be a tough situation regardless of whether we have a
> sat-solver or a human who has to struggle with that situation.

But yeah, that's the real problem.

> It's also going to be a bit sad because it means that we won't be able
> to create a single config that could be used to run all the kunit
> tests when a user pushes a change to a Gerrit server for review.  :-/

Yeah...well, we can do the next best thing and generate a set of
kunitconfigs that in sum will run all the tests. Not nearly as nice,
but it's the next best thing, right? If you think about it, it's
really not all that different from the eventual goal of having many
independent test binaries.

> I suppose that if we use a strict definition of "unit tests", and we
> assume that all of the tests impacted by a change in foo/bar/baz.c
> will be found in foo/bar/baz-test.c, or maybe foo/bar/*-test.c, we can
> automate the generation of the kunitconfig file, perhaps?

Possibly. I have some friends on the TAP team (automated testing team
within Google), and it sounds like that is actually a pretty hard
problem, but something that is at least possible. Still, it would be
nice to have a way to periodically run all the tests.

> The other sad bit about having mutually exclusive config options is
> that we can't easily "run all KUinit tests" for some kind of test
> spinner or zero-day bot.
>
> I'm not sure there's a good solution to that issue, though.

I think, as I mentioned above, the best we can do is probably have a
thing which generates a set of kunitconfigs that in sum will run all
the tests.

Thoughts?

Alan Maguire Oct. 19, 2019, 12:56 p.m. UTC | #6

On Fri, 18 Oct 2019, Luis Chamberlain wrote:

> On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> > From: Mike Salvatore <[e-mail]>
> > 
> > In order to write the tests against the policy unpacking code, some
> > static functions needed to be exposed for testing purposes. One of the
> > goals of this patch is to establish a pattern for which testing these
> > kinds of functions should be done in the future.
> 
> And you'd run into the same situation expressed elsewhere with kunit of
> an issue of the kunit test as built-in working but if built as a module
> then it would not work, given the lack of exports. Symbols namespaces
> should resolve this [0], and we'd be careful where a driver imports this
> namespace.
> 
> [0] https://lwn.net/Articles/798254/
>

Thanks for the link! Looks interesting for us definitely!

WRT adding tests, I think what we're aiming at is a set of best practices 
to advise test developers using KUnit, while attempting to minimize 
side-effects of any changes we need to make to support testability.

One aspect of this we probably have to consider is inlining of code. For 
cases like this where the functions are small and are called in a small 
number of cases, any testability changes we make may push a 
previously-inlined function to not be inlined, with potential performance 
side-effects for the subsystem.  In such cases, I wonder if the right 
answer would be to suggest actually defining the functions as 
inline in the header file? That way the compiler still gets to decide (as 
opposed to __always_inline), and we don't perturb performance too much.

Thanks!

Alan

>   Luis
>

Luis Chamberlain Oct. 19, 2019, 6:36 p.m. UTC | #7

On Sat, Oct 19, 2019 at 01:56:01PM +0100, Alan Maguire wrote:
> On Fri, 18 Oct 2019, Luis Chamberlain wrote:
> 
> > On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> > > From: Mike Salvatore <[e-mail]>
> > > 
> > > In order to write the tests against the policy unpacking code, some
> > > static functions needed to be exposed for testing purposes. One of the
> > > goals of this patch is to establish a pattern for which testing these
> > > kinds of functions should be done in the future.
> > 
> > And you'd run into the same situation expressed elsewhere with kunit of
> > an issue of the kunit test as built-in working but if built as a module
> > then it would not work, given the lack of exports. Symbols namespaces
> > should resolve this [0], and we'd be careful where a driver imports this
> > namespace.
> > 
> > [0] https://lwn.net/Articles/798254/
> >
> 
> Thanks for the link! Looks interesting for us definitely!
> 
> WRT adding tests, I think what we're aiming at is a set of best practices 
> to advise test developers using KUnit, while attempting to minimize 
> side-effects of any changes we need to make to support testability.
> 
> One aspect of this we probably have to consider is inlining of code. 

Sure. Makes sense.

  Luis

Brendan Higgins Oct. 24, 2019, 12:42 a.m. UTC | #8

On Sat, Oct 19, 2019 at 5:56 AM Alan Maguire <[e-mail]> wrote:
>
> On Fri, 18 Oct 2019, Luis Chamberlain wrote:
>
> > On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> > > From: Mike Salvatore <[e-mail]>
> > >
> > > In order to write the tests against the policy unpacking code, some
> > > static functions needed to be exposed for testing purposes. One of the
> > > goals of this patch is to establish a pattern for which testing these
> > > kinds of functions should be done in the future.
> >
> > And you'd run into the same situation expressed elsewhere with kunit of
> > an issue of the kunit test as built-in working but if built as a module
> > then it would not work, given the lack of exports. Symbols namespaces
> > should resolve this [0], and we'd be careful where a driver imports this
> > namespace.
> >
> > [0] https://lwn.net/Articles/798254/

Maybe I am not understanding how the symbol namespaces work, but it
seems that it doesn't actually solve our problem, at least not all of
it.

First off this doesn't solve the problem for when a piece of code is
included as a module; it also does not address the problem for symbols
that would not normally be exported. Also, I think we still expect a
symbol that is not static to have an appropriate prefix, right? As in,
it is *not* okay to have a non-static symbol in apparmor called
"inbounds", correct?

> WRT adding tests, I think what we're aiming at is a set of best practices
> to advise test developers using KUnit, while attempting to minimize
> side-effects of any changes we need to make to support testability.
>
> One aspect of this we probably have to consider is inlining of code. For
> cases like this where the functions are small and are called in a small
> number of cases, any testability changes we make may push a
> previously-inlined function to not be inlined, with potential performance
> side-effects for the subsystem.  In such cases, I wonder if the right
> answer would be to suggest actually defining the functions as
> inline in the header file? That way the compiler still gets to decide (as
> opposed to __always_inline), and we don't perturb performance too much.

That's a really good point. Okay, so it seems that making the symbols
public when not testing is probably not okay on its own. If we are
going to do that, we probably need to do something like what you are
suggesting.

With that, I think the best solution in this case will be the
"__visible_for_testing" route. It has no overhead when testing is
turned off (in fact it is no different in anyway when testing is
turned off). The downsides I see are:

1) You may not be able to test non-module code not compiled for
testing later with the test modules that Alan is working on (But the
only way I think that will work is by preventing the symbol from being
inlined, right?).

2) I think "__visible_for_testing" will be prone to abuse. Here, I
think there are reasons why we might want to expose these symbols for
testing, but not otherwise. Nevertheless, I think most symbols that
should be tested should probably be made visible by default. Since you
usually only want to test your public interfaces. I could very well
see this getting used as a kludge that gets used far too frequently.

Nevertheless, based on Alan's point, I suspect it, for this case at
least, will likely be the least painful.

How do people feel about that?

Luis Chamberlain Oct. 24, 2019, 10:15 a.m. UTC | #9

On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
> On Sat, Oct 19, 2019 at 5:56 AM Alan Maguire <[e-mail]> wrote:
> >
> > On Fri, 18 Oct 2019, Luis Chamberlain wrote:
> >
> > > On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
> > > > From: Mike Salvatore <[e-mail]>
> > > >
> > > > In order to write the tests against the policy unpacking code, some
> > > > static functions needed to be exposed for testing purposes. One of the
> > > > goals of this patch is to establish a pattern for which testing these
> > > > kinds of functions should be done in the future.
> > >
> > > And you'd run into the same situation expressed elsewhere with kunit of
> > > an issue of the kunit test as built-in working but if built as a module
> > > then it would not work, given the lack of exports. Symbols namespaces
> > > should resolve this [0], and we'd be careful where a driver imports this
> > > namespace.
> > >
> > > [0] https://lwn.net/Articles/798254/
> 
> Maybe I am not understanding how the symbol namespaces work, but it
> seems that it doesn't actually solve our problem, at least not all of
> it.
> 
> First off this doesn't solve the problem for when a piece of code is
> included as a module; it also does not address the problem for symbols
> that would not normally be exported.

The suggestion is that since exporting certain symbols may not be wise,
exporting them for say a test namespace may make more sense. Then
the hacks don't need to be used for the lookup, therefore decreasing
test / complexity time. This would only make sense if there really is
no performance penalty known for having the symbol be exported.

> > WRT adding tests, I think what we're aiming at is a set of best practices
> > to advise test developers using KUnit, while attempting to minimize
> > side-effects of any changes we need to make to support testability.
> >
> > One aspect of this we probably have to consider is inlining of code. For
> > cases like this where the functions are small and are called in a small
> > number of cases, any testability changes we make may push a
> > previously-inlined function to not be inlined, with potential performance
> > side-effects for the subsystem.  In such cases, I wonder if the right
> > answer would be to suggest actually defining the functions as
> > inline in the header file? That way the compiler still gets to decide (as
> > opposed to __always_inline), and we don't perturb performance too much.
> 
> That's a really good point. Okay, so it seems that making the symbols
> public when not testing is probably not okay on its own. If we are
> going to do that, we probably need to do something like what you are
> suggesting.

If namespaces were to be used, and a consideration is that certain
symbols should not be public as otherwise there would be a real
perfomance hit, having a macro which only exports the namespace
if a build option is enabled would suffice as well. That is, a
no-op if the test feature is disabled, exported into a namespace
otherwise. But if a new build option were to be considered to help build
a different test kernel it begs the question if we just want or not a
full sweaping -fno-inline could be considered on the top level
Makefile when testing. The cost is a bloat the kernel, and it may
also produce slightly different runtimes. With a test specific
symbol namespace thing, we'd only export to a namespace those
symbols being tested.

FWIW we currently only enable -fno-inline-functions-called-once when we
have CONFIG_DEBUG_SECTION_MISMATCH enabled.

Your suggestion below with __visible_for_testing aligns with the gcc
flags I mentioned.

> With that, I think the best solution in this case will be the
> "__visible_for_testing" route. It has no overhead when testing is
> turned off (in fact it is no different in anyway when testing is
> turned off). The downsides I see are:
> 
> 1) You may not be able to test non-module code not compiled for
> testing later with the test modules that Alan is working on (But the
> only way I think that will work is by preventing the symbol from being
> inlined, right?).
> 
> 2) I think "__visible_for_testing" will be prone to abuse. Here, I
> think there are reasons why we might want to expose these symbols for
> testing, but not otherwise. Nevertheless, I think most symbols that
> should be tested should probably be made visible by default. Since you
> usually only want to test your public interfaces. I could very well
> see this getting used as a kludge that gets used far too frequently.

There are two parts to your statement on 2):

  a) possible abuse of say __visible_for_testing
  b) you typically only want to test your public interfaces

For a) I think a proper module testing namespace thing would less
likely be prone to abuse over somethingmore generic. It has the
penalty of always being enabled (unless you kconfig it to allow
you to disable it).

While I don't fully agree with b) I think for the most part it remains
true. But I'd agree more with this:

  Most functions you want to test likely don't have a performance
  criteria to be kept inline

And for those situations I think a wrapper kconfig to convert a
subsystem specific namespace call to be a nop would make sense.

  Luis

> Nevertheless, based on Alan's point, I suspect it, for this case at
> least, will likely be the least painful.
> 
> How do people feel about that?

Kees Cook Oct. 30, 2019, 6:59 p.m. UTC | #10

On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
> On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins
> <[e-mail]> wrote:
> 
> > +config SECURITY_APPARMOR_TEST
> > +       bool "Build KUnit tests for policy_unpack.c"
> > +       default n

New options already already default n, this can be left off.

> > +       depends on KUNIT && SECURITY_APPARMOR
> > +       help
> >
> select SECURITY_APPARMOR ?

"select" doesn't enforce dependencies, so just a "depends ..." is
correct.

> > +       KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> > +       KUNIT_EXPECT_TRUE(test,
> > +               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> I think this must be  KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);,
> otherwise there could be a buffer overflow in memcmp. All tests that
> follow such pattern

Agreed.

> are suspect. Also, not sure about your stylistic preference for
> KUNIT_EXPECT_TRUE(test,
>                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> vs
> KUNIT_EXPECT_EQ(test,
>                0,
>                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));

I like == 0.

Kees Cook Oct. 30, 2019, 7:02 p.m. UTC | #11

On Fri, Oct 18, 2019 at 02:41:38PM -0700, Brendan Higgins wrote:
> On Fri, Oct 18, 2019 at 9:25 AM Theodore Y. Ts'o <[e-mail]> wrote:
> >
> > On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
> > > > +config SECURITY_APPARMOR_TEST
> > > > +   bool "Build KUnit tests for policy_unpack.c"
> > > > +   default n
> > > > +   depends on KUNIT && SECURITY_APPARMOR
> > >
> > > Ted, here is an example where doing select on direct dependencies is
> > > tricky because SECURITY_APPARMOR has a number of indirect dependencies.
> >
> > Well, that could be solved by adding a select on all of the indirect
> > dependencies.  I did get your point about the fact that we could have
> 
> In this particular case that would work.
> 
> > cases where the indirect dependencies might conflict with one another.
> > That's going to be a tough situation regardless of whether we have a
> > sat-solver or a human who has to struggle with that situation.
> 
> But yeah, that's the real problem.

I think at this stage we want to make it _possible_ to write tests
sanely without causing all kinds of headaches. I think "build all the
tests" can just be a function of "allmodconfig" and leave it at that
until we have cases we really need to deal with.

Kees Cook Oct. 30, 2019, 7:09 p.m. UTC | #12

On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
> On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
> > With that, I think the best solution in this case will be the
> > "__visible_for_testing" route. It has no overhead when testing is
> > turned off (in fact it is no different in anyway when testing is
> > turned off). The downsides I see are:
> > 
> > 1) You may not be able to test non-module code not compiled for
> > testing later with the test modules that Alan is working on (But the
> > only way I think that will work is by preventing the symbol from being
> > inlined, right?).
> > 
> > 2) I think "__visible_for_testing" will be prone to abuse. Here, I
> > think there are reasons why we might want to expose these symbols for
> > testing, but not otherwise. Nevertheless, I think most symbols that
> > should be tested should probably be made visible by default. Since you
> > usually only want to test your public interfaces. I could very well
> > see this getting used as a kludge that gets used far too frequently.
> 
> There are two parts to your statement on 2):
> 
>   a) possible abuse of say __visible_for_testing

I really don't like the idea of littering the kernel with these. It'll
also require chunks in header files wrapped in #ifdefs. This is really
ugly.

>   b) you typically only want to test your public interfaces

True, but being able to test the little helper functions is a nice
starting point and a good building block.

Why can't unit tests live with the code they're testing? They're already
logically tied together; what's the harm there? This needn't be the case
for ALL tests, etc. The test driver could still live externally. The
test in the other .c would just have exported functions... ?

Iurii Zaikin Oct. 30, 2019, 8:11 p.m. UTC | #13

> Why can't unit tests live with the code they're testing? They're already
> logically tied together; what's the harm there? This needn't be the case
> for ALL tests, etc. The test driver could still live externally. The
> test in the other .c would just have exported functions... ?
>
Curiously enough, this approach has been adopted by D 2.0 where unittests are
members of the class under test:  https://digitalmars.com/d/2.0/unittest.html
but such approach is not mainstream.
I personally like the idea of testing the lowest level bits in isolation even if
they are not a part of any interface. I think that specifying the
interface using
unit tests and ensuring implementation correctness are complementary but
I haven't had much luck arguing this with our esteemed colleagues.

John Johansen Oct. 31, 2019, 1:37 a.m. UTC | #14

On 10/30/19 12:09 PM, Kees Cook wrote:
> On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
>> On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
>>> With that, I think the best solution in this case will be the
>>> "__visible_for_testing" route. It has no overhead when testing is
>>> turned off (in fact it is no different in anyway when testing is
>>> turned off). The downsides I see are:
>>>
>>> 1) You may not be able to test non-module code not compiled for
>>> testing later with the test modules that Alan is working on (But the
>>> only way I think that will work is by preventing the symbol from being
>>> inlined, right?).
>>>
>>> 2) I think "__visible_for_testing" will be prone to abuse. Here, I
>>> think there are reasons why we might want to expose these symbols for
>>> testing, but not otherwise. Nevertheless, I think most symbols that
>>> should be tested should probably be made visible by default. Since you
>>> usually only want to test your public interfaces. I could very well
>>> see this getting used as a kludge that gets used far too frequently.
>>
>> There are two parts to your statement on 2):
>>
>>   a) possible abuse of say __visible_for_testing
> 
> I really don't like the idea of littering the kernel with these. It'll
> also require chunks in header files wrapped in #ifdefs. This is really
> ugly.
> 
>>   b) you typically only want to test your public interfaces
>
I would disagree with this. Helper/lib functions benefit from testing,
and ensuring they work as expected. Having tests on these helps catch
errors when you fix bugs or make improvements. If you want its indirect
testing of public interfaces.

> True, but being able to test the little helper functions is a nice
> starting point and a good building block.
> 
yeah its a nice building block

> Why can't unit tests live with the code they're testing? They're already
> logically tied together; what's the harm there? This needn't be the case
> for ALL tests, etc. The test driver could still live externally. The
> test in the other .c would just have exported functions... ?
> 

they can, its my preference too. Or if the tests must live separate I
don't even mind the abomination of including a test .c that contains
the tests for the private fns only.

In the end though, I just want to see more unit testing and will happily
take what ever the community decides is the way to go.

John Johansen Oct. 31, 2019, 1:40 a.m. UTC | #15

On 10/30/19 1:11 PM, Iurii Zaikin wrote:
>> Why can't unit tests live with the code they're testing? They're already
>> logically tied together; what's the harm there? This needn't be the case
>> for ALL tests, etc. The test driver could still live externally. The
>> test in the other .c would just have exported functions... ?
>>
> Curiously enough, this approach has been adopted by D 2.0 where unittests are
> members of the class under test:  https://digitalmars.com/d/2.0/unittest.html
> but such approach is not mainstream.
> I personally like the idea of testing the lowest level bits in isolation even if
> they are not a part of any interface. I think that specifying the
> interface using
> unit tests and ensuring implementation correctness are complementary but

fwiw this is my preferred approach as well

> I haven't had much luck arguing this with our esteemed colleagues.
> 

surprise, surprise /s

Brendan Higgins Oct. 31, 2019, 9:01 a.m. UTC | #16

On Wed, Oct 30, 2019 at 12:02 PM Kees Cook <[e-mail]> wrote:
>
> On Fri, Oct 18, 2019 at 02:41:38PM -0700, Brendan Higgins wrote:
> > On Fri, Oct 18, 2019 at 9:25 AM Theodore Y. Ts'o <[e-mail]> wrote:
> > >
> > > On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
> > > > > +config SECURITY_APPARMOR_TEST
> > > > > +   bool "Build KUnit tests for policy_unpack.c"
> > > > > +   default n
> > > > > +   depends on KUNIT && SECURITY_APPARMOR
> > > >
> > > > Ted, here is an example where doing select on direct dependencies is
> > > > tricky because SECURITY_APPARMOR has a number of indirect dependencies.
> > >
> > > Well, that could be solved by adding a select on all of the indirect
> > > dependencies.  I did get your point about the fact that we could have
> >
> > In this particular case that would work.
> >
> > > cases where the indirect dependencies might conflict with one another.
> > > That's going to be a tough situation regardless of whether we have a
> > > sat-solver or a human who has to struggle with that situation.
> >
> > But yeah, that's the real problem.
>
> I think at this stage we want to make it _possible_ to write tests
> sanely without causing all kinds of headaches. I think "build all the
> tests" can just be a function of "allmodconfig" and leave it at that
> until we have cases we really need to deal with.

That...appears to work. I really can't see any reason why that isn't
good enough for now.

I am surprised that this hasn't been suggested yet.

Thanks!

Brendan Higgins Oct. 31, 2019, 9:17 a.m. UTC | #17

On Wed, Oct 30, 2019 at 12:09 PM Kees Cook <[e-mail]> wrote:
>
> On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
> > On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
> > > With that, I think the best solution in this case will be the
> > > "__visible_for_testing" route. It has no overhead when testing is
> > > turned off (in fact it is no different in anyway when testing is
> > > turned off). The downsides I see are:
> > >
> > > 1) You may not be able to test non-module code not compiled for
> > > testing later with the test modules that Alan is working on (But the
> > > only way I think that will work is by preventing the symbol from being
> > > inlined, right?).
> > >
> > > 2) I think "__visible_for_testing" will be prone to abuse. Here, I
> > > think there are reasons why we might want to expose these symbols for
> > > testing, but not otherwise. Nevertheless, I think most symbols that
> > > should be tested should probably be made visible by default. Since you
> > > usually only want to test your public interfaces. I could very well
> > > see this getting used as a kludge that gets used far too frequently.
> >
> > There are two parts to your statement on 2):
> >
> >   a) possible abuse of say __visible_for_testing
>
> I really don't like the idea of littering the kernel with these. It'll

Yeah, I kind of hope that it would make people think more
intentionally about what is a public interface so that they wouldn't
litter the kernel with those. But I agree that in the world where
people *didn't* do that. Lots of these sprinkled around would be
annoying.

> also require chunks in header files wrapped in #ifdefs. This is really

Why would it require header files wrapped in #ifdefs?

We could put all the ifdeffery logic in the __visible_for_testing
macro so that nothing in the original code has to change except for
adding an #include and replacing a couple of `static`s with
`__visible_for_testing`.

> ugly.
>
> >   b) you typically only want to test your public interfaces
>
> True, but being able to test the little helper functions is a nice
> starting point and a good building block.

Yeah, I think I have come to accept that. We can argue about how this
should change and how people need to learn to be more intentional
about which interfaces are public and many other high minded ideas,
but when it comes down to it, we need to provide a starting point that
is easy.

If our nice starting point becomes a problem, we can always improve it later.

> Why can't unit tests live with the code they're testing? They're already
> logically tied together; what's the harm there? This needn't be the case
> for ALL tests, etc. The test driver could still live externally. The
> test in the other .c would just have exported functions... ?

Well, for one, it totally tanks certain cases for building KUnit tests
as modules. I don't care about this point *too* much personally, but I
accept that there are others that want this, and I don't want to make
these people's lives too difficult.

The main reason I care, however, is just that I think it looks bad to
me. The file that these tests were in was already pretty long, and the
tests made it even longer. So that makes the tests harder to find. If
all tests are in a *-test.c file, then it becomes really easy to find
all of your tests. Admittedly, this is a pretty minor point. Honestly,
the main reason it looks bad to me, is because it is different from
what I am used to, which, I know, is not a great reason.

Brendan Higgins Oct. 31, 2019, 9:33 a.m. UTC | #18

On Wed, Oct 30, 2019 at 1:12 PM Iurii Zaikin <[e-mail]> wrote:
>
> > Why can't unit tests live with the code they're testing? They're already
> > logically tied together; what's the harm there? This needn't be the case
> > for ALL tests, etc. The test driver could still live externally. The
> > test in the other .c would just have exported functions... ?
> >
> Curiously enough, this approach has been adopted by D 2.0 where unittests are
> members of the class under test:  https://digitalmars.com/d/2.0/unittest.html

Thanks for pointing this out, Iurii, that actually looks pretty cool.
I still personally prefer keeping tests and code separate, but if we
decide to go the route of mixing tests and code, maybe we might want
to use this as a model.

> but such approach is not mainstream.
> I personally like the idea of testing the lowest level bits in isolation even if
> they are not a part of any interface. I think that specifying the
> interface using
> unit tests and ensuring implementation correctness are complementary but
> I haven't had much luck arguing this with our esteemed colleagues.

So I think this is a very subtle point which is very widely
misunderstood. Most people write code and then write their tests,
following this practice along with only testing public interfaces
often causes people to just not test all of their code, which is
wrong.

The idea of only testing public interfaces is supposed to make people
think more carefully about what the composite layers of the program
is. If you are having difficulty getting decent coverage by only
testing your public interfaces, then it likely tells you that you have
one of two problems:

1) You have code that you don't need, and you should remove it.

2) One of the layers in your program is too think, and you should
introduce a new layer with a new public interface that you can test
through.

I think the second point here is problematic with how C is written in
the kernel. We don't really have any concept of public vs. private
inside the kernel outside of static vs. not static, which is much more
restricted.

Kees Cook Oct. 31, 2019, 6:40 p.m. UTC | #19

On Thu, Oct 31, 2019 at 02:33:32AM -0700, Brendan Higgins wrote:
> 2) One of the layers in your program is too think, and you should
> introduce a new layer with a new public interface that you can test
> through.
> 
> I think the second point here is problematic with how C is written in
> the kernel. We don't really have any concept of public vs. private
> inside the kernel outside of static vs. not static, which is much more
> restricted.

I don't find "2" to be a convincing argument (as you hint a bit at in
the next paragraph)_. There are lots of things code is depending on
(especially given the kernel's coding style guides about breaking up
large functions into little ones), that you want to test to make sure
is working correctly that has no public exposure, and you want to test
those helper's corner cases which might be hard to (currently) reach via
the higher level public APIs.

Alan Maguire Nov. 1, 2019, 12:30 p.m. UTC | #20

On Thu, 31 Oct 2019, Brendan Higgins wrote:

> On Wed, Oct 30, 2019 at 12:09 PM Kees Cook <[e-mail]> wrote:
> >
> > On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
> > > On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
> > > > With that, I think the best solution in this case will be the
> > > > "__visible_for_testing" route. It has no overhead when testing is
> > > > turned off (in fact it is no different in anyway when testing is
> > > > turned off). The downsides I see are:
> > > >
> > > > 1) You may not be able to test non-module code not compiled for
> > > > testing later with the test modules that Alan is working on (But the
> > > > only way I think that will work is by preventing the symbol from being
> > > > inlined, right?).
> > > >
> > > > 2) I think "__visible_for_testing" will be prone to abuse. Here, I
> > > > think there are reasons why we might want to expose these symbols for
> > > > testing, but not otherwise. Nevertheless, I think most symbols that
> > > > should be tested should probably be made visible by default. Since you
> > > > usually only want to test your public interfaces. I could very well
> > > > see this getting used as a kludge that gets used far too frequently.
> > >
> > > There are two parts to your statement on 2):
> > >
> > >   a) possible abuse of say __visible_for_testing
> >
> > I really don't like the idea of littering the kernel with these. It'll
> 
> Yeah, I kind of hope that it would make people think more
> intentionally about what is a public interface so that they wouldn't
> litter the kernel with those. But I agree that in the world where
> people *didn't* do that. Lots of these sprinkled around would be
> annoying.
> 
> > also require chunks in header files wrapped in #ifdefs. This is really
> 
> Why would it require header files wrapped in #ifdefs?
> 
> We could put all the ifdeffery logic in the __visible_for_testing
> macro so that nothing in the original code has to change except for
> adding an #include and replacing a couple of `static`s with
> `__visible_for_testing`.
> 

FWIW I think this approach, if used sparingly, is fine.  However I'd
propose a hierarchy of options when looking to expose interfaces for 
testing.

1. For small, largely self-contained functions, move their definitions 
from .c files to a .h file where those functions are defined as "static 
inline".  That way the original code and tests can included them and we 
have solved function availability for both the cases where the tests are 
built-in and compiled as a module.  The apparmor interfaces here seem to 
be candidates for that approach.

2. For more complex cases, __visible_for_testing (for built-in visbility)
and some sort of equivalent EXPORT_FOR_TESTING (for module 
visibility) would work, or the kunit_find_symbol() based lookup approach I 
suggested in the module patches.  Either of these allows for building 
tests as modules or builtin.

3. For some cases, module support will probably be impossible or difficult 
to maintain.  In such cases, builtin tests make most sense so any 
questions about symbol visibility would largely concern changing static 
definitions to be __visibile_for_testing, with no need for any symbol 
export for module visibility.


> > ugly.
> >
> > >   b) you typically only want to test your public interfaces
> >
> > True, but being able to test the little helper functions is a nice
> > starting point and a good building block.
> 
> Yeah, I think I have come to accept that. We can argue about how this
> should change and how people need to learn to be more intentional
> about which interfaces are public and many other high minded ideas,
> but when it comes down to it, we need to provide a starting point that
> is easy.
> 
> If our nice starting point becomes a problem, we can always improve it later.
> 
> > Why can't unit tests live with the code they're testing? They're already
> > logically tied together; what's the harm there? This needn't be the case
> > for ALL tests, etc. The test driver could still live externally. The
> > test in the other .c would just have exported functions... ?
> 
> Well, for one, it totally tanks certain cases for building KUnit tests
> as modules. I don't care about this point *too* much personally, but I
> accept that there are others that want this, and I don't want to make
> these people's lives too difficult.
>

Appreciated.  I think at this point it might be useful to lay out my 
thinking on why being able to build tests as modules may be helpful moving 
forward.

- First and foremost, if the functionality itself is predominantly 
delivered in module form, or indeed is simply tristate, having a way to 
test kernel code when built as a module seems to me to be necessary. To 
test module code with built-in test code seems broken, and even if it 
could be made to work we'd end up having to invent a bunch of the mechanisms
we'd need for building tests as modules anyway.

- Running tests on demand.  From previous discussions, I think this is 
wanted for kselftest, and if we have a set of modules with a conventional 
prefix (e.g. kunit-*), running tests becomes simply a "find + modprobe" in 
the kernel module tree.  Results could be harvested from debugfs (I have a 
WIP patch to store logging data in the per-test data structures such that 
"cat /sys/kernel/debug/kunit-results/kunit-foo" will display results for 
that test suite).  There are other ways to achieve this goal, and it's 
a crude method without any test selection beyond which modules are 
loaded, but this path is noticeably shorter to having a simple way to 
execute tests in a kselftest-friendly way I think. 

- Developing tests. I've also found this model to be neat for test 
development; add a test, build, load the module to test the test, add 
another test, build, unload/load etc. 

- The late_initcall() initialization of tests may not always be appropriate
for subsystems under test, and as the number of tests grow (a good 
problem to have!), it will likely become infeasible.

Anyway I'm not sure if any of the above resonates with others as being 
useful, but hopefully it clarifies why module support might matter moving 
forward.

If it makes sense, I can look at tweaking the module patchset to remove
the kunit_find_symbol() stuff so that we can punt on specific mechanisms 
for now; my main aim at this point is to ensure we're thinking about 
providing mechanisms for testing modules. 

Thanks!

Alan

Mike Salvatore Nov. 5, 2019, 4:43 p.m. UTC | #21

>> but such approach is not mainstream.
>> I personally like the idea of testing the lowest level bits in isolation even if
>> they are not a part of any interface. I think that specifying the
>> interface using
>> unit tests and ensuring implementation correctness are complementary but
>> I haven't had much luck arguing this with our esteemed colleagues.

In general, testing public interfaces is preferable, however, I think it's
important to avoid becoming dogmatic. IMHO, it's more important to have tests
that are clear in what they test than to not write tests (or write confusing
tests) in order to adhere to a generalized principle.

> So I think this is a very subtle point which is very widely
> misunderstood. Most people write code and then write their tests,
> following this practice along with only testing public interfaces
> often causes people to just not test all of their code, which is
> wrong.

The very nature of this situation is that the code was written before the tests.

> The idea of only testing public interfaces is supposed to make people
> think more carefully about what the composite layers of the program
> is. If you are having difficulty getting decent coverage by only
> testing your public interfaces, then it likely tells you that you have
> one of two problems:
> 
> 1) You have code that you don't need, and you should remove it.
> 
> 2) One of the layers in your program is too think, and you should
> introduce a new layer with a new public interface that you can test
> through.
> 
> I think the second point here is problematic with how C is written in
> the kernel. We don't really have any concept of public vs. private
> inside the kernel outside of static vs. not static, which is much more
> restricted.

I don't think we can expect developers to refactor large portions of complex
kernel code in order to improve its testability. I imagine this will happen
naturally over time, but I think we need to allow for developers to test
"private" code in the meanwhile.

My opinion is that it's more important to have tests than not. As evidence, I
submit the following commit:
https://github.com/torvalds/linux/commit/156e42996bd84eccb6acf319f19ce0cb140d00e3.

While not a major bug, this bug was discovered as a direct result of writing
these unit tests. So, in summary, I see value in "testing the lowest level bits
in isolation", even if it doesn't necessarily represent the Gold Standard in
Unit Testing.

Brendan Higgins Nov. 5, 2019, 11:44 p.m. UTC | #22

On Fri, Nov 1, 2019 at 5:30 AM Alan Maguire <[e-mail]> wrote:
>
> On Thu, 31 Oct 2019, Brendan Higgins wrote:
>
> > On Wed, Oct 30, 2019 at 12:09 PM Kees Cook <[e-mail]> wrote:
> > >
> > > On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
> > > > On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
> > > > > With that, I think the best solution in this case will be the
> > > > > "__visible_for_testing" route. It has no overhead when testing is
> > > > > turned off (in fact it is no different in anyway when testing is
> > > > > turned off). The downsides I see are:
> > > > >
> > > > > 1) You may not be able to test non-module code not compiled for
> > > > > testing later with the test modules that Alan is working on (But the
> > > > > only way I think that will work is by preventing the symbol from being
> > > > > inlined, right?).
> > > > >
> > > > > 2) I think "__visible_for_testing" will be prone to abuse. Here, I
> > > > > think there are reasons why we might want to expose these symbols for
> > > > > testing, but not otherwise. Nevertheless, I think most symbols that
> > > > > should be tested should probably be made visible by default. Since you
> > > > > usually only want to test your public interfaces. I could very well
> > > > > see this getting used as a kludge that gets used far too frequently.
> > > >
> > > > There are two parts to your statement on 2):
> > > >
> > > >   a) possible abuse of say __visible_for_testing
> > >
> > > I really don't like the idea of littering the kernel with these. It'll
> >
> > Yeah, I kind of hope that it would make people think more
> > intentionally about what is a public interface so that they wouldn't
> > litter the kernel with those. But I agree that in the world where
> > people *didn't* do that. Lots of these sprinkled around would be
> > annoying.
> >
> > > also require chunks in header files wrapped in #ifdefs. This is really
> >
> > Why would it require header files wrapped in #ifdefs?
> >
> > We could put all the ifdeffery logic in the __visible_for_testing
> > macro so that nothing in the original code has to change except for
> > adding an #include and replacing a couple of `static`s with
> > `__visible_for_testing`.
> >
>
> FWIW I think this approach, if used sparingly, is fine.  However I'd
> propose a hierarchy of options when looking to expose interfaces for
> testing.
>
> 1. For small, largely self-contained functions, move their definitions
> from .c files to a .h file where those functions are defined as "static
> inline".  That way the original code and tests can included them and we
> have solved function availability for both the cases where the tests are
> built-in and compiled as a module.  The apparmor interfaces here seem to
> be candidates for that approach.
>
> 2. For more complex cases, __visible_for_testing (for built-in visbility)
> and some sort of equivalent EXPORT_FOR_TESTING (for module
> visibility) would work, or the kunit_find_symbol() based lookup approach I
> suggested in the module patches.  Either of these allows for building
> tests as modules or builtin.
>
> 3. For some cases, module support will probably be impossible or difficult
> to maintain.  In such cases, builtin tests make most sense so any
> questions about symbol visibility would largely concern changing static
> definitions to be __visibile_for_testing, with no need for any symbol
> export for module visibility.

Very well said, I think this sums up a lot of good points. Basically,
I think you illustrate that it's not just one of the ways that were
proposed is the most appropriate for all cases, but really several are
valid strategies in different instances.

> > > ugly.
> > >
> > > >   b) you typically only want to test your public interfaces
> > >
> > > True, but being able to test the little helper functions is a nice
> > > starting point and a good building block.
> >
> > Yeah, I think I have come to accept that. We can argue about how this
> > should change and how people need to learn to be more intentional
> > about which interfaces are public and many other high minded ideas,
> > but when it comes down to it, we need to provide a starting point that
> > is easy.
> >
> > If our nice starting point becomes a problem, we can always improve it later.
> >
> > > Why can't unit tests live with the code they're testing? They're already
> > > logically tied together; what's the harm there? This needn't be the case
> > > for ALL tests, etc. The test driver could still live externally. The
> > > test in the other .c would just have exported functions... ?
> >
> > Well, for one, it totally tanks certain cases for building KUnit tests
> > as modules. I don't care about this point *too* much personally, but I
> > accept that there are others that want this, and I don't want to make
> > these people's lives too difficult.
> >
>
> Appreciated.  I think at this point it might be useful to lay out my
> thinking on why being able to build tests as modules may be helpful moving
> forward.
>
> - First and foremost, if the functionality itself is predominantly
> delivered in module form, or indeed is simply tristate, having a way to
> test kernel code when built as a module seems to me to be necessary. To
> test module code with built-in test code seems broken, and even if it
> could be made to work we'd end up having to invent a bunch of the mechanisms
> we'd need for building tests as modules anyway.

I think that is a fair point. I think I was thinking of it as an all
or nothing type thing. I know that we had moved past it in words, but
I think I was still hung up on the idea that we were going to try to
aggressively make tests buildable as modules. Here, and combined with
what Mike said (in a later email), I think I realized that a better
metric is what the code under test does.

It's probably not a big deal to make *everything* available as a
module. The right thing is probably that, if the code is available as
a module, the test should probably be available as a module. If the
code is not available as a module, it is not necessary to provide the
test as a module.

But that and what Mike said, I think, gets at something deeper. Each
subsystem has its own way of doing things, and that is a reality we
have to deal with. As long as there is some way to "run all the tests"
what conventions we enforce at the outset may not really be all that
important. Sure, some amount of consistency is important, but what is
more important is that we make something that is easy to use. We can
always go back and clean things up later.

After writing this, it sounds kind of obvious and like things that
people have said already; nevertheless, I think it is still worthwhile
to say, if nothing else to show that I think we are all on the same
page.

So yeah, I think that optionally including tests in the code under
test is fine (if that is what works best for the developer), I think
that __visible_for_testing is fine if that's what works best, and
testing through public interfaces is also fine. We might want to
gently push people in one direction or another over time, but all seem
like things that are reasonable to support now. In this case, since
people seem to be more in favor of including tests in source, that's
probably the right thing to do here.

I will make the __visible_for_testing thing available in a separate
patch at some point. Someone can pick it up if they want to use it.

> - Running tests on demand.  From previous discussions, I think this is
> wanted for kselftest, and if we have a set of modules with a conventional
> prefix (e.g. kunit-*), running tests becomes simply a "find + modprobe" in
> the kernel module tree.  Results could be harvested from debugfs (I have a
> WIP patch to store logging data in the per-test data structures such that
> "cat /sys/kernel/debug/kunit-results/kunit-foo" will display results for
> that test suite).  There are other ways to achieve this goal, and it's
> a crude method without any test selection beyond which modules are
> loaded, but this path is noticeably shorter to having a simple way to
> execute tests in a kselftest-friendly way I think.

Yep, I think we are all in agreement here. Shuah, Knut, and myself all
agreed at LPC that running tests on demand via kselftest was a
worthwhile goal. I am not strongly opposed to the common prefix idea.
I think that is something we might want to run past Linus though, as
he has not been a fan of certain file prefixes that he considers to
convey redundant information.

> - Developing tests. I've also found this model to be neat for test
> development; add a test, build, load the module to test the test, add
> another test, build, unload/load etc.

Not really sure what you mean here. I suspect that I probably won't
agree, as I have found that rebuilding the kernel for every test is
not overly burdensome. Nevertheless, I also don't see any reason to
oppose you here. If some developer likes that model (even if I don't),
it is best to support it if possible, as we should ideally make things
easier for every development flow.

> - The late_initcall() initialization of tests may not always be appropriate
> for subsystems under test, and as the number of tests grow (a good
> problem to have!), it will likely become infeasible.

Agreed. I just went with it for now because it was easy and
uncontroversial. I already have some WIP patches to get rid of it (I
am not sure how that will affect what you are doing, but I suspect
your module patches will be done first - since they already look close
- so I will probably try to figure that out after your module patches
get merged).

> Anyway I'm not sure if any of the above resonates with others as being
> useful, but hopefully it clarifies why module support might matter moving
> forward.

I definitely agree with the point about supporting building tests as
modules if the code under test builds as modules, especially if the
developers want it. So yes, it does resonate with me! :-)

> If it makes sense, I can look at tweaking the module patchset to remove
> the kunit_find_symbol() stuff so that we can punt on specific mechanisms
> for now; my main aim at this point is to ensure we're thinking about
> providing mechanisms for testing modules.

Yeah, I started looking at the latest version of your patches (sorry
for the late follow up), and yeah, I think it probably makes sense to
split that out into a separate patch/patchset.

Thanks for the comments! They really helped clear some things up for me!

Brendan Higgins Nov. 5, 2019, 11:59 p.m. UTC | #23

On Tue, Nov 5, 2019 at 8:43 AM Mike Salvatore
<[e-mail]> wrote:
>
> >> but such approach is not mainstream.
> >> I personally like the idea of testing the lowest level bits in isolation even if
> >> they are not a part of any interface. I think that specifying the
> >> interface using
> >> unit tests and ensuring implementation correctness are complementary but
> >> I haven't had much luck arguing this with our esteemed colleagues.
>
> In general, testing public interfaces is preferable, however, I think it's
> important to avoid becoming dogmatic. IMHO, it's more important to have tests
> that are clear in what they test than to not write tests (or write confusing
> tests) in order to adhere to a generalized principle.

That's a really good point.

> > So I think this is a very subtle point which is very widely
> > misunderstood. Most people write code and then write their tests,
> > following this practice along with only testing public interfaces
> > often causes people to just not test all of their code, which is
> > wrong.
>
> The very nature of this situation is that the code was written before the tests.
>
> > The idea of only testing public interfaces is supposed to make people
> > think more carefully about what the composite layers of the program
> > is. If you are having difficulty getting decent coverage by only
> > testing your public interfaces, then it likely tells you that you have
> > one of two problems:
> >
> > 1) You have code that you don't need, and you should remove it.
> >
> > 2) One of the layers in your program is too think, and you should
> > introduce a new layer with a new public interface that you can test
> > through.
> >
> > I think the second point here is problematic with how C is written in
> > the kernel. We don't really have any concept of public vs. private
> > inside the kernel outside of static vs. not static, which is much more
> > restricted.
>
> I don't think we can expect developers to refactor large portions of complex
> kernel code in order to improve its testability. I imagine this will happen
> naturally over time, but I think we need to allow for developers to test
> "private" code in the meanwhile.
>
> My opinion is that it's more important to have tests than not. As evidence, I
> submit the following commit:
> https://github.com/torvalds/linux/commit/156e42996bd84eccb6acf319f19ce0cb140d00e3.
>
> While not a major bug, this bug was discovered as a direct result of writing
> these unit tests. So, in summary, I see value in "testing the lowest level bits
> in isolation", even if it doesn't necessarily represent the Gold Standard in
> Unit Testing.

You're right.

I think, in summary, it seems that pretty much everyone agrees that we
need to provide a mechanism for testing low level bits of code in
isolation in such a way that the tests are easy to write, and don't
require the code under test to be massively refactored. Beyond that it
seems that we are mostly in between either including tests in the
source for the code under test or using the __visible_for_testing
mechanism. Between the two, it seems the preference is for including
the test in the source.

So I think I will still send out a patch to add in the
__visible_for_testing mechanism in case someone wants to use it in the
future.

Nevertheless, I will reformat this patch to include the tests in the
files that are under test. One question, do we have a preference
between putting all the tests in the same file as the code under test?
Or do we want to put them in a separate file that gets #included in
the file under test? I have a preference for the latter, but will
defer to what everyone else wants.

Thanks everyone!

Brendan Higgins Nov. 6, 2019, 12:35 a.m. UTC | #24

On Wed, Oct 30, 2019 at 11:59 AM Kees Cook <[e-mail]> wrote:
>
> On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
> > On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins
> > <[e-mail]> wrote:
> >
> > > +config SECURITY_APPARMOR_TEST
> > > +       bool "Build KUnit tests for policy_unpack.c"
> > > +       default n
>
> New options already already default n, this can be left off.
>
> > > +       depends on KUNIT && SECURITY_APPARMOR
> > > +       help
> > >
> > select SECURITY_APPARMOR ?
>
> "select" doesn't enforce dependencies, so just a "depends ..." is
> correct.
>
> > > +       KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> > > +       KUNIT_EXPECT_TRUE(test,
> > > +               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > I think this must be  KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);,
> > otherwise there could be a buffer overflow in memcmp. All tests that
> > follow such pattern
>
> Agreed.
>
> > are suspect. Also, not sure about your stylistic preference for
> > KUNIT_EXPECT_TRUE(test,
> >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > vs
> > KUNIT_EXPECT_EQ(test,
> >                0,
> >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
>
> I like == 0.

Oh, I almost missed this. I think the *_EQ(...) is better than the
*_TRUE(...) because the EQ is able to provide more debug information
if the test fails (otherwise there would really be no point in
providing all these variants).

Any objections?

Thanks for the catch Iurii!

Brendan Higgins Nov. 6, 2019, 12:37 a.m. UTC | #25

On Tue, Nov 5, 2019 at 4:35 PM Brendan Higgins
<[e-mail]> wrote:
>
> On Wed, Oct 30, 2019 at 11:59 AM Kees Cook <[e-mail]> wrote:
> >
> > On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
> > > On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins
> > > <[e-mail]> wrote:
> > >
> > > > +config SECURITY_APPARMOR_TEST
> > > > +       bool "Build KUnit tests for policy_unpack.c"
> > > > +       default n
> >
> > New options already already default n, this can be left off.
> >
> > > > +       depends on KUNIT && SECURITY_APPARMOR
> > > > +       help
> > > >
> > > select SECURITY_APPARMOR ?
> >
> > "select" doesn't enforce dependencies, so just a "depends ..." is
> > correct.
> >
> > > > +       KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
> > > > +       KUNIT_EXPECT_TRUE(test,
> > > > +               memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > > I think this must be  KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);,
> > > otherwise there could be a buffer overflow in memcmp. All tests that
> > > follow such pattern
> >
> > Agreed.
> >
> > > are suspect. Also, not sure about your stylistic preference for
> > > KUNIT_EXPECT_TRUE(test,
> > >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
> > > vs
> > > KUNIT_EXPECT_EQ(test,
> > >                0,
> > >                memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
> >
> > I like == 0.
>
> Oh, I almost missed this. I think the *_EQ(...) is better than the
> *_TRUE(...) because the EQ is able to provide more debug information
> if the test fails (otherwise there would really be no point in
> providing all these variants).
>
> Any objections?
>
> Thanks for the catch Iurii!

Wait, nevermind.

Either way is fine because memcmp probably won't show terribly
interesting information in the non-zero case. I'll just leave it the
way Mike wrote it.

Sorry!

Patch

diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
index d8b1a360a636..632fbb873389 100644
--- a/security/apparmor/Kconfig
+++ b/security/apparmor/Kconfig
@@ -66,3 +66,20 @@  config SECURITY_APPARMOR_DEBUG_MESSAGES
 	  Set the default value of the apparmor.debug kernel parameter.
 	  When enabled, various debug messages will be logged to
 	  the kernel message buffer.
+
+config SECURITY_APPARMOR_TEST
+	bool "Build KUnit tests for policy_unpack.c"
+	default n
+	depends on KUNIT && SECURITY_APPARMOR
+	help
+	  This builds the AppArmor KUnit tests.
+
+	  KUnit tests run during boot and output the results to the debug log
+	  in TAP format (http://testanything.org/). Only useful for kernel devs
+	  running KUnit test harness and are not for inclusion into a
+	  production build.
+
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
diff --git a/security/apparmor/Makefile b/security/apparmor/Makefile
index ff23fcfefe19..7ac1fc2dc045 100644
--- a/security/apparmor/Makefile
+++ b/security/apparmor/Makefile
@@ -2,6 +2,7 @@ 
 # Makefile for AppArmor Linux Security Module
 #
 obj-$(CONFIG_SECURITY_APPARMOR) += apparmor.o
+obj-$(CONFIG_SECURITY_APPARMOR_TEST) += policy_unpack_test.o
 
 apparmor-y := apparmorfs.o audit.o capability.o task.o ipc.o lib.o match.o \
               path.o domain.o policy.o policy_unpack.o procattr.o lsm.o \
diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h
index 46aefae918f5..dc272bafee2b 100644
--- a/security/apparmor/include/policy_unpack.h
+++ b/security/apparmor/include/policy_unpack.h
@@ -16,6 +16,59 @@ 
 #include <linux/dcache.h>
 #include <linux/workqueue.h>
 
+/*
+ * The AppArmor interface treats data as a type byte followed by the
+ * actual data.  The interface has the notion of a a named entry
+ * which has a name (AA_NAME typecode followed by name string) followed by
+ * the entries typecode and data.  Named types allow for optional
+ * elements and extensions to be added and tested for without breaking
+ * backwards compatibility.
+ */
+
+enum aa_code {
+	AA_U8,
+	AA_U16,
+	AA_U32,
+	AA_U64,
+	AA_NAME,		/* same as string except it is items name */
+	AA_STRING,
+	AA_BLOB,
+	AA_STRUCT,
+	AA_STRUCTEND,
+	AA_LIST,
+	AA_LISTEND,
+	AA_ARRAY,
+	AA_ARRAYEND,
+};
+
+/*
+ * aa_ext is the read of the buffer containing the serialized profile.  The
+ * data is copied into a kernel buffer in apparmorfs and then handed off to
+ * the unpack routines.
+ */
+struct aa_ext {
+	void *start;
+	void *end;
+	void *pos;		/* pointer to current position in the buffer */
+	u32 version;
+};
+
+/* test if read will be in packed data bounds */
+static inline bool inbounds(struct aa_ext *e, size_t size)
+{
+	return (size <= e->end - e->pos);
+}
+
+size_t unpack_u16_chunk(struct aa_ext *e, char **chunk);
+bool unpack_X(struct aa_ext *e, enum aa_code code);
+bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name);
+bool unpack_u32(struct aa_ext *e, u32 *data, const char *name);
+bool unpack_u64(struct aa_ext *e, u64 *data, const char *name);
+size_t unpack_array(struct aa_ext *e, const char *name);
+size_t unpack_blob(struct aa_ext *e, char **blob, const char *name);
+int unpack_str(struct aa_ext *e, const char **string, const char *name);
+int unpack_strdup(struct aa_ext *e, char **string, const char *name);
+
 struct aa_load_ent {
 	struct list_head list;
 	struct aa_profile *new;
diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index 8cfc9493eefc..7811e9b7a154 100644
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -36,43 +36,6 @@ 
 #define v7	7
 #define v8	8	/* full network masking */
 
-/*
- * The AppArmor interface treats data as a type byte followed by the
- * actual data.  The interface has the notion of a a named entry
- * which has a name (AA_NAME typecode followed by name string) followed by
- * the entries typecode and data.  Named types allow for optional
- * elements and extensions to be added and tested for without breaking
- * backwards compatibility.
- */
-
-enum aa_code {
-	AA_U8,
-	AA_U16,
-	AA_U32,
-	AA_U64,
-	AA_NAME,		/* same as string except it is items name */
-	AA_STRING,
-	AA_BLOB,
-	AA_STRUCT,
-	AA_STRUCTEND,
-	AA_LIST,
-	AA_LISTEND,
-	AA_ARRAY,
-	AA_ARRAYEND,
-};
-
-/*
- * aa_ext is the read of the buffer containing the serialized profile.  The
- * data is copied into a kernel buffer in apparmorfs and then handed off to
- * the unpack routines.
- */
-struct aa_ext {
-	void *start;
-	void *end;
-	void *pos;		/* pointer to current position in the buffer */
-	u32 version;
-};
-
 /* audit callback for unpack fields */
 static void audit_cb(struct audit_buffer *ab, void *va)
 {
@@ -194,12 +157,6 @@  struct aa_loaddata *aa_loaddata_alloc(size_t size)
 	return d;
 }
 
-/* test if read will be in packed data bounds */
-static bool inbounds(struct aa_ext *e, size_t size)
-{
-	return (size <= e->end - e->pos);
-}
-
 static void *kvmemdup(const void *src, size_t len)
 {
 	void *p = kvmalloc(len, GFP_KERNEL);
@@ -216,7 +173,7 @@  static void *kvmemdup(const void *src, size_t len)
  *
  * Returns: the size of chunk found with the read head at the end of the chunk.
  */
-static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
+size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
 {
 	size_t size = 0;
 	void *pos = e->pos;
@@ -237,7 +194,7 @@  static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
 }
 
 /* unpack control byte */
-static bool unpack_X(struct aa_ext *e, enum aa_code code)
+bool unpack_X(struct aa_ext *e, enum aa_code code)
 {
 	if (!inbounds(e, 1))
 		return 0;
@@ -263,7 +220,7 @@  static bool unpack_X(struct aa_ext *e, enum aa_code code)
  *
  * Returns: 0 if either match fails, the read head does not move
  */
-static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
+bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
 {
 	/*
 	 * May need to reset pos if name or type doesn't match
@@ -311,7 +268,7 @@  static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name)
 	return 0;
 }
 
-static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
+bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
 {
 	void *pos = e->pos;
 
@@ -329,7 +286,7 @@  static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
 	return 0;
 }
 
-static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
+bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
 {
 	void *pos = e->pos;
 
@@ -347,7 +304,7 @@  static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
 	return 0;
 }
 
-static size_t unpack_array(struct aa_ext *e, const char *name)
+size_t unpack_array(struct aa_ext *e, const char *name)
 {
 	void *pos = e->pos;
 
@@ -365,7 +322,7 @@  static size_t unpack_array(struct aa_ext *e, const char *name)
 	return 0;
 }
 
-static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
+size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
 {
 	void *pos = e->pos;
 
@@ -387,7 +344,7 @@  static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
 	return 0;
 }
 
-static int unpack_str(struct aa_ext *e, const char **string, const char *name)
+int unpack_str(struct aa_ext *e, const char **string, const char *name)
 {
 	char *src_str;
 	size_t size = 0;
@@ -410,7 +367,7 @@  static int unpack_str(struct aa_ext *e, const char **string, const char *name)
 	return 0;
 }
 
-static int unpack_strdup(struct aa_ext *e, char **string, const char *name)
+int unpack_strdup(struct aa_ext *e, char **string, const char *name)
 {
 	const char *tmp;
 	void *pos = e->pos;
diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
new file mode 100644
index 000000000000..3907f7d642e6
--- /dev/null
+++ b/security/apparmor/policy_unpack_test.c
@@ -0,0 +1,607 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * KUnit tests for AppArmor's policy unpack.
+ */
+
+#include <kunit/test.h>
+
+#include "include/policy.h"
+#include "include/policy_unpack.h"
+
+#define TEST_STRING_NAME "TEST_STRING"
+#define TEST_STRING_DATA "testing"
+#define TEST_STRING_BUF_OFFSET \
+	(3 + strlen(TEST_STRING_NAME) + 1)
+
+#define TEST_U32_NAME "U32_TEST"
+#define TEST_U32_DATA ((u32)0x01020304)
+#define TEST_NAMED_U32_BUF_OFFSET \
+	(TEST_STRING_BUF_OFFSET + 3 + strlen(TEST_STRING_DATA) + 1)
+#define TEST_U32_BUF_OFFSET \
+	(TEST_NAMED_U32_BUF_OFFSET + 3 + strlen(TEST_U32_NAME) + 1)
+
+#define TEST_U16_OFFSET (TEST_U32_BUF_OFFSET + 3)
+#define TEST_U16_DATA ((u16)(TEST_U32_DATA >> 16))
+
+#define TEST_U64_NAME "U64_TEST"
+#define TEST_U64_DATA ((u64)0x0102030405060708)
+#define TEST_NAMED_U64_BUF_OFFSET (TEST_U32_BUF_OFFSET + sizeof(u32) + 1)
+#define TEST_U64_BUF_OFFSET \
+	(TEST_NAMED_U64_BUF_OFFSET + 3 + strlen(TEST_U64_NAME) + 1)
+
+#define TEST_BLOB_NAME "BLOB_TEST"
+#define TEST_BLOB_DATA "\xde\xad\x00\xbe\xef"
+#define TEST_BLOB_DATA_SIZE (ARRAY_SIZE(TEST_BLOB_DATA))
+#define TEST_NAMED_BLOB_BUF_OFFSET (TEST_U64_BUF_OFFSET + sizeof(u64) + 1)
+#define TEST_BLOB_BUF_OFFSET \
+	(TEST_NAMED_BLOB_BUF_OFFSET + 3 + strlen(TEST_BLOB_NAME) + 1)
+
+#define TEST_ARRAY_NAME "ARRAY_TEST"
+#define TEST_ARRAY_SIZE 16
+#define TEST_NAMED_ARRAY_BUF_OFFSET \
+	(TEST_BLOB_BUF_OFFSET + 5 + TEST_BLOB_DATA_SIZE)
+#define TEST_ARRAY_BUF_OFFSET \
+	(TEST_NAMED_ARRAY_BUF_OFFSET + 3 + strlen(TEST_ARRAY_NAME) + 1)
+
+struct policy_unpack_fixture {
+	struct aa_ext *e;
+	size_t e_size;
+};
+
+struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf,
+				   struct kunit *test, size_t buf_size)
+{
+	char *buf;
+	struct aa_ext *e;
+
+	buf = kunit_kzalloc(test, buf_size, GFP_USER);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, buf);
+
+	e = kunit_kmalloc(test, sizeof(*e), GFP_USER);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, e);
+
+	e->start = buf;
+	e->end = e->start + buf_size;
+	e->pos = e->start;
+
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_STRING_NAME) + 1;
+	strcpy(buf + 3, TEST_STRING_NAME);
+
+	buf = e->start + TEST_STRING_BUF_OFFSET;
+	*buf = AA_STRING;
+	*(buf + 1) = strlen(TEST_STRING_DATA) + 1;
+	strcpy(buf + 3, TEST_STRING_DATA);
+
+	buf = e->start + TEST_NAMED_U32_BUF_OFFSET;
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_U32_NAME) + 1;
+	strcpy(buf + 3, TEST_U32_NAME);
+	*(buf + 3 + strlen(TEST_U32_NAME) + 1) = AA_U32;
+	*((u32 *)(buf + 3 + strlen(TEST_U32_NAME) + 2)) = TEST_U32_DATA;
+
+	buf = e->start + TEST_NAMED_U64_BUF_OFFSET;
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_U64_NAME) + 1;
+	strcpy(buf + 3, TEST_U64_NAME);
+	*(buf + 3 + strlen(TEST_U64_NAME) + 1) = AA_U64;
+	*((u64 *)(buf + 3 + strlen(TEST_U64_NAME) + 2)) = TEST_U64_DATA;
+
+	buf = e->start + TEST_NAMED_BLOB_BUF_OFFSET;
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_BLOB_NAME) + 1;
+	strcpy(buf + 3, TEST_BLOB_NAME);
+	*(buf + 3 + strlen(TEST_BLOB_NAME) + 1) = AA_BLOB;
+	*(buf + 3 + strlen(TEST_BLOB_NAME) + 2) = TEST_BLOB_DATA_SIZE;
+	memcpy(buf + 3 + strlen(TEST_BLOB_NAME) + 6,
+		TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE);
+
+	buf = e->start + TEST_NAMED_ARRAY_BUF_OFFSET;
+	*buf = AA_NAME;
+	*(buf + 1) = strlen(TEST_ARRAY_NAME) + 1;
+	strcpy(buf + 3, TEST_ARRAY_NAME);
+	*(buf + 3 + strlen(TEST_ARRAY_NAME) + 1) = AA_ARRAY;
+	*((u16 *)(buf + 3 + strlen(TEST_ARRAY_NAME) + 2)) = TEST_ARRAY_SIZE;
+
+	return e;
+}
+
+static int policy_unpack_test_init(struct kunit *test)
+{
+	size_t e_size = TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1;
+	struct policy_unpack_fixture *puf;
+
+	puf = kunit_kmalloc(test, sizeof(*puf), GFP_USER);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, puf);
+
+	puf->e_size = e_size;
+	puf->e = build_aa_ext_struct(puf, test, e_size);
+
+	test->priv = puf;
+	return 0;
+}
+
+static void policy_unpack_test_inbounds_when_inbounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+
+	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, 0));
+	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size / 2));
+	KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size));
+}
+
+static void policy_unpack_test_inbounds_when_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+
+	KUNIT_EXPECT_FALSE(test, inbounds(puf->e, puf->e_size + 1));
+}
+
+static void policy_unpack_test_unpack_array_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	u16 array_size;
+
+	puf->e->pos += TEST_ARRAY_BUF_OFFSET;
+
+	array_size = unpack_array(puf->e, NULL);
+
+	KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+		puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
+}
+
+static void policy_unpack_test_unpack_array_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_ARRAY_NAME;
+	u16 array_size;
+
+	puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
+
+	array_size = unpack_array(puf->e, name);
+
+	KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+		puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);
+}
+
+static void policy_unpack_test_unpack_array_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_ARRAY_NAME;
+	u16 array_size;
+
+	puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
+	puf->e->end = puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16);
+
+	array_size = unpack_array(puf->e, name);
+
+	KUNIT_EXPECT_EQ(test, array_size, (u16)0);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+		puf->e->start + TEST_NAMED_ARRAY_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_blob_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *blob = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_BLOB_BUF_OFFSET;
+	size = unpack_blob(puf->e, &blob, NULL);
+
+	KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
+	KUNIT_EXPECT_TRUE(test,
+		memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
+}
+
+static void policy_unpack_test_unpack_blob_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *blob = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
+	size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
+	KUNIT_EXPECT_TRUE(test,
+		memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
+}
+
+static void policy_unpack_test_unpack_blob_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *blob = NULL;
+	void *start;
+	int size;
+
+	puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
+	start = puf->e->pos;
+	puf->e->end = puf->e->start + TEST_BLOB_BUF_OFFSET
+		+ TEST_BLOB_DATA_SIZE - 1;
+
+	size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, 0);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+
+static void policy_unpack_test_unpack_str_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char *string = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_STRING_BUF_OFFSET;
+	size = unpack_str(puf->e, &string, NULL);
+
+	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
+	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+
+static void policy_unpack_test_unpack_str_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char *string = NULL;
+	size_t size;
+
+	size = unpack_str(puf->e, &string, TEST_STRING_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
+	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+
+static void policy_unpack_test_unpack_str_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char *string = NULL;
+	void *start = puf->e->pos;
+	int size;
+
+	puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
+		+ strlen(TEST_STRING_DATA) - 1;
+
+	size = unpack_str(puf->e, &string, TEST_STRING_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, 0);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+
+static void policy_unpack_test_unpack_strdup_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *string = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_STRING_BUF_OFFSET;
+	size = unpack_strdup(puf->e, &string, NULL);
+
+	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
+	KUNIT_EXPECT_FALSE(test,
+			   ((uintptr_t)puf->e->start <= (uintptr_t)string)
+			   && ((uintptr_t)string <= (uintptr_t)puf->e->end));
+	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+
+static void policy_unpack_test_unpack_strdup_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *string = NULL;
+	size_t size;
+
+	size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
+	KUNIT_EXPECT_FALSE(test,
+			   ((uintptr_t)puf->e->start <= (uintptr_t)string)
+			   && ((uintptr_t)string <= (uintptr_t)puf->e->end));
+	KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+
+static void policy_unpack_test_unpack_strdup_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	void *start = puf->e->pos;
+	char *string = NULL;
+	int size;
+
+	puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
+		+ strlen(TEST_STRING_DATA) - 1;
+
+	size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
+
+	KUNIT_EXPECT_EQ(test, size, 0);
+	KUNIT_EXPECT_PTR_EQ(test, string, (char *)NULL);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+
+static void policy_unpack_test_unpack_nameX_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+
+	puf->e->pos += TEST_U32_BUF_OFFSET;
+
+	success = unpack_nameX(puf->e, AA_U32, NULL);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			    puf->e->start + TEST_U32_BUF_OFFSET + 1);
+}
+
+static void policy_unpack_test_unpack_nameX_with_wrong_code(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+
+	puf->e->pos += TEST_U32_BUF_OFFSET;
+
+	success = unpack_nameX(puf->e, AA_BLOB, NULL);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			    puf->e->start + TEST_U32_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_nameX_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U32_NAME;
+	bool success;
+
+	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
+
+	success = unpack_nameX(puf->e, AA_U32, name);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			    puf->e->start + TEST_U32_BUF_OFFSET + 1);
+}
+
+static void policy_unpack_test_unpack_nameX_with_wrong_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	static const char name[] = "12345678";
+	bool success;
+
+	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
+
+	success = unpack_nameX(puf->e, AA_U32, name);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			    puf->e->start + TEST_NAMED_U32_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_u16_chunk_basic(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *chunk = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_U16_OFFSET;
+	/*
+	 * WARNING: For unit testing purposes, we're pushing puf->e->end past
+	 * the end of the allocated memory. Doing anything other than comparing
+	 * memory addresses is dangerous.
+	 */
+	puf->e->end += TEST_U16_DATA;
+
+	size = unpack_u16_chunk(puf->e, &chunk);
+
+	KUNIT_EXPECT_PTR_EQ(test, (void *)chunk,
+			    puf->e->start + TEST_U16_OFFSET + 2);
+	KUNIT_EXPECT_EQ(test, size, (size_t)TEST_U16_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, (void *)(chunk + TEST_U16_DATA));
+}
+
+static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_1(
+		struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *chunk = NULL;
+	size_t size;
+
+	puf->e->pos = puf->e->end - 1;
+
+	size = unpack_u16_chunk(puf->e, &chunk);
+
+	KUNIT_EXPECT_EQ(test, size, (size_t)0);
+	KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->end - 1);
+}
+
+static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_2(
+		struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	char *chunk = NULL;
+	size_t size;
+
+	puf->e->pos += TEST_U16_OFFSET;
+	/*
+	 * WARNING: For unit testing purposes, we're pushing puf->e->end past
+	 * the end of the allocated memory. Doing anything other than comparing
+	 * memory addresses is dangerous.
+	 */
+	puf->e->end = puf->e->pos + TEST_U16_DATA - 1;
+
+	size = unpack_u16_chunk(puf->e, &chunk);
+
+	KUNIT_EXPECT_EQ(test, size, (size_t)0);
+	KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->start + TEST_U16_OFFSET);
+}
+
+static void policy_unpack_test_unpack_u32_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+	u32 data;
+
+	puf->e->pos += TEST_U32_BUF_OFFSET;
+
+	success = unpack_u32(puf->e, &data, NULL);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);
+}
+
+static void policy_unpack_test_unpack_u32_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U32_NAME;
+	bool success;
+	u32 data;
+
+	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
+
+	success = unpack_u32(puf->e, &data, name);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);
+}
+
+static void policy_unpack_test_unpack_u32_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U32_NAME;
+	bool success;
+	u32 data;
+
+	puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
+	puf->e->end = puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32);
+
+	success = unpack_u32(puf->e, &data, name);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_NAMED_U32_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_u64_with_null_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+	u64 data;
+
+	puf->e->pos += TEST_U64_BUF_OFFSET;
+
+	success = unpack_u64(puf->e, &data, NULL);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);
+}
+
+static void policy_unpack_test_unpack_u64_with_name(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U64_NAME;
+	bool success;
+	u64 data;
+
+	puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
+
+	success = unpack_u64(puf->e, &data, name);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);
+}
+
+static void policy_unpack_test_unpack_u64_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	const char name[] = TEST_U64_NAME;
+	bool success;
+	u64 data;
+
+	puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
+	puf->e->end = puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64);
+
+	success = unpack_u64(puf->e, &data, name);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
+			puf->e->start + TEST_NAMED_U64_BUF_OFFSET);
+}
+
+static void policy_unpack_test_unpack_X_code_match(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success = unpack_X(puf->e, AA_NAME);
+
+	KUNIT_EXPECT_TRUE(test, success);
+	KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start + 1);
+}
+
+static void policy_unpack_test_unpack_X_code_mismatch(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success = unpack_X(puf->e, AA_STRING);
+
+	KUNIT_EXPECT_FALSE(test, success);
+	KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start);
+}
+
+static void policy_unpack_test_unpack_X_out_of_bounds(struct kunit *test)
+{
+	struct policy_unpack_fixture *puf = test->priv;
+	bool success;
+
+	puf->e->pos = puf->e->end;
+	success = unpack_X(puf->e, AA_NAME);
+
+	KUNIT_EXPECT_FALSE(test, success);
+}
+
+static struct kunit_case apparmor_policy_unpack_test_cases[] = {
+	KUNIT_CASE(policy_unpack_test_inbounds_when_inbounds),
+	KUNIT_CASE(policy_unpack_test_inbounds_when_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_array_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_array_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_array_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_blob_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_blob_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_blob_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_code),
+	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_name),
+	KUNIT_CASE(policy_unpack_test_unpack_str_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_str_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_str_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_strdup_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_strdup_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_strdup_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_basic),
+	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_1),
+	KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_2),
+	KUNIT_CASE(policy_unpack_test_unpack_u32_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_u32_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_u32_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_u64_with_null_name),
+	KUNIT_CASE(policy_unpack_test_unpack_u64_with_name),
+	KUNIT_CASE(policy_unpack_test_unpack_u64_out_of_bounds),
+	KUNIT_CASE(policy_unpack_test_unpack_X_code_match),
+	KUNIT_CASE(policy_unpack_test_unpack_X_code_mismatch),
+	KUNIT_CASE(policy_unpack_test_unpack_X_out_of_bounds),
+	{},
+};
+
+static struct kunit_suite apparmor_policy_unpack_test_module = {
+	.name = "apparmor_policy_unpack",
+	.init = policy_unpack_test_init,
+	.test_cases = apparmor_policy_unpack_test_cases,
+};
+
+kunit_test_suite(apparmor_policy_unpack_test_module);