| Message ID | 1658734261-4951-1-git-send-email-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: augment snprintf_btf tests with string overflow tests | expand |
On Mon, Jul 25, 2022 at 08:31:01AM +0100, Alan Maguire wrote: > add tests that verify bpf_snprintf_btf() behaviour with strings that > > - exactly fit the buffer (string size + null terminator == buffer_size) > - overrun the buffer (string size + null terminator == buffer size + 1) > - overrun the buffer (string size + null terminator == buffer size + 2) > > These tests require [1] ("bpf: btf: Fix vsnprintf return value check") > > ...which has not landed yet. patch looks good, but I have the test passing even without [1], it should fail, right? #151 snprintf_btf:OK jirka > > [1] https://lore.kernel.org/bpf/20220711211317.GA1143610@laptop/ > > Suggested-by: Jiri Olsa <jolsa@redhat.com> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > --- > .../selftests/bpf/progs/netif_receive_skb.c | 41 ++++++++++++++++++++-- > 1 file changed, 38 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c b/tools/testing/selftests/bpf/progs/netif_receive_skb.c > index 1d8918d..9fc48e4 100644 > --- a/tools/testing/selftests/bpf/progs/netif_receive_skb.c > +++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c > @@ -49,7 +49,7 @@ static int __strncmp(const void *m1, const void *m2, size_t len) > } > > #if __has_builtin(__builtin_btf_type_id) > -#define TEST_BTF(_str, _type, _flags, _expected, ...) \ > +#define TEST_BTF_SIZE(_str, _size, _type, _flags, _expected, ...) \ > do { \ > static const char _expectedval[EXPECTED_STRSIZE] = \ > _expected; \ > @@ -69,10 +69,13 @@ static int __strncmp(const void *m1, const void *m2, size_t len) > ret = -EINVAL; \ > break; \ > } \ > - ret = bpf_snprintf_btf(_str, STRSIZE, \ > + ret = bpf_snprintf_btf(_str, _size, \ > &_ptr, sizeof(_ptr), _hflags); \ > - if (ret) \ > + if (ret < 0) { \ > + bpf_printk("bpf_snprintf_btf_failed (%s): %d\n",\ > + _str, _expectedval, ret); \ > break; \ > + } \ > _cmp = __strncmp(_str, _expectedval, EXPECTED_STRSIZE); \ > if (_cmp != 0) { \ > bpf_printk("(%d) got %s", _cmp, _str); \ > @@ -82,6 +85,10 @@ static int __strncmp(const void *m1, const void *m2, size_t len) > break; \ > } \ > } while (0) > + > +#define TEST_BTF(_str, _type, _flags, _expected, ...) \ > + TEST_BTF_SIZE(_str, STRSIZE, _type, _flags, _expected, \ > + __VA_ARGS__) > #endif > > /* Use where expected data string matches its stringified declaration */ > @@ -98,7 +105,9 @@ int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb) > static __u64 flags[] = { 0, BTF_F_COMPACT, BTF_F_ZERO, BTF_F_PTR_RAW, > BTF_F_NONAME, BTF_F_COMPACT | BTF_F_ZERO | > BTF_F_PTR_RAW | BTF_F_NONAME }; > + static char _short_str[2] = {}; > static struct btf_ptr p = { }; > + char *short_str = _short_str; > __u32 key = 0; > int i, __ret; > char *str; > @@ -141,6 +150,32 @@ int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb) > TEST_BTF_C(str, int, 0, -4567); > TEST_BTF(str, int, BTF_F_NONAME, "-4567", -4567); > > + /* overflow tests; first string + terminator fits, others do not. */ > + TEST_BTF_SIZE(short_str, sizeof(_short_str), int, BTF_F_NONAME, "1", 1); > + if (ret != 1) { > + bpf_printk("bpf_snprintf_btf() should return 1 for '%s'/2-byte array", > + short_str); > + ret = -ERANGE; > + } > + /* not enough space to write "10", write "1", return 2 for number of bytes we > + * should have written. > + */ > + TEST_BTF_SIZE(short_str, sizeof(_short_str), int, BTF_F_NONAME, "1", 10); > + if (ret != 2) { > + bpf_printk("bpf_snprintf_btf() should return 2 for '%s'/2-byte array", > + short_str); > + ret = -ERANGE; > + } > + /* not enough space to write "100", write "1", return 3 for number of bytes we > + * should have written. > + */ > + TEST_BTF_SIZE(short_str, sizeof(_short_str), int, BTF_F_NONAME, "1", 100); > + if (ret != 3) { > + bpf_printk("bpf_snprintf_btf() should return 3 for '%s'/3-byte array", > + short_str); > + ret = -ERANGE; > + } > + > /* simple char */ > TEST_BTF_C(str, char, 0, 100); > TEST_BTF(str, char, BTF_F_NONAME, "100", 100); > -- > 1.8.3.1 >
On Tue, Jul 26, 2022 at 5:40 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Mon, Jul 25, 2022 at 08:31:01AM +0100, Alan Maguire wrote: > > add tests that verify bpf_snprintf_btf() behaviour with strings that > > > > - exactly fit the buffer (string size + null terminator == buffer_size) > > - overrun the buffer (string size + null terminator == buffer size + 1) > > - overrun the buffer (string size + null terminator == buffer size + 2) > > > > These tests require [1] ("bpf: btf: Fix vsnprintf return value check") > > > > ...which has not landed yet. > > patch looks good, but I have the test passing even without [1], > it should fail, right? > > #151 snprintf_btf:OK > The way that test is structured it's essentially impossible to communicate a test failure back, as each subsequent test case overrides common ret variable, if I understand it correctly. Alan, can you please update the test to store results for each individual test case separately, e.g., in an array? Meanwhile I've applied that trivial fix from Fedor anyway. > jirka > > > > > [1] https://lore.kernel.org/bpf/20220711211317.GA1143610@laptop/ > > > > Suggested-by: Jiri Olsa <jolsa@redhat.com> > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > --- > > .../selftests/bpf/progs/netif_receive_skb.c | 41 ++++++++++++++++++++-- > > 1 file changed, 38 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c b/tools/testing/selftests/bpf/progs/netif_receive_skb.c > > index 1d8918d..9fc48e4 100644 > > --- a/tools/testing/selftests/bpf/progs/netif_receive_skb.c > > +++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c > > @@ -49,7 +49,7 @@ static int __strncmp(const void *m1, const void *m2, size_t len) > > } > > [...]
diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c b/tools/testing/selftests/bpf/progs/netif_receive_skb.c index 1d8918d..9fc48e4 100644 --- a/tools/testing/selftests/bpf/progs/netif_receive_skb.c +++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c @@ -49,7 +49,7 @@ static int __strncmp(const void *m1, const void *m2, size_t len) } #if __has_builtin(__builtin_btf_type_id) -#define TEST_BTF(_str, _type, _flags, _expected, ...) \ +#define TEST_BTF_SIZE(_str, _size, _type, _flags, _expected, ...) \ do { \ static const char _expectedval[EXPECTED_STRSIZE] = \ _expected; \ @@ -69,10 +69,13 @@ static int __strncmp(const void *m1, const void *m2, size_t len) ret = -EINVAL; \ break; \ } \ - ret = bpf_snprintf_btf(_str, STRSIZE, \ + ret = bpf_snprintf_btf(_str, _size, \ &_ptr, sizeof(_ptr), _hflags); \ - if (ret) \ + if (ret < 0) { \ + bpf_printk("bpf_snprintf_btf_failed (%s): %d\n",\ + _str, _expectedval, ret); \ break; \ + } \ _cmp = __strncmp(_str, _expectedval, EXPECTED_STRSIZE); \ if (_cmp != 0) { \ bpf_printk("(%d) got %s", _cmp, _str); \ @@ -82,6 +85,10 @@ static int __strncmp(const void *m1, const void *m2, size_t len) break; \ } \ } while (0) + +#define TEST_BTF(_str, _type, _flags, _expected, ...) \ + TEST_BTF_SIZE(_str, STRSIZE, _type, _flags, _expected, \ + __VA_ARGS__) #endif /* Use where expected data string matches its stringified declaration */ @@ -98,7 +105,9 @@ int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb) static __u64 flags[] = { 0, BTF_F_COMPACT, BTF_F_ZERO, BTF_F_PTR_RAW, BTF_F_NONAME, BTF_F_COMPACT | BTF_F_ZERO | BTF_F_PTR_RAW | BTF_F_NONAME }; + static char _short_str[2] = {}; static struct btf_ptr p = { }; + char *short_str = _short_str; __u32 key = 0; int i, __ret; char *str; @@ -141,6 +150,32 @@ int BPF_PROG(trace_netif_receive_skb, struct sk_buff *skb) TEST_BTF_C(str, int, 0, -4567); TEST_BTF(str, int, BTF_F_NONAME, "-4567", -4567); + /* overflow tests; first string + terminator fits, others do not. */ + TEST_BTF_SIZE(short_str, sizeof(_short_str), int, BTF_F_NONAME, "1", 1); + if (ret != 1) { + bpf_printk("bpf_snprintf_btf() should return 1 for '%s'/2-byte array", + short_str); + ret = -ERANGE; + } + /* not enough space to write "10", write "1", return 2 for number of bytes we + * should have written. + */ + TEST_BTF_SIZE(short_str, sizeof(_short_str), int, BTF_F_NONAME, "1", 10); + if (ret != 2) { + bpf_printk("bpf_snprintf_btf() should return 2 for '%s'/2-byte array", + short_str); + ret = -ERANGE; + } + /* not enough space to write "100", write "1", return 3 for number of bytes we + * should have written. + */ + TEST_BTF_SIZE(short_str, sizeof(_short_str), int, BTF_F_NONAME, "1", 100); + if (ret != 3) { + bpf_printk("bpf_snprintf_btf() should return 3 for '%s'/3-byte array", + short_str); + ret = -ERANGE; + } + /* simple char */ TEST_BTF_C(str, char, 0, 100); TEST_BTF(str, char, BTF_F_NONAME, "100", 100);
add tests that verify bpf_snprintf_btf() behaviour with strings that - exactly fit the buffer (string size + null terminator == buffer_size) - overrun the buffer (string size + null terminator == buffer size + 1) - overrun the buffer (string size + null terminator == buffer size + 2) These tests require [1] ("bpf: btf: Fix vsnprintf return value check") ...which has not landed yet. [1] https://lore.kernel.org/bpf/20220711211317.GA1143610@laptop/ Suggested-by: Jiri Olsa <jolsa@redhat.com> Signed-off-by: Alan Maguire <alan.maguire@oracle.com> --- .../selftests/bpf/progs/netif_receive_skb.c | 41 ++++++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-)