[PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Stanislav Fomichev]

Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
  * 128-bit (uuid)
  * 160-bit (sha1)
  * any length specified in ld --build-id=0xhexstring

To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
assume that build-id is somewhere in the range of 1 .. 20.
Set the remaining bytes to zero.

v2:
* don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
  we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
  this 'if' condition

Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/stackmap.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index d9e2483669d0..f9df545e92f6 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -180,11 +180,14 @@ static inline int stack_map_parse_build_id(void *page_addr,
 
 		if (nhdr->n_type == BPF_BUILD_ID &&
 		    nhdr->n_namesz == sizeof("GNU") &&
-		    nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
+		    nhdr->n_descsz > 0 &&
+		    nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
 			memcpy(build_id,
 			       note_start + note_offs +
 			       ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
-			       BPF_BUILD_ID_SIZE);
+			       nhdr->n_descsz);
+			memset(build_id + nhdr->n_descsz, 0,
+			       BPF_BUILD_ID_SIZE - nhdr->n_descsz);
 			return 0;
 		}
 		new_offs = note_offs + sizeof(Elf32_Nhdr) +
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Song Liu]

> On Jan 16, 2019, at 10:11 AM, Stanislav Fomichev <sdf@google.com> wrote:
> 
> Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
>  * 128-bit (uuid)
>  * 160-bit (sha1)
>  * any length specified in ld --build-id=0xhexstring
> 
> To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> assume that build-id is somewhere in the range of 1 .. 20.
> Set the remaining bytes to zero.
> 
> v2:
> * don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
>  we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
>  this 'if' condition
> 
> Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Acked-by: Song Liu <songliubraving@fb.com>
 

> ---
> kernel/bpf/stackmap.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index d9e2483669d0..f9df545e92f6 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -180,11 +180,14 @@ static inline int stack_map_parse_build_id(void *page_addr,
> 
> 		if (nhdr->n_type == BPF_BUILD_ID &&
> 		    nhdr->n_namesz == sizeof("GNU") &&
> -		    nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
> +		    nhdr->n_descsz > 0 &&
> +		    nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
> 			memcpy(build_id,
> 			       note_start + note_offs +
> 			       ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
> -			       BPF_BUILD_ID_SIZE);
> +			       nhdr->n_descsz);
> +			memset(build_id + nhdr->n_descsz, 0,
> +			       BPF_BUILD_ID_SIZE - nhdr->n_descsz);
> 			return 0;
> 		}
> 		new_offs = note_offs + sizeof(Elf32_Nhdr) +
> -- 
> 2.20.1.97.g81188d93c3-goog
> 

Re: [PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Daniel Borkmann]

On 01/16/2019 07:11 PM, Stanislav Fomichev wrote:
> Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
>   * 128-bit (uuid)
>   * 160-bit (sha1)
>   * any length specified in ld --build-id=0xhexstring
> 
> To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> assume that build-id is somewhere in the range of 1 .. 20.
> Set the remaining bytes to zero.
> 
> v2:
> * don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
>   we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
>   this 'if' condition
> 
> Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Hmm, looks like rest of the v2 series didn't make it to the list.
Please double check; just in case simply resend the full v2 set so
it properly ends up in patchwork.

Thanks,
Daniel

Re: [PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Stanislav Fomichev]

On 01/16, Daniel Borkmann wrote:
> On 01/16/2019 07:11 PM, Stanislav Fomichev wrote:
> > Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
> >   * 128-bit (uuid)
> >   * 160-bit (sha1)
> >   * any length specified in ld --build-id=0xhexstring
> > 
> > To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> > assume that build-id is somewhere in the range of 1 .. 20.
> > Set the remaining bytes to zero.
> > 
> > v2:
> > * don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
> >   we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
> >   this 'if' condition
> > 
> > Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> 
> Hmm, looks like rest of the v2 series didn't make it to the list.
> Please double check; just in case simply resend the full v2 set so
> it properly ends up in patchwork.
Oh, I didn't send the first two patches because I didn't change them, I'll
resend full v2 series in a moment.
> 
> Thanks,
> Daniel

[PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Stanislav Fomichev]

Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
  * 128-bit (uuid)
  * 160-bit (sha1)
  * any length specified in ld --build-id=0xhexstring

To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
assume that build-id is somewhere in the range of 1 .. 20.
Set the remaining bytes to zero.

v2:
* don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
  we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
  this 'if' condition

Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/stackmap.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index d9e2483669d0..f9df545e92f6 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -180,11 +180,14 @@ static inline int stack_map_parse_build_id(void *page_addr,
 
 		if (nhdr->n_type == BPF_BUILD_ID &&
 		    nhdr->n_namesz == sizeof("GNU") &&
-		    nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
+		    nhdr->n_descsz > 0 &&
+		    nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
 			memcpy(build_id,
 			       note_start + note_offs +
 			       ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
-			       BPF_BUILD_ID_SIZE);
+			       nhdr->n_descsz);
+			memset(build_id + nhdr->n_descsz, 0,
+			       BPF_BUILD_ID_SIZE - nhdr->n_descsz);
 			return 0;
 		}
 		new_offs = note_offs + sizeof(Elf32_Nhdr) +
-- 
2.20.1.97.g81188d93c3-goog

[PATCH bpf v2 2/3] bpf: zero out build_id for BPF_STACK_BUILD_ID_IP

[Stanislav Fomichev]

When returning BPF_STACK_BUILD_ID_IP from stack_map_get_build_id_offset,
make sure that build_id field is empty. Since we are using percpu
free list, there is a possibility that we might reuse some previous
bpf_stack_build_id with non-zero build_id.

Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/stackmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index f9df545e92f6..d43b14535827 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -314,6 +314,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 		for (i = 0; i < trace_nr; i++) {
 			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
 			id_offs[i].ip = ips[i];
+			memset(id_offs[i].build_id, 0, BPF_BUILD_ID_SIZE);
 		}
 		return;
 	}
@@ -324,6 +325,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 			/* per entry fall back to ips */
 			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
 			id_offs[i].ip = ips[i];
+			memset(id_offs[i].build_id, 0, BPF_BUILD_ID_SIZE);
 			continue;
 		}
 		id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ips[i]
-- 
2.20.1.97.g81188d93c3-goog

[PATCH bpf v2 3/3] selftests/bpf: retry tests that expect build-id

[Stanislav Fomichev]

While running test_progs in a loop I found out that I'm sometimes hitting
"Didn't find expected build ID from the map" error.
Looking at stack_map_get_build_id_offset() it seems that it is racy (by
design) and can sometimes return BPF_STACK_BUILD_ID_IP (i.e. can't trylock
current->mm->mmap_sem).

Let's retry this test a single time.

Fixes: 13790d1cc72c ("bpf: add selftest for stackmap with build_id in NMI context")
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/test_progs.c | 30 ++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 126fc624290d..25f0083a9b2e 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1188,7 +1188,9 @@ static void test_stacktrace_build_id(void)
 	int i, j;
 	struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
 	int build_id_matches = 0;
+	int retry = 1;
 
+retry:
 	err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
 	if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
 		goto out;
@@ -1301,6 +1303,19 @@ static void test_stacktrace_build_id(void)
 		previous_key = key;
 	} while (bpf_map_get_next_key(stackmap_fd, &previous_key, &key) == 0);
 
+	/* stack_map_get_build_id_offset() is racy and sometimes can return
+	 * BPF_STACK_BUILD_ID_IP instead of BPF_STACK_BUILD_ID_VALID;
+	 * try it one more time.
+	 */
+	if (build_id_matches < 1 && retry--) {
+		ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
+		close(pmu_fd);
+		bpf_object__close(obj);
+		printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+		       __func__);
+		goto retry;
+	}
+
 	if (CHECK(build_id_matches < 1, "build id match",
 		  "Didn't find expected build ID from the map\n"))
 		goto disable_pmu;
@@ -1341,7 +1356,9 @@ static void test_stacktrace_build_id_nmi(void)
 	int i, j;
 	struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
 	int build_id_matches = 0;
+	int retry = 1;
 
+retry:
 	err = bpf_prog_load(file, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd);
 	if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
 		return;
@@ -1436,6 +1453,19 @@ static void test_stacktrace_build_id_nmi(void)
 		previous_key = key;
 	} while (bpf_map_get_next_key(stackmap_fd, &previous_key, &key) == 0);
 
+	/* stack_map_get_build_id_offset() is racy and sometimes can return
+	 * BPF_STACK_BUILD_ID_IP instead of BPF_STACK_BUILD_ID_VALID;
+	 * try it one more time.
+	 */
+	if (build_id_matches < 1 && retry--) {
+		ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
+		close(pmu_fd);
+		bpf_object__close(obj);
+		printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+		       __func__);
+		goto retry;
+	}
+
 	if (CHECK(build_id_matches < 1, "build id match",
 		  "Didn't find expected build ID from the map\n"))
 		goto disable_pmu;
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Daniel Borkmann]

On 01/16/2019 11:03 PM, Stanislav Fomichev wrote:
> Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
>   * 128-bit (uuid)
>   * 160-bit (sha1)
>   * any length specified in ld --build-id=0xhexstring
> 
> To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> assume that build-id is somewhere in the range of 1 .. 20.
> Set the remaining bytes to zero.
> 
> v2:
> * don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
>   we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
>   this 'if' condition
> 
> Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Series applied, thanks!