[PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

[PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

[Lorenz Bauer]

Use a proper temporary file for sendpage tests. This means that running
the tests doesn't clutter the working directory, and allows running the
test on read-only filesystems.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 4a851513c842..779e11da979c 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 	FILE *file;
 	int i, fp;
 
-	file = fopen(".sendpage_tst.tmp", "w+");
+	file = tmpfile();
 	if (!file) {
 		perror("create file for sendpage");
 		return 1;
@@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 		fwrite(&k, sizeof(char), 1, file);
 	fflush(file);
 	fseek(file, 0, SEEK_SET);
-	fclose(file);
 
-	fp = open(".sendpage_tst.tmp", O_RDONLY);
-	if (fp < 0) {
-		perror("reopen file for sendpage");
-		return 1;
-	}
+	fp = fileno(file);
 
 	clock_gettime(CLOCK_MONOTONIC, &s->start);
 	for (i = 0; i < cnt; i++) {
@@ -354,11 +349,11 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 
 		if (!drop && sent < 0) {
 			perror("send loop error");
-			close(fp);
+			fclose(file);
 			return sent;
 		} else if (drop && sent >= 0) {
 			printf("sendpage loop error expected: %i\n", sent);
-			close(fp);
+			fclose(file);
 			return -EIO;
 		}
 
@@ -366,7 +361,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 			s->bytes_sent += sent;
 	}
 	clock_gettime(CLOCK_MONOTONIC, &s->end);
-	close(fp);
+	fclose(file);
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

[Martin Lau]

On Thu, Jan 23, 2020 at 04:59:30PM +0000, Lorenz Bauer wrote:
> Use a proper temporary file for sendpage tests. This means that running
> the tests doesn't clutter the working directory, and allows running the
> test on read-only filesystems.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index 4a851513c842..779e11da979c 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
>  	FILE *file;
>  	int i, fp;
>  
> -	file = fopen(".sendpage_tst.tmp", "w+");
> +	file = tmpfile();
>  	if (!file) {
>  		perror("create file for sendpage");
>  		return 1;
> @@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
>  		fwrite(&k, sizeof(char), 1, file);
>  	fflush(file);
>  	fseek(file, 0, SEEK_SET);
> -	fclose(file);
>  
> -	fp = open(".sendpage_tst.tmp", O_RDONLY);
> -	if (fp < 0) {
> -		perror("reopen file for sendpage");
> -		return 1;
> -	}
> +	fp = fileno(file);
It may be better to keep fp == -1 check here.
It is not clear to me the original intention of reopen.
I would defer to John for comment.

>  
>  	clock_gettime(CLOCK_MONOTONIC, &s->start);
>  	for (i = 0; i < cnt; i++) {

Re: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

[John Fastabend]

Martin Lau wrote:
> On Thu, Jan 23, 2020 at 04:59:30PM +0000, Lorenz Bauer wrote:
> > Use a proper temporary file for sendpage tests. This means that running
> > the tests doesn't clutter the working directory, and allows running the
> > test on read-only filesystems.
> > 
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
> >  tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> > index 4a851513c842..779e11da979c 100644
> > --- a/tools/testing/selftests/bpf/test_sockmap.c
> > +++ b/tools/testing/selftests/bpf/test_sockmap.c
> > @@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
> >  	FILE *file;
> >  	int i, fp;
> >  
> > -	file = fopen(".sendpage_tst.tmp", "w+");
> > +	file = tmpfile();
> >  	if (!file) {
> >  		perror("create file for sendpage");
> >  		return 1;
> > @@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
> >  		fwrite(&k, sizeof(char), 1, file);
> >  	fflush(file);
> >  	fseek(file, 0, SEEK_SET);
> > -	fclose(file);
> >  
> > -	fp = open(".sendpage_tst.tmp", O_RDONLY);
> > -	if (fp < 0) {
> > -		perror("reopen file for sendpage");
> > -		return 1;
> > -	}
> > +	fp = fileno(file);
> It may be better to keep fp == -1 check here.
> It is not clear to me the original intention of reopen.
> I would defer to John for comment.
> 

Seeing fileno shouldn't fail seems OK to me.

> >  
> >  	clock_gettime(CLOCK_MONOTONIC, &s->start);
> >  	for (i = 0; i < cnt; i++) {

Re: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

[Jakub Sitnicki]

On Thu, Jan 23, 2020 at 05:59 PM CET, Lorenz Bauer wrote:
> Use a proper temporary file for sendpage tests. This means that running
> the tests doesn't clutter the working directory, and allows running the
> test on read-only filesystems.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index 4a851513c842..779e11da979c 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
>  	FILE *file;
>  	int i, fp;
>
> -	file = fopen(".sendpage_tst.tmp", "w+");
> +	file = tmpfile();
>  	if (!file) {
>  		perror("create file for sendpage");
>  		return 1;

memfd_create() would be an alternative that doesn't clutter anything.

[...]

RE: [PATCH bpf 1/4] selftests: bpf: use a temporary file in test_sockmap

[John Fastabend]

Lorenz Bauer wrote:
> Use a proper temporary file for sendpage tests. This means that running
> the tests doesn't clutter the working directory, and allows running the
> test on read-only filesystems.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 

Agree nice bit of cleanup. We should merge this in bpf-next though
considering its late in the bpf tree.

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf 2/4] selftests: bpf: ignore RST packets for reuseport tests

[Lorenz Bauer]

The reuseport tests currently suffer from a race condition: RST
packets count towards DROP_ERR_SKB_DATA, since they don't contain
a valid struct cmd. Tests will spuriously fail depending on whether
check_results is called before or after the RST is processed.

Exit the BPF program early if FIN is set.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 .../selftests/bpf/progs/test_select_reuseport_kern.c        | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c b/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
index d69a1f2bbbfd..26e77dcc7e91 100644
--- a/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
@@ -113,6 +113,12 @@ int _select_by_skb_data(struct sk_reuseport_md *reuse_md)
 		data_check.skb_ports[0] = th->source;
 		data_check.skb_ports[1] = th->dest;
 
+		if (th->fin)
+			/* The connection is being torn down at the end of a
+			 * test. It can't contain a cmd, so return early.
+			 */
+			return SK_PASS;
+
 		if ((th->doff << 2) + sizeof(*cmd) > data_check.len)
 			GOTO_DONE(DROP_ERR_SKB_DATA);
 		if (bpf_skb_load_bytes(reuse_md, th->doff << 2, &cmd_copy,
-- 
2.20.1

Re: [PATCH bpf 2/4] selftests: bpf: ignore RST packets for reuseport tests

[Martin Lau]

On Thu, Jan 23, 2020 at 04:59:31PM +0000, Lorenz Bauer wrote:
> The reuseport tests currently suffer from a race condition: RST
> packets count towards DROP_ERR_SKB_DATA, since they don't contain
> a valid struct cmd. Tests will spuriously fail depending on whether
> check_results is called before or after the RST is processed.
> 
> Exit the BPF program early if FIN is set.
Make sense.
Is it a RST or FIN?  The earlier commit message said RST.

Re: [PATCH bpf 2/4] selftests: bpf: ignore RST packets for reuseport tests

[Lorenz Bauer]

On Thu, 23 Jan 2020 at 17:16, Martin Lau <kafai@fb.com> wrote:
>
> On Thu, Jan 23, 2020 at 04:59:31PM +0000, Lorenz Bauer wrote:
> > The reuseport tests currently suffer from a race condition: RST
> > packets count towards DROP_ERR_SKB_DATA, since they don't contain
> > a valid struct cmd. Tests will spuriously fail depending on whether
> > check_results is called before or after the RST is processed.
> >
> > Exit the BPF program early if FIN is set.
> Make sense.
> Is it a RST or FIN?  The earlier commit message said RST.

FIN, sorry. I'll update in a follow up.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf 2/4] selftests: bpf: ignore RST packets for reuseport tests

[Martin Lau]

On Thu, Jan 23, 2020 at 04:59:31PM +0000, Lorenz Bauer wrote:
> The reuseport tests currently suffer from a race condition: RST
> packets count towards DROP_ERR_SKB_DATA, since they don't contain
> a valid struct cmd. Tests will spuriously fail depending on whether
> check_results is called before or after the RST is processed.
> 
> Exit the BPF program early if FIN is set.
btw, it needs a Fixes tag.

Patch 4 and Patch 1 also need a Fixes tag.

Re: [PATCH bpf 2/4] selftests: bpf: ignore RST packets for reuseport tests

[Lorenz Bauer]

On Thu, 23 Jan 2020 at 21:54, Martin Lau <kafai@fb.com> wrote:
>
> btw, it needs a Fixes tag.
>
> Patch 4 and Patch 1 also need a Fixes tag.

This makes me wonder, should these go via bpf or bpf-next? Do I have
to split the series then?

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf 2/4] selftests: bpf: ignore RST packets for reuseport tests

[Daniel Borkmann]

On 1/24/20 10:00 AM, Lorenz Bauer wrote:
> On Thu, 23 Jan 2020 at 21:54, Martin Lau <kafai@fb.com> wrote:
>>
>> btw, it needs a Fixes tag.
>>
>> Patch 4 and Patch 1 also need a Fixes tag.
> 
> This makes me wonder, should these go via bpf or bpf-next? Do I have
> to split the series then?

Lets do all of these for bpf-next since timing is very close before v5.5 release.
If needed, we can later have them picked up for 5.5 stable.

Thanks,
Daniel

RE: [PATCH bpf 2/4] selftests: bpf: ignore RST packets for reuseport tests

[John Fastabend]

Lorenz Bauer wrote:
> The reuseport tests currently suffer from a race condition: RST
> packets count towards DROP_ERR_SKB_DATA, since they don't contain
> a valid struct cmd. Tests will spuriously fail depending on whether
> check_results is called before or after the RST is processed.
> 
> Exit the BPF program early if FIN is set.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  .../selftests/bpf/progs/test_select_reuseport_kern.c        | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c b/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
> index d69a1f2bbbfd..26e77dcc7e91 100644
> --- a/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
> @@ -113,6 +113,12 @@ int _select_by_skb_data(struct sk_reuseport_md *reuse_md)
>  		data_check.skb_ports[0] = th->source;
>  		data_check.skb_ports[1] = th->dest;
>  
> +		if (th->fin)
> +			/* The connection is being torn down at the end of a
> +			 * test. It can't contain a cmd, so return early.
> +			 */
> +			return SK_PASS;
> +
>  		if ((th->doff << 2) + sizeof(*cmd) > data_check.len)
>  			GOTO_DONE(DROP_ERR_SKB_DATA);
>  		if (bpf_skb_load_bytes(reuse_md, th->doff << 2, &cmd_copy,
> -- 
> 2.20.1
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf 3/4] selftests: bpf: make reuseport test output more legible

[Lorenz Bauer]

Include the name of the mismatching result in human readable format
when reporting an error. The new output looks like the following:

  unexpected result
   result: [1, 0, 0, 0, 0, 0]
  expected: [0, 0, 0, 0, 0, 0]
  mismatch on DROP_ERR_INNER_MAP (bpf_prog_linum:153)
  check_results:FAIL:382

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 .../bpf/prog_tests/select_reuseport.c         | 30 ++++++++++++++++---
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
index 2c37ae7dc214..09a536af139a 100644
--- a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
+++ b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
@@ -316,6 +316,28 @@ static void check_data(int type, sa_family_t family, const struct cmd *cmd,
 		       expected.len, result.len, get_linum());
 }
 
+static const char *result_to_str(enum result res)
+{
+	switch (res) {
+	case DROP_ERR_INNER_MAP:
+		return "DROP_ERR_INNER_MAP";
+	case DROP_ERR_SKB_DATA:
+		return "DROP_ERR_SKB_DATA";
+	case DROP_ERR_SK_SELECT_REUSEPORT:
+		return "DROP_ERR_SK_SELECT_REUSEPORT";
+	case DROP_MISC:
+		return "DROP_MISC";
+	case PASS:
+		return "PASS";
+	case PASS_ERR_SK_SELECT_REUSEPORT:
+		return "PASS_ERR_SK_SELECT_REUSEPORT";
+	case NR_RESULTS:
+		return "NR_RESULTS";
+	default:
+		return "UNKNOWN";
+	}
+}
+
 static void check_results(void)
 {
 	__u32 results[NR_RESULTS];
@@ -351,10 +373,10 @@ static void check_results(void)
 		printf(", %u", expected_results[i]);
 	printf("]\n");
 
-	RET_IF(expected_results[broken] != results[broken],
-	       "unexpected result",
-	       "expected_results[%u] != results[%u] bpf_prog_linum:%ld\n",
-	       broken, broken, get_linum());
+	printf("mismatch on %s (bpf_prog_linum:%ld)\n", result_to_str(broken),
+	       get_linum());
+
+	CHECK_FAIL(true);
 }
 
 static int send_data(int type, sa_family_t family, void *data, size_t len,
-- 
2.20.1

Re: [PATCH bpf 3/4] selftests: bpf: make reuseport test output more legible

[Martin Lau]

On Thu, Jan 23, 2020 at 04:59:32PM +0000, Lorenz Bauer wrote:
> Include the name of the mismatching result in human readable format
> when reporting an error. The new output looks like the following:
> 
>   unexpected result
>    result: [1, 0, 0, 0, 0, 0]
>   expected: [0, 0, 0, 0, 0, 0]
>   mismatch on DROP_ERR_INNER_MAP (bpf_prog_linum:153)
>   check_results:FAIL:382
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  .../bpf/prog_tests/select_reuseport.c         | 30 ++++++++++++++++---
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
> index 2c37ae7dc214..09a536af139a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
> +++ b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
> @@ -316,6 +316,28 @@ static void check_data(int type, sa_family_t family, const struct cmd *cmd,
>  		       expected.len, result.len, get_linum());
>  }
>  
> +static const char *result_to_str(enum result res)
> +{
> +	switch (res) {
> +	case DROP_ERR_INNER_MAP:
> +		return "DROP_ERR_INNER_MAP";
> +	case DROP_ERR_SKB_DATA:
> +		return "DROP_ERR_SKB_DATA";
> +	case DROP_ERR_SK_SELECT_REUSEPORT:
> +		return "DROP_ERR_SK_SELECT_REUSEPORT";
> +	case DROP_MISC:
> +		return "DROP_MISC";
> +	case PASS:
> +		return "PASS";
> +	case PASS_ERR_SK_SELECT_REUSEPORT:
> +		return "PASS_ERR_SK_SELECT_REUSEPORT";
> +	case NR_RESULTS:
This should return "UNKNOWN" also.

> +		return "NR_RESULTS";
> +	default:
> +		return "UNKNOWN";
> +	}
> +}
> +

RE: [PATCH bpf 3/4] selftests: bpf: make reuseport test output more legible

[John Fastabend]

Lorenz Bauer wrote:
> Include the name of the mismatching result in human readable format
> when reporting an error. The new output looks like the following:
> 
>   unexpected result
>    result: [1, 0, 0, 0, 0, 0]
>   expected: [0, 0, 0, 0, 0, 0]
>   mismatch on DROP_ERR_INNER_MAP (bpf_prog_linum:153)
>   check_results:FAIL:382
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  .../bpf/prog_tests/select_reuseport.c         | 30 ++++++++++++++++---
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
> index 2c37ae7dc214..09a536af139a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
> +++ b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
> @@ -316,6 +316,28 @@ static void check_data(int type, sa_family_t family, const struct cmd *cmd,
>  		       expected.len, result.len, get_linum());
>  }
>  
> +static const char *result_to_str(enum result res)
> +{
> +	switch (res) {
> +	case DROP_ERR_INNER_MAP:
> +		return "DROP_ERR_INNER_MAP";
> +	case DROP_ERR_SKB_DATA:
> +		return "DROP_ERR_SKB_DATA";
> +	case DROP_ERR_SK_SELECT_REUSEPORT:
> +		return "DROP_ERR_SK_SELECT_REUSEPORT";
> +	case DROP_MISC:
> +		return "DROP_MISC";
> +	case PASS:
> +		return "PASS";
> +	case PASS_ERR_SK_SELECT_REUSEPORT:
> +		return "PASS_ERR_SK_SELECT_REUSEPORT";
> +	case NR_RESULTS:
> +		return "NR_RESULTS";
> +	default:
> +		return "UNKNOWN";
> +	}
> +}
> +
>  static void check_results(void)
>  {
>  	__u32 results[NR_RESULTS];
> @@ -351,10 +373,10 @@ static void check_results(void)
>  		printf(", %u", expected_results[i]);
>  	printf("]\n");
>  
> -	RET_IF(expected_results[broken] != results[broken],
> -	       "unexpected result",
> -	       "expected_results[%u] != results[%u] bpf_prog_linum:%ld\n",
> -	       broken, broken, get_linum());
> +	printf("mismatch on %s (bpf_prog_linum:%ld)\n", result_to_str(broken),
> +	       get_linum());
> +
> +	CHECK_FAIL(true);
>  }
>  
>  static int send_data(int type, sa_family_t family, void *data, size_t len,
> -- 
> 2.20.1
> 

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf 4/4] selftests: bpf: reset global state between reuseport test runs

[Lorenz Bauer]

Currently, there is a lot of false positives if a single reuseport test
fails. This is because expected_results and the result map are not cleared.

Zero both after individual test runs, which fixes the mentioned false
positives.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 .../selftests/bpf/prog_tests/select_reuseport.c    | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
index 09a536af139a..0bab8b1ca1c3 100644
--- a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
+++ b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
@@ -699,7 +699,19 @@ static void setup_per_test(int type, sa_family_t family, bool inany,
 
 static void cleanup_per_test(bool no_inner_map)
 {
-	int i, err;
+	int i, err, zero = 0;
+
+	memset(expected_results, 0, sizeof(expected_results));
+
+	for (i = 0; i < NR_RESULTS; i++) {
+		err = bpf_map_update_elem(result_map, &i, &zero, BPF_ANY);
+		RET_IF(err, "reset elem in result_map",
+		       "i:%u err:%d errno:%d\n", i, err, errno);
+	}
+
+	err = bpf_map_update_elem(linum_map, &zero, &zero, BPF_ANY);
+	RET_IF(err, "reset line number in linum_map", "err:%d errno:%d\n",
+	       err, errno);
 
 	for (i = 0; i < REUSEPORT_ARRAY_SIZE; i++)
 		close(sk_fds[i]);
-- 
2.20.1

Re: [PATCH bpf 4/4] selftests: bpf: reset global state between reuseport test runs

[Martin Lau]

On Thu, Jan 23, 2020 at 04:59:33PM +0000, Lorenz Bauer wrote:
> Currently, there is a lot of false positives if a single reuseport test
> fails. This is because expected_results and the result map are not cleared.
Ah, right.  An earlier test failure has ripple effect on the following tests.

I notice another embarrassing typo.  Can you also make this change in this fix?

-static enum result expected_results[NR_RESULTS];
+static __u32 expected_results[NR_RESULTS];

> 
> Zero both after individual test runs, which fixes the mentioned false
> positives.
Thanks for the fix!

Acked-by: Martin KaFai Lau <kafai@fb.com>

[PATCH bpf-next v2 1/4] selftests: bpf: use a temporary file in test_sockmap

[Lorenz Bauer]

Use a proper temporary file for sendpage tests. This means that running
the tests doesn't clutter the working directory, and allows running the
test on read-only filesystems.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 16962b2404ac ("bpf: sockmap, add selftests")
---
 tools/testing/selftests/bpf/test_sockmap.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
index 4a851513c842..779e11da979c 100644
--- a/tools/testing/selftests/bpf/test_sockmap.c
+++ b/tools/testing/selftests/bpf/test_sockmap.c
@@ -331,7 +331,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 	FILE *file;
 	int i, fp;
 
-	file = fopen(".sendpage_tst.tmp", "w+");
+	file = tmpfile();
 	if (!file) {
 		perror("create file for sendpage");
 		return 1;
@@ -340,13 +340,8 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 		fwrite(&k, sizeof(char), 1, file);
 	fflush(file);
 	fseek(file, 0, SEEK_SET);
-	fclose(file);
 
-	fp = open(".sendpage_tst.tmp", O_RDONLY);
-	if (fp < 0) {
-		perror("reopen file for sendpage");
-		return 1;
-	}
+	fp = fileno(file);
 
 	clock_gettime(CLOCK_MONOTONIC, &s->start);
 	for (i = 0; i < cnt; i++) {
@@ -354,11 +349,11 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 
 		if (!drop && sent < 0) {
 			perror("send loop error");
-			close(fp);
+			fclose(file);
 			return sent;
 		} else if (drop && sent >= 0) {
 			printf("sendpage loop error expected: %i\n", sent);
-			close(fp);
+			fclose(file);
 			return -EIO;
 		}
 
@@ -366,7 +361,7 @@ static int msg_loop_sendpage(int fd, int iov_length, int cnt,
 			s->bytes_sent += sent;
 	}
 	clock_gettime(CLOCK_MONOTONIC, &s->end);
-	close(fp);
+	fclose(file);
 	return 0;
 }
 
-- 
2.20.1

[PATCH bpf-next v2 2/4] selftests: bpf: ignore FIN packets for reuseport tests

[Lorenz Bauer]

The reuseport tests currently suffer from a race condition: FIN
packets count towards DROP_ERR_SKB_DATA, since they don't contain
a valid struct cmd. Tests will spuriously fail depending on whether
check_results is called before or after the FIN is processed.

Exit the BPF program early if FIN is set.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 91134d849a0e ("bpf: Test BPF_PROG_TYPE_SK_REUSEPORT")
---
 .../selftests/bpf/progs/test_select_reuseport_kern.c        | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c b/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
index d69a1f2bbbfd..26e77dcc7e91 100644
--- a/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_select_reuseport_kern.c
@@ -113,6 +113,12 @@ int _select_by_skb_data(struct sk_reuseport_md *reuse_md)
 		data_check.skb_ports[0] = th->source;
 		data_check.skb_ports[1] = th->dest;
 
+		if (th->fin)
+			/* The connection is being torn down at the end of a
+			 * test. It can't contain a cmd, so return early.
+			 */
+			return SK_PASS;
+
 		if ((th->doff << 2) + sizeof(*cmd) > data_check.len)
 			GOTO_DONE(DROP_ERR_SKB_DATA);
 		if (bpf_skb_load_bytes(reuse_md, th->doff << 2, &cmd_copy,
-- 
2.20.1

[PATCH bpf-next v2 3/4] selftests: bpf: make reuseport test output more legible

[Lorenz Bauer]

Include the name of the mismatching result in human readable format
when reporting an error. The new output looks like the following:

  unexpected result
   result: [1, 0, 0, 0, 0, 0]
  expected: [0, 0, 0, 0, 0, 0]
  mismatch on DROP_ERR_INNER_MAP (bpf_prog_linum:153)
  check_results:FAIL:382

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 .../bpf/prog_tests/select_reuseport.c         | 28 ++++++++++++++++---
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
index 2c37ae7dc214..e7e56929751c 100644
--- a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
+++ b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
@@ -316,6 +316,26 @@ static void check_data(int type, sa_family_t family, const struct cmd *cmd,
 		       expected.len, result.len, get_linum());
 }
 
+static const char *result_to_str(enum result res)
+{
+	switch (res) {
+	case DROP_ERR_INNER_MAP:
+		return "DROP_ERR_INNER_MAP";
+	case DROP_ERR_SKB_DATA:
+		return "DROP_ERR_SKB_DATA";
+	case DROP_ERR_SK_SELECT_REUSEPORT:
+		return "DROP_ERR_SK_SELECT_REUSEPORT";
+	case DROP_MISC:
+		return "DROP_MISC";
+	case PASS:
+		return "PASS";
+	case PASS_ERR_SK_SELECT_REUSEPORT:
+		return "PASS_ERR_SK_SELECT_REUSEPORT";
+	default:
+		return "UNKNOWN";
+	}
+}
+
 static void check_results(void)
 {
 	__u32 results[NR_RESULTS];
@@ -351,10 +371,10 @@ static void check_results(void)
 		printf(", %u", expected_results[i]);
 	printf("]\n");
 
-	RET_IF(expected_results[broken] != results[broken],
-	       "unexpected result",
-	       "expected_results[%u] != results[%u] bpf_prog_linum:%ld\n",
-	       broken, broken, get_linum());
+	printf("mismatch on %s (bpf_prog_linum:%ld)\n", result_to_str(broken),
+	       get_linum());
+
+	CHECK_FAIL(true);
 }
 
 static int send_data(int type, sa_family_t family, void *data, size_t len,
-- 
2.20.1

[PATCH bpf-next v2 4/4] selftests: bpf: reset global state between reuseport test runs

[Lorenz Bauer]

Currently, there is a lot of false positives if a single reuseport test
fails. This is because expected_results and the result map are not cleared.

Zero both after individual test runs, which fixes the mentioned false
positives.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 91134d849a0e ("bpf: Test BPF_PROG_TYPE_SK_REUSEPORT")
---
 .../selftests/bpf/prog_tests/select_reuseport.c  | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
index e7e56929751c..098bcae5f827 100644
--- a/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
+++ b/tools/testing/selftests/bpf/prog_tests/select_reuseport.c
@@ -33,7 +33,7 @@
 #define REUSEPORT_ARRAY_SIZE 32
 
 static int result_map, tmp_index_ovr_map, linum_map, data_check_map;
-static enum result expected_results[NR_RESULTS];
+static __u32 expected_results[NR_RESULTS];
 static int sk_fds[REUSEPORT_ARRAY_SIZE];
 static int reuseport_array = -1, outer_map = -1;
 static int select_by_skb_data_prog;
@@ -697,7 +697,19 @@ static void setup_per_test(int type, sa_family_t family, bool inany,
 
 static void cleanup_per_test(bool no_inner_map)
 {
-	int i, err;
+	int i, err, zero = 0;
+
+	memset(expected_results, 0, sizeof(expected_results));
+
+	for (i = 0; i < NR_RESULTS; i++) {
+		err = bpf_map_update_elem(result_map, &i, &zero, BPF_ANY);
+		RET_IF(err, "reset elem in result_map",
+		       "i:%u err:%d errno:%d\n", i, err, errno);
+	}
+
+	err = bpf_map_update_elem(linum_map, &zero, &zero, BPF_ANY);
+	RET_IF(err, "reset line number in linum_map", "err:%d errno:%d\n",
+	       err, errno);
 
 	for (i = 0; i < REUSEPORT_ARRAY_SIZE; i++)
 		close(sk_fds[i]);
-- 
2.20.1