| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200623130216.GW2428291@smile.fi.intel.com>
Date: Tue, 23 Jun 2020 16:02:16 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Alan Maguire <alan.maguire@...cle.com>
Cc: ast@...nel.org, daniel@...earbox.net, yhs@...com, andriin@...com,
arnaldo.melo@...il.com, kafai@...com, songliubraving@...com,
john.fastabend@...il.com, kpsingh@...omium.org,
linux@...musvillemoes.dk, joe@...ches.com, pmladek@...e.com,
rostedt@...dmis.org, sergey.senozhatsky@...il.com, corbet@....net,
bpf@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 bpf-next 6/8] printk: extend test_printf to test %pT
BTF-based format specifier
On Tue, Jun 23, 2020 at 01:07:09PM +0100, Alan Maguire wrote:
> Add tests to verify basic type display and to iterate through all
> enums, structs, unions and typedefs ensuring expected behaviour
> occurs. Since test_printf can be built as a module we need to
> export a BTF kind iterator function to allow us to iterate over
> all names of a particular BTF kind.
>
> These changes add up to approximately 20,000 new tests covering
> all enum, struct, union and typedefs in vmlinux BTF.
>
> Individual tests are also added for int, char, struct, enum
> and typedefs which verify output is as expected.
...
> #include <linux/mm.h>
>
> #include <linux/property.h>
+ blank line, you see, headers are grouped.
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
> +#include <linux/skbuff.h>
> +#define __TEST_BTF(fmt, type, ptr, expected) \
> + test(expected, "%pT"fmt, ptr)
> +
> +#define TEST_BTF_C(type, var, ...) \
> + do { \
> + type var = __VA_ARGS__; \
> + struct btf_ptr *ptr = BTF_PTR_TYPE(&var, type); \
> + pr_debug("type %s: %pTc", #type, ptr); \
Hmm... Can't we modify test() (or underneath macros / functions) to do this?
> + __TEST_BTF("c", type, ptr, "(" #type ")" #__VA_ARGS__); \
> + } while (0)
> +
> +#define TEST_BTF(fmt, type, var, expected, ...) \
> + do { \
> + type var = __VA_ARGS__; \
> + struct btf_ptr *ptr = BTF_PTR_TYPE(&var, type); \
> + pr_debug("type %s: %pT"fmt, #type, ptr); \
> + __TEST_BTF(fmt, type, ptr, expected); \
> + } while (0)
...
> +static void __init
> +btf_print_kind(u8 kind, const char *kind_name, u64 fillval)
> +{
> + const char *fmt1 = "%pT", *fmt2 = "%pTN", *fmt3 = "%pT0";
This is hard to read. Provide a simple data structure or an array.
> + const char *name, *fmt = fmt1;
> + int i, res1, res2, res3, res4;
> + char type_name[256];
> + char *buf, *buf2;
> + u8 *dummy_data;
> + s32 id = 0;
> +
> + dummy_data = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);
check?
> + /* fill our dummy data with supplied fillval. */
> + for (i = 0; i < BTF_MAX_DATA_SIZE; i++)
> + dummy_data[i] = fillval;
> + buf = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);
> + buf2 = kzalloc(BTF_MAX_DATA_SIZE, GFP_KERNEL);
Ditto.
> + for (;;) {
> + name = btf_vmlinux_next_type_name(kind, &id);
> + if (!name)
> + break;
> +
> + total_tests++;
> +
> + snprintf(type_name, sizeof(type_name), "%s%s",
> + kind_name, name);
> +
> + res1 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt1,
> + BTF_PTR_TYPE(dummy_data, type_name));
> + res2 = snprintf(buf, 0, fmt1,
> + BTF_PTR_TYPE(dummy_data, type_name));
> + res3 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt2,
> + BTF_PTR_TYPE(dummy_data, type_name));
> + res4 = snprintf(buf, BTF_MAX_DATA_SIZE, fmt3,
> + BTF_PTR_TYPE(dummy_data, type_name));
> +
> + (void) snprintf(buf, BTF_MAX_DATA_SIZE, "%pT",
> + BTF_PTR_TYPE(dummy_data, type_name));
> + (void) snprintf(buf2, BTF_MAX_DATA_SIZE, "%pT",
> + BTF_PTR_TYPE(dummy_data, type_name));
> +
> + /*
> + * Ensure return value is > 0 and identical irrespective
> + * of whether we pass in a big enough buffer;
> + * also ensure that printing names always results in as
> + * long/longer buffer length.
> + */
> + if (res1 <= 0 || res2 <= 0 || res3 <= 0 || res4 <= 0) {
> + if (res3 <= 0)
> + fmt = fmt2;
> + if (res4 <= 0)
> + fmt = fmt3;
> + pr_warn("snprintf(%s%s); %d <= 0 (fmt %s)",
> + kind_name, name,
> + res1 <= 0 ? res1 : res2 <= 0 ? res2 :
> + res3 <= 0 ? res3 : res4, fmt);
> + failed_tests++;
For these kind of prints you can use a new macro, right?
> + } else if (res1 != res2) {
> + pr_warn("snprintf(%s%s): %d (to buf) != %d (no buf)",
> + kind_name, name, res1, res2);
> + failed_tests++;
Ditto.
> + } else if (res3 > res2) {
> + pr_warn("snprintf(%s%s); %d (no names) > %d (names)",
> + kind_name, name, res3, res2);
> + failed_tests++;
Ditto.
> + } else if (strcmp(buf, buf2) != 0) {
> + /* Safe and unsafe buffers should match. */
> + pr_warn("snprintf(%s%s); safe != unsafe",
> + kind_name, name);
> + pr_warn("safe: %s", buf);
> + pr_warn("unsafe: %s", buf2);
> + failed_tests++;
Perhaps also makes sense in a macro then somebody may reuse in the future.
That said, the first warning here somehow cryptic, please be more human friendly.
> + } else {
> + pr_debug("Printed %s%s (%d bytes)",
> + kind_name, name, res1);
> + }
> + }
> + kfree(dummy_data);
> + kfree(buf);
> + kfree(buf2);
> +}
...
> + TEST_BTF_C(int, testint, 1234);
> + TEST_BTF("cN", int, testint, "1234", 1234);
We use small letter macros in other cases. So can you?
...
> + /* typedef struct */
> + TEST_BTF_C(atomic_t, testtype, {.counter = (int)1,});
> + TEST_BTF("cN", atomic_t, testtype, "{1,}", {.counter = 1,});
> + /* typedef with 0 value should be printed at toplevel */
> + TEST_BTF("c", atomic_t, testtype, "(atomic_t){}", {.counter = 0,});
> + TEST_BTF("cN", atomic_t, testtype, "{}", {.counter = 0,});
> + TEST_BTF("c0", atomic_t, testtype, "(atomic_t){.counter = (int)0,}",
> + {.counter = 0,});
> + TEST_BTF("cN0", atomic_t, testtype, "{0,}", {.counter = 0,});
For one type, provide a data structure filled with test data and use loops.
Same for all similar places over the code.
...
> + u64 fillvals[] = { 0x0, 0xffffffffffffffff, 0x0123456789abcdef };
U64_MAX?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists