diff mbox series

[bpf-next] selftest/bpf: testing for multiple logs on REJECT

Message ID 20210124190532.428065-1-andreimatei1@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] selftest/bpf: testing for multiple logs on REJECT | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers fail 11 maintainers not CCed: shuah@kernel.org netdev@vger.kernel.org songliubraving@fb.com linux-kselftest@vger.kernel.org andrii@kernel.org daniel@iogearbox.net ast@kernel.org kpsingh@kernel.org john.fastabend@gmail.com kafai@fb.com yhs@fb.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 85 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Andrei Matei Jan. 24, 2021, 7:05 p.m. UTC
This patch adds support to verifier tests to check for a succession of
verifier log messages on program load failure. This makes the
errstr field work uniformly across REJECT and VERBOSE_ACCEPT checks.

This patch also increases the maximum size of an accepted series of
messages to test from 80 chars to 200 chars. This is in order to keep
existing tests working, which sometimes test for messages larger than 80
chars (which was accepted in the REJECT case, when testing for a single
message, but ironically not in the VERBOSE_ACCEPT case, when testing for
possibly multiple messages).
And example of such a long, checked message is in bounds.c:
"R1 has unknown scalar with mixed signed bounds, pointer arithmetic with
it prohibited for !root"

Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
---
 tools/testing/selftests/bpf/test_verifier.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Daniel Borkmann Jan. 26, 2021, 11:21 p.m. UTC | #1
On 1/24/21 8:05 PM, Andrei Matei wrote:
> This patch adds support to verifier tests to check for a succession of
> verifier log messages on program load failure. This makes the
> errstr field work uniformly across REJECT and VERBOSE_ACCEPT checks.
> 
> This patch also increases the maximum size of an accepted series of
> messages to test from 80 chars to 200 chars. This is in order to keep
> existing tests working, which sometimes test for messages larger than 80
> chars (which was accepted in the REJECT case, when testing for a single
> message, but ironically not in the VERBOSE_ACCEPT case, when testing for
> possibly multiple messages).
> And example of such a long, checked message is in bounds.c:
> "R1 has unknown scalar with mixed signed bounds, pointer arithmetic with
> it prohibited for !root"
> 
> Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> ---
>   tools/testing/selftests/bpf/test_verifier.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 59bfa6201d1d..69298bf8ee86 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -88,6 +88,9 @@ struct bpf_test {
>   	int fixup_map_event_output[MAX_FIXUPS];
>   	int fixup_map_reuseport_array[MAX_FIXUPS];
>   	int fixup_map_ringbuf[MAX_FIXUPS];
> +	/* Expected verifier log output for result REJECT or VERBOSE_ACCEPT. Can be a
> +	 * tab-separated sequence of expected strings.
> +	 */
>   	const char *errstr;
>   	const char *errstr_unpriv;
>   	uint32_t insn_processed;
> @@ -995,9 +998,11 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
>   	return 0;
>   }
>   
> +/* Returns true if every part of exp (tab-separated) appears in log, in order.
> + */
>   static bool cmp_str_seq(const char *log, const char *exp)
>   {
> -	char needle[80];
> +	char needle[200];
>   	const char *p, *q;
>   	int len;
>   
> @@ -1015,7 +1020,7 @@ static bool cmp_str_seq(const char *log, const char *exp)
>   		needle[len] = 0;
>   		q = strstr(log, needle);
>   		if (!q) {
> -			printf("FAIL\nUnexpected verifier log in successful load!\n"
> +			printf("FAIL\nUnexpected verifier log!\n"
>   			       "EXP: %s\nRES:\n", needle);
>   			return false;
>   		}
> @@ -1130,7 +1135,11 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>   			printf("FAIL\nUnexpected success to load!\n");
>   			goto fail_log;
>   		}
> -		if (!expected_err || !strstr(bpf_vlog, expected_err)) {
> +		if (!expected_err) {
> +			printf("FAIL\nTestcase bug; missing expected_err\n");
> +			goto fail_log;

Do we have an in-tree case like this? Given this would also be visible below with 'EXP:'
being (null), I might simplify and just replace the strstr() with cmp_str_seq().

Also, could you elaborate on which test cases need the cmp_str_seq() conversion?

> +		}
> +		if ((strlen(expected_err) > 0) && !cmp_str_seq(bpf_vlog, expected_err)) {

(nit: no extra '()' needed, but either way I'd rather opt for '!expected_err ||
!cmp_str_seq(bpf_vlog, expected_err)' anyway)

>   			printf("FAIL\nUnexpected error message!\n\tEXP: %s\n\tRES: %s\n",
>   			      expected_err, bpf_vlog);
>   			goto fail_log;
>
Andrei Matei Jan. 27, 2021, 2:31 a.m. UTC | #2
On Tue, Jan 26, 2021 at 6:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 1/24/21 8:05 PM, Andrei Matei wrote:
> > This patch adds support to verifier tests to check for a succession of
> > verifier log messages on program load failure. This makes the
> > errstr field work uniformly across REJECT and VERBOSE_ACCEPT checks.
> >
> > This patch also increases the maximum size of an accepted series of
> > messages to test from 80 chars to 200 chars. This is in order to keep
> > existing tests working, which sometimes test for messages larger than 80
> > chars (which was accepted in the REJECT case, when testing for a single
> > message, but ironically not in the VERBOSE_ACCEPT case, when testing for
> > possibly multiple messages).
> > And example of such a long, checked message is in bounds.c:
> > "R1 has unknown scalar with mixed signed bounds, pointer arithmetic with
> > it prohibited for !root"
> >
> > Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> > ---
> >   tools/testing/selftests/bpf/test_verifier.c | 15 ++++++++++++---
> >   1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index 59bfa6201d1d..69298bf8ee86 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -88,6 +88,9 @@ struct bpf_test {
> >       int fixup_map_event_output[MAX_FIXUPS];
> >       int fixup_map_reuseport_array[MAX_FIXUPS];
> >       int fixup_map_ringbuf[MAX_FIXUPS];
> > +     /* Expected verifier log output for result REJECT or VERBOSE_ACCEPT. Can be a
> > +      * tab-separated sequence of expected strings.
> > +      */
> >       const char *errstr;
> >       const char *errstr_unpriv;
> >       uint32_t insn_processed;
> > @@ -995,9 +998,11 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> >       return 0;
> >   }
> >
> > +/* Returns true if every part of exp (tab-separated) appears in log, in order.
> > + */
> >   static bool cmp_str_seq(const char *log, const char *exp)
> >   {
> > -     char needle[80];
> > +     char needle[200];
> >       const char *p, *q;
> >       int len;
> >
> > @@ -1015,7 +1020,7 @@ static bool cmp_str_seq(const char *log, const char *exp)
> >               needle[len] = 0;
> >               q = strstr(log, needle);
> >               if (!q) {
> > -                     printf("FAIL\nUnexpected verifier log in successful load!\n"
> > +                     printf("FAIL\nUnexpected verifier log!\n"
> >                              "EXP: %s\nRES:\n", needle);
> >                       return false;
> >               }
> > @@ -1130,7 +1135,11 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >                       printf("FAIL\nUnexpected success to load!\n");
> >                       goto fail_log;
> >               }
> > -             if (!expected_err || !strstr(bpf_vlog, expected_err)) {
> > +             if (!expected_err) {
> > +                     printf("FAIL\nTestcase bug; missing expected_err\n");
> > +                     goto fail_log;
>
> Do we have an in-tree case like this?

You're asking if there are tests with expected_res == REJECT and
expected_err == NULL?
There are no such test cases, and the intention of this "testcase bug"
check was to keep it that way.
I can simply fold it into the test failure below, as you're suggesting.

> Given this would also be visible below with 'EXP:'
> being (null), I might simplify and just replace the strstr() with cmp_str_seq().
>
> Also, could you elaborate on which test cases need the cmp_str_seq() conversion?

There are VERBOSE_ACCEPT tests that you a tab-separated list of
expected messages; see precise.c.
There are no such REJECT tests yet. I was about to introduce one in
another patch that's inflight, but I ended
up not needing to. Still, I figured that unifying the capabilities of
.errstr between VERBOSE_ACCEPT and REJECT
is a good idea. If you don't think so, I'm happy to drop this patch.

>
> > +             }
> > +             if ((strlen(expected_err) > 0) && !cmp_str_seq(bpf_vlog, expected_err)) {
>
> (nit: no extra '()' needed, but either way I'd rather opt for '!expected_err ||
> !cmp_str_seq(bpf_vlog, expected_err)' anyway)
>
> >                       printf("FAIL\nUnexpected error message!\n\tEXP: %s\n\tRES: %s\n",
> >                             expected_err, bpf_vlog);
> >                       goto fail_log;
> >
>
Daniel Borkmann Jan. 27, 2021, 11:24 p.m. UTC | #3
On 1/27/21 3:31 AM, Andrei Matei wrote:
> On Tue, Jan 26, 2021 at 6:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 1/24/21 8:05 PM, Andrei Matei wrote:
>>> This patch adds support to verifier tests to check for a succession of
>>> verifier log messages on program load failure. This makes the
>>> errstr field work uniformly across REJECT and VERBOSE_ACCEPT checks.
>>>
>>> This patch also increases the maximum size of an accepted series of
>>> messages to test from 80 chars to 200 chars. This is in order to keep
>>> existing tests working, which sometimes test for messages larger than 80
>>> chars (which was accepted in the REJECT case, when testing for a single
>>> message, but ironically not in the VERBOSE_ACCEPT case, when testing for
>>> possibly multiple messages).
>>> And example of such a long, checked message is in bounds.c:
>>> "R1 has unknown scalar with mixed signed bounds, pointer arithmetic with
>>> it prohibited for !root"
>>>
>>> Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
>>> ---
>>>    tools/testing/selftests/bpf/test_verifier.c | 15 ++++++++++++---
>>>    1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
>>> index 59bfa6201d1d..69298bf8ee86 100644
>>> --- a/tools/testing/selftests/bpf/test_verifier.c
>>> +++ b/tools/testing/selftests/bpf/test_verifier.c
>>> @@ -88,6 +88,9 @@ struct bpf_test {
>>>        int fixup_map_event_output[MAX_FIXUPS];
>>>        int fixup_map_reuseport_array[MAX_FIXUPS];
>>>        int fixup_map_ringbuf[MAX_FIXUPS];
>>> +     /* Expected verifier log output for result REJECT or VERBOSE_ACCEPT. Can be a
>>> +      * tab-separated sequence of expected strings.
>>> +      */
>>>        const char *errstr;
>>>        const char *errstr_unpriv;
>>>        uint32_t insn_processed;
>>> @@ -995,9 +998,11 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
>>>        return 0;
>>>    }
>>>
>>> +/* Returns true if every part of exp (tab-separated) appears in log, in order.
>>> + */
>>>    static bool cmp_str_seq(const char *log, const char *exp)
>>>    {
>>> -     char needle[80];
>>> +     char needle[200];
>>>        const char *p, *q;
>>>        int len;
>>>
>>> @@ -1015,7 +1020,7 @@ static bool cmp_str_seq(const char *log, const char *exp)
>>>                needle[len] = 0;
>>>                q = strstr(log, needle);
>>>                if (!q) {
>>> -                     printf("FAIL\nUnexpected verifier log in successful load!\n"
>>> +                     printf("FAIL\nUnexpected verifier log!\n"
>>>                               "EXP: %s\nRES:\n", needle);
>>>                        return false;
>>>                }
>>> @@ -1130,7 +1135,11 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
>>>                        printf("FAIL\nUnexpected success to load!\n");
>>>                        goto fail_log;
>>>                }
>>> -             if (!expected_err || !strstr(bpf_vlog, expected_err)) {
>>> +             if (!expected_err) {
>>> +                     printf("FAIL\nTestcase bug; missing expected_err\n");
>>> +                     goto fail_log;
>>
>> Do we have an in-tree case like this?
> 
> You're asking if there are tests with expected_res == REJECT and
> expected_err == NULL?
> There are no such test cases, and the intention of this "testcase bug"
> check was to keep it that way.
> I can simply fold it into the test failure below, as you're suggesting.

Yeah, I would just fold it given such issue would be visible there as well.

>> Given this would also be visible below with 'EXP:'
>> being (null), I might simplify and just replace the strstr() with cmp_str_seq().
>>
>> Also, could you elaborate on which test cases need the cmp_str_seq() conversion?
> 
> There are VERBOSE_ACCEPT tests that you a tab-separated list of
> expected messages; see precise.c.
> There are no such REJECT tests yet. I was about to introduce one in
> another patch that's inflight, but I ended
> up not needing to. Still, I figured that unifying the capabilities of
> .errstr between VERBOSE_ACCEPT and REJECT
> is a good idea.
I think unifying seems reasonable, lets do then.

Thanks,
Daniel
Andrei Matei Jan. 30, 2021, 10:03 p.m. UTC | #4
On Wed, Jan 27, 2021 at 6:24 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 1/27/21 3:31 AM, Andrei Matei wrote:
> > On Tue, Jan 26, 2021 at 6:21 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >> On 1/24/21 8:05 PM, Andrei Matei wrote:
> >>> This patch adds support to verifier tests to check for a succession of
> >>> verifier log messages on program load failure. This makes the
> >>> errstr field work uniformly across REJECT and VERBOSE_ACCEPT checks.
> >>>
> >>> This patch also increases the maximum size of an accepted series of
> >>> messages to test from 80 chars to 200 chars. This is in order to keep
> >>> existing tests working, which sometimes test for messages larger than 80
> >>> chars (which was accepted in the REJECT case, when testing for a single
> >>> message, but ironically not in the VERBOSE_ACCEPT case, when testing for
> >>> possibly multiple messages).
> >>> And example of such a long, checked message is in bounds.c:
> >>> "R1 has unknown scalar with mixed signed bounds, pointer arithmetic with
> >>> it prohibited for !root"
> >>>
> >>> Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> >>> ---
> >>>    tools/testing/selftests/bpf/test_verifier.c | 15 ++++++++++++---
> >>>    1 file changed, 12 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> >>> index 59bfa6201d1d..69298bf8ee86 100644
> >>> --- a/tools/testing/selftests/bpf/test_verifier.c
> >>> +++ b/tools/testing/selftests/bpf/test_verifier.c
> >>> @@ -88,6 +88,9 @@ struct bpf_test {
> >>>        int fixup_map_event_output[MAX_FIXUPS];
> >>>        int fixup_map_reuseport_array[MAX_FIXUPS];
> >>>        int fixup_map_ringbuf[MAX_FIXUPS];
> >>> +     /* Expected verifier log output for result REJECT or VERBOSE_ACCEPT. Can be a
> >>> +      * tab-separated sequence of expected strings.
> >>> +      */
> >>>        const char *errstr;
> >>>        const char *errstr_unpriv;
> >>>        uint32_t insn_processed;
> >>> @@ -995,9 +998,11 @@ static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
> >>>        return 0;
> >>>    }
> >>>
> >>> +/* Returns true if every part of exp (tab-separated) appears in log, in order.
> >>> + */
> >>>    static bool cmp_str_seq(const char *log, const char *exp)
> >>>    {
> >>> -     char needle[80];
> >>> +     char needle[200];
> >>>        const char *p, *q;
> >>>        int len;
> >>>
> >>> @@ -1015,7 +1020,7 @@ static bool cmp_str_seq(const char *log, const char *exp)
> >>>                needle[len] = 0;
> >>>                q = strstr(log, needle);
> >>>                if (!q) {
> >>> -                     printf("FAIL\nUnexpected verifier log in successful load!\n"
> >>> +                     printf("FAIL\nUnexpected verifier log!\n"
> >>>                               "EXP: %s\nRES:\n", needle);
> >>>                        return false;
> >>>                }
> >>> @@ -1130,7 +1135,11 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >>>                        printf("FAIL\nUnexpected success to load!\n");
> >>>                        goto fail_log;
> >>>                }
> >>> -             if (!expected_err || !strstr(bpf_vlog, expected_err)) {
> >>> +             if (!expected_err) {
> >>> +                     printf("FAIL\nTestcase bug; missing expected_err\n");
> >>> +                     goto fail_log;
> >>
> >> Do we have an in-tree case like this?
> >
> > You're asking if there are tests with expected_res == REJECT and
> > expected_err == NULL?
> > There are no such test cases, and the intention of this "testcase bug"
> > check was to keep it that way.
> > I can simply fold it into the test failure below, as you're suggesting.
>
> Yeah, I would just fold it given such issue would be visible there as well.
>
> >> Given this would also be visible below with 'EXP:'
> >> being (null), I might simplify and just replace the strstr() with cmp_str_seq().
> >>
> >> Also, could you elaborate on which test cases need the cmp_str_seq() conversion?
> >
> > There are VERBOSE_ACCEPT tests that you a tab-separated list of
> > expected messages; see precise.c.
> > There are no such REJECT tests yet. I was about to introduce one in
> > another patch that's inflight, but I ended
> > up not needing to. Still, I figured that unifying the capabilities of
> > .errstr between VERBOSE_ACCEPT and REJECT
> > is a good idea.
> I think unifying seems reasonable, lets do then.

Please see v2. I've had to do a small change to cmp_str_seq to have it
declare success when looking for "". This is to keep a couple of
existing tests happy which expected REJECT but didn't want to check
any log message.

>
> Thanks,
> Daniel
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 59bfa6201d1d..69298bf8ee86 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -88,6 +88,9 @@  struct bpf_test {
 	int fixup_map_event_output[MAX_FIXUPS];
 	int fixup_map_reuseport_array[MAX_FIXUPS];
 	int fixup_map_ringbuf[MAX_FIXUPS];
+	/* Expected verifier log output for result REJECT or VERBOSE_ACCEPT. Can be a
+	 * tab-separated sequence of expected strings.
+	 */
 	const char *errstr;
 	const char *errstr_unpriv;
 	uint32_t insn_processed;
@@ -995,9 +998,11 @@  static int do_prog_test_run(int fd_prog, bool unpriv, uint32_t expected_val,
 	return 0;
 }
 
+/* Returns true if every part of exp (tab-separated) appears in log, in order.
+ */
 static bool cmp_str_seq(const char *log, const char *exp)
 {
-	char needle[80];
+	char needle[200];
 	const char *p, *q;
 	int len;
 
@@ -1015,7 +1020,7 @@  static bool cmp_str_seq(const char *log, const char *exp)
 		needle[len] = 0;
 		q = strstr(log, needle);
 		if (!q) {
-			printf("FAIL\nUnexpected verifier log in successful load!\n"
+			printf("FAIL\nUnexpected verifier log!\n"
 			       "EXP: %s\nRES:\n", needle);
 			return false;
 		}
@@ -1130,7 +1135,11 @@  static void do_test_single(struct bpf_test *test, bool unpriv,
 			printf("FAIL\nUnexpected success to load!\n");
 			goto fail_log;
 		}
-		if (!expected_err || !strstr(bpf_vlog, expected_err)) {
+		if (!expected_err) {
+			printf("FAIL\nTestcase bug; missing expected_err\n");
+			goto fail_log;
+		}
+		if ((strlen(expected_err) > 0) && !cmp_str_seq(bpf_vlog, expected_err)) {
 			printf("FAIL\nUnexpected error message!\n\tEXP: %s\n\tRES: %s\n",
 			      expected_err, bpf_vlog);
 			goto fail_log;