bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: John Fastabend <john.fastabend@gmail.com>
To: Andrii Nakryiko <andriin@fb.com>,
	bpf@vger.kernel.org, netdev@vger.kernel.org, ast@fb.com,
	daniel@iogearbox.net
Cc: andrii.nakryiko@gmail.com, kernel-team@fb.com,
	Andrii Nakryiko <andriin@fb.com>, Christoph Hellwig <hch@lst.de>
Subject: RE: [PATCH bpf 2/2] selftests/bpf: add variable-length data concatenation pattern test
Date: Thu, 18 Jun 2020 12:09:35 -0700	[thread overview]
Message-ID: <5eebbbef8f904_6d292ad5e7a285b883@john-XPS-13-9370.notmuch> (raw)
In-Reply-To: <20200616050432.1902042-2-andriin@fb.com>

Andrii Nakryiko wrote:
> Add selftest that validates variable-length data reading and concatentation
> with one big shared data array. This is a common pattern in production use for
> monitoring and tracing applications, that potentially can read a lot of data,
> but usually reads much less. Such pattern allows to determine precisely what
> amount of data needs to be sent over perfbuf/ringbuf and maximize efficiency.
> 
> This is the first BPF selftest that at all looks at and tests
> bpf_probe_read_str()-like helper's return value, closing a major gap in BPF
> testing. It surfaced the problem with bpf_probe_read_kernel_str() returning
> 0 on success, instead of amount of bytes successfully read.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---

[...]

> +/* .data */
> +int payload2_len1 = -1;
> +int payload2_len2 = -1;
> +int total2 = -1;
> +char payload2[MAX_LEN + MAX_LEN] = { 1 };
> +
> +SEC("raw_tp/sys_enter")
> +int handler64(void *regs)
> +{
> +	int pid = bpf_get_current_pid_tgid() >> 32;
> +	void *payload = payload1;
> +	u64 len;
> +
> +	/* ignore irrelevant invocations */
> +	if (test_pid != pid || !capture)
> +		return 0;
> +
> +	len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in1[0]);
> +	if (len <= MAX_LEN) {

Took me a bit grok this. You are relying on the fact that in errors,
such as a page fault, will encode to a large u64 value and so you
verifier is happy. But most of my programs actually want to distinguish
between legitimate errors on the probe vs buffer overrun cases.

Can we make these tests do explicit check for errors. For example,

  if (len < 0) goto abort;

But this also breaks your types here. This is what I was trying to
point out in the 1/2 patch thread. Wanted to make the point here as
well in case it wasn't clear. Not sure I did the best job explaining.

> +		payload += len;
> +		payload1_len1 = len;
> +	}
> +
> +	len = bpf_probe_read_kernel_str(payload, MAX_LEN, &buf_in2[0]);
> +	if (len <= MAX_LEN) {
> +		payload += len;
> +		payload1_len2 = len;
> +	}
> +
> +	total1 = payload - (void *)payload1;
> +
> +	return 0;
> +}

  parent reply	other threads:[~2020-06-18 19:09 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-16  5:04 [PATCH bpf 1/2] bpf: bpf_probe_read_kernel_str() has to return amount of data read on success Andrii Nakryiko
2020-06-16  5:04 ` [PATCH bpf 2/2] selftests/bpf: add variable-length data concatenation pattern test Andrii Nakryiko
2020-06-16 20:21   ` Daniel Borkmann
2020-06-16 21:27     ` Andrii Nakryiko
2020-06-16 22:23       ` Daniel Borkmann
2020-06-16 23:14         ` Andrii Nakryiko
2020-06-17 15:51           ` Daniel Borkmann
2020-06-18 19:09   ` John Fastabend [this message]
2020-06-18 21:59     ` Andrii Nakryiko
2020-06-18 23:48       ` John Fastabend
2020-06-19  0:09         ` Andrii Nakryiko
2020-06-16  6:54 ` [PATCH bpf 1/2] bpf: bpf_probe_read_kernel_str() has to return amount of data read on success Christoph Hellwig
2020-06-16  7:01 ` John Fastabend

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5eebbbef8f904_6d292ad5e7a285b883@john-XPS-13-9370.notmuch \
    --to=john.fastabend@gmail.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=hch@lst.de \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).