bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/3] bpf: bpf trampoline improvements
@ 2023-05-15 13:08 Yafang Shao
  2023-05-15 13:08 ` [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure Yafang Shao
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Yafang Shao @ 2023-05-15 13:08 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa
  Cc: bpf, Yafang Shao

When we run fexit bpf programs (e.g. attaching tcp_recvmsg) on our servers
which were running old kernels, some of these servers crashed. Finally we
figured out that it was caused by the same issue resolved by
commit e21aa341785c ("bpf: Fix fexit trampoline."). After we backported
that commit, the crash disappears. However new issues are introduced by
that commit. This patchset fixes them.

PATCH #1: Fix a memory leak found on our server
PATCH #2: Remove bpf trampoline selector and also fix the issue in perf
          caused by the name change in bpf trampoline
PATCH #3: Show target_{obj,btf}_id when link to a bpf trampoline

v1->v2:
- Reuse the common code between __bpf_tramp_image_put_deferred and
  bpf_tramp_image_free (Song)
- Add fixes tag in patch #1 (Song)
- Restore the old bpf trampoline name format (Song)
- Jiri pointed out a issue in perf
- Show btf information in the tracing link info

Yafang Shao (3):
  bpf: Fix memleak due to fentry attach failure
  bpf: Remove bpf trampoline selector
  bpf: Show target_{obj,btf}_id in tracing link info

 include/linux/bpf.h      |  1 -
 kernel/bpf/syscall.c     | 12 ++++++++++--
 kernel/bpf/trampoline.c  | 32 +++++++++++++++++++-------------
 tools/bpf/bpftool/link.c |  4 ++++
 4 files changed, 33 insertions(+), 16 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure
  2023-05-15 13:08 [PATCH bpf-next v2 0/3] bpf: bpf trampoline improvements Yafang Shao
@ 2023-05-15 13:08 ` Yafang Shao
  2023-05-15 15:52   ` Song Liu
  2023-05-15 13:08 ` [PATCH bpf-next v2 2/3] bpf: Remove bpf trampoline selector Yafang Shao
  2023-05-15 13:08 ` [PATCH bpf-next v2 3/3] bpf: Show target_{obj,btf}_id in tracing link info Yafang Shao
  2 siblings, 1 reply; 12+ messages in thread
From: Yafang Shao @ 2023-05-15 13:08 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa
  Cc: bpf, Yafang Shao, Song Liu, Jiri Olsa

If it fails to attach fentry, the allocated bpf trampoline image will be
left in the system. That can be verified by checking /proc/kallsyms.

This meamleak can be verified by a simple bpf program as follows,

  SEC("fentry/trap_init")
  int fentry_run()
  {
      return 0;
  }

It will fail to attach trap_init because this function is freed after
kernel init, and then we can find the trampoline image is left in the
system by checking /proc/kallsyms.
  $ tail /proc/kallsyms
  ffffffffc0613000 t bpf_trampoline_6442453466_1  [bpf]
  ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]

  $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'"
  [2522] FUNC 'trap_init' type_id=119 linkage=static

  $ echo $((6442453466 & 0x7fffffff))
  2522

Note that there are two left bpf trampoline images, that is because the
libbpf will fallback to raw tracepoint if -EINVAL is returned.

Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Song Liu <song@kernel.org>
Cc: Jiri Olsa <olsajiri@gmail.com>
---
 kernel/bpf/trampoline.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index ac021bc..2a3849c 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -251,11 +251,8 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
 	return tlinks;
 }
 
-static void __bpf_tramp_image_put_deferred(struct work_struct *work)
+static void bpf_tramp_image_free(struct bpf_tramp_image *im)
 {
-	struct bpf_tramp_image *im;
-
-	im = container_of(work, struct bpf_tramp_image, work);
 	bpf_image_ksym_del(&im->ksym);
 	bpf_jit_free_exec(im->image);
 	bpf_jit_uncharge_modmem(PAGE_SIZE);
@@ -263,6 +260,14 @@ static void __bpf_tramp_image_put_deferred(struct work_struct *work)
 	kfree_rcu(im, rcu);
 }
 
+static void __bpf_tramp_image_put_deferred(struct work_struct *work)
+{
+	struct bpf_tramp_image *im;
+
+	im = container_of(work, struct bpf_tramp_image, work);
+	bpf_tramp_image_free(im);
+}
+
 /* callback, fexit step 3 or fentry step 2 */
 static void __bpf_tramp_image_put_rcu(struct rcu_head *rcu)
 {
@@ -438,7 +443,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 					  &tr->func.model, tr->flags, tlinks,
 					  tr->func.addr);
 	if (err < 0)
-		goto out;
+		goto out_free;
 
 	set_memory_rox((long)im->image, 1);
 
@@ -468,7 +473,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 	}
 #endif
 	if (err)
-		goto out;
+		goto out_free;
 
 	if (tr->cur_image)
 		bpf_tramp_image_put(tr->cur_image);
@@ -480,6 +485,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 		tr->flags = orig_flags;
 	kfree(tlinks);
 	return err;
+
+out_free:
+	bpf_tramp_image_free(im);
+	goto out;
 }
 
 static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH bpf-next v2 2/3] bpf: Remove bpf trampoline selector
  2023-05-15 13:08 [PATCH bpf-next v2 0/3] bpf: bpf trampoline improvements Yafang Shao
  2023-05-15 13:08 ` [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure Yafang Shao
@ 2023-05-15 13:08 ` Yafang Shao
  2023-05-15 15:53   ` Song Liu
  2023-05-15 13:08 ` [PATCH bpf-next v2 3/3] bpf: Show target_{obj,btf}_id in tracing link info Yafang Shao
  2 siblings, 1 reply; 12+ messages in thread
From: Yafang Shao @ 2023-05-15 13:08 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa
  Cc: bpf, Yafang Shao, Song Liu, Jiri Olsa

After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
is only used to indicate how many times the bpf trampoline image are
updated and been displayed in the trampoline ksym name. After the
trampoline is freed, the selector will start from 0 again. So the
selector is a useless value to the user. We can remove it.
If the user want to check whether the bpf trampoline image has been updated
or not, the user can compare the address. Each time the trampoline image
is updated, the address will change consequently.

Jiri pointed out antoher issue that perf is still using the old name
"bpf_trampoline_%lu", so this change can fix the issue in perf.

Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Song Liu <song@kernel.org>
Cc: Jiri Olsa <olsajiri@gmail.com>
---
 include/linux/bpf.h     |  1 -
 kernel/bpf/trampoline.c | 11 ++++-------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 456f33b..36e4b2d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1125,7 +1125,6 @@ struct bpf_trampoline {
 	int progs_cnt[BPF_TRAMP_MAX];
 	/* Executable image of trampoline */
 	struct bpf_tramp_image *cur_image;
-	u64 selector;
 	struct module *mod;
 };
 
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 2a3849c..78acf28 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -349,7 +349,7 @@ static void bpf_tramp_image_put(struct bpf_tramp_image *im)
 	call_rcu_tasks_trace(&im->rcu, __bpf_tramp_image_put_rcu_tasks);
 }
 
-static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, u32 idx)
+static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key)
 {
 	struct bpf_tramp_image *im;
 	struct bpf_ksym *ksym;
@@ -376,7 +376,7 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, u32 idx)
 
 	ksym = &im->ksym;
 	INIT_LIST_HEAD_RCU(&ksym->lnode);
-	snprintf(ksym->name, KSYM_NAME_LEN, "bpf_trampoline_%llu_%u", key, idx);
+	snprintf(ksym->name, KSYM_NAME_LEN, "bpf_trampoline_%llu", key);
 	bpf_image_ksym_add(image, ksym);
 	return im;
 
@@ -406,11 +406,10 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 		err = unregister_fentry(tr, tr->cur_image->image);
 		bpf_tramp_image_put(tr->cur_image);
 		tr->cur_image = NULL;
-		tr->selector = 0;
 		goto out;
 	}
 
-	im = bpf_tramp_image_alloc(tr->key, tr->selector);
+	im = bpf_tramp_image_alloc(tr->key);
 	if (IS_ERR(im)) {
 		err = PTR_ERR(im);
 		goto out;
@@ -447,8 +446,7 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 
 	set_memory_rox((long)im->image, 1);
 
-	WARN_ON(tr->cur_image && tr->selector == 0);
-	WARN_ON(!tr->cur_image && tr->selector);
+	WARN_ON(tr->cur_image && total == 0);
 	if (tr->cur_image)
 		/* progs already running at this address */
 		err = modify_fentry(tr, tr->cur_image->image, im->image, lock_direct_mutex);
@@ -478,7 +476,6 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 	if (tr->cur_image)
 		bpf_tramp_image_put(tr->cur_image);
 	tr->cur_image = im;
-	tr->selector++;
 out:
 	/* If any error happens, restore previous flags */
 	if (err)
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH bpf-next v2 3/3] bpf: Show target_{obj,btf}_id in tracing link info
  2023-05-15 13:08 [PATCH bpf-next v2 0/3] bpf: bpf trampoline improvements Yafang Shao
  2023-05-15 13:08 ` [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure Yafang Shao
  2023-05-15 13:08 ` [PATCH bpf-next v2 2/3] bpf: Remove bpf trampoline selector Yafang Shao
@ 2023-05-15 13:08 ` Yafang Shao
  2023-05-15 16:12   ` Song Liu
  2 siblings, 1 reply; 12+ messages in thread
From: Yafang Shao @ 2023-05-15 13:08 UTC (permalink / raw)
  To: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa
  Cc: bpf, Yafang Shao

The target_btf_id can help us understand which kernel function is
linked by a tracing prog. The target_btf_id and target_obj_id have
already been exposed to userspace, so we just need to show them.

The result as follows,

tools/bpf/bpftool/bpftool link show
2: tracing  prog 13
        prog_type tracing  attach_type trace_fentry
        target_obj_id 1  target_btf_id 13964
        pids trace(10673)

$ tools/bpf/bpftool/bpftool link show -j
[{"id":2,"type":"tracing","prog_id":13,"prog_type":"tracing","attach_type":"trace_fentry","target_obj_id":1,"target_btf_id":13964,"pids":[{"pid":10673,"comm":"trace"}]}]

$ cat /proc/10673/fdinfo/10
pos:    0
flags:  02000000
mnt_id: 15
ino:    2094
link_type:      tracing
link_id:        2
prog_tag:       a04f5eef06a7f555
prog_id:        13
attach_type:    24
target_obj_id:  1
target_btf_id:  13964

$ tail /proc/kallsyms
ffffffffc0400fa0 t bpf_prog_a04f5eef06a7f555_fentry_run [bpf]
ffffffffc062f000 t bpf_trampoline_6442464908    [bpf]

$ echo $((6442464908 & 0x7fffffff)) $((6442464908 >> 32))
13964 1

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 kernel/bpf/syscall.c     | 12 ++++++++++--
 tools/bpf/bpftool/link.c |  4 ++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 909c112..870395a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2968,10 +2968,18 @@ static void bpf_tracing_link_show_fdinfo(const struct bpf_link *link,
 {
 	struct bpf_tracing_link *tr_link =
 		container_of(link, struct bpf_tracing_link, link.link);
+	u32 target_btf_id;
+	u32 target_obj_id;
 
+	bpf_trampoline_unpack_key(tr_link->trampoline->key,
+							  &target_obj_id, &target_btf_id);
 	seq_printf(seq,
-		   "attach_type:\t%d\n",
-		   tr_link->attach_type);
+		   "attach_type:\t%d\n"
+		   "target_obj_id:\t%u\n"
+		   "target_btf_id:\t%u\n",
+		   tr_link->attach_type,
+		   target_obj_id,
+		   target_btf_id);
 }
 
 static int bpf_tracing_link_fill_link_info(const struct bpf_link *link,
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index 243b74e..cfe896f 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -195,6 +195,8 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
 
 		show_link_attach_type_json(info->tracing.attach_type,
 					   json_wtr);
+		jsonw_uint_field(json_wtr, "target_obj_id", info->tracing.target_obj_id);
+		jsonw_uint_field(json_wtr, "target_btf_id", info->tracing.target_btf_id);
 		break;
 	case BPF_LINK_TYPE_CGROUP:
 		jsonw_lluint_field(json_wtr, "cgroup_id",
@@ -375,6 +377,8 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
 			printf("\n\tprog_type %u  ", prog_info.type);
 
 		show_link_attach_type_plain(info->tracing.attach_type);
+		printf("\n\ttarget_obj_id %u  target_btf_id %u  ",
+			   info->tracing.target_obj_id, info->tracing.target_btf_id);
 		break;
 	case BPF_LINK_TYPE_CGROUP:
 		printf("\n\tcgroup_id %zu  ", (size_t)info->cgroup.cgroup_id);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure
  2023-05-15 13:08 ` [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure Yafang Shao
@ 2023-05-15 15:52   ` Song Liu
  2023-05-15 20:17     ` Daniel Borkmann
  0 siblings, 1 reply; 12+ messages in thread
From: Song Liu @ 2023-05-15 15:52 UTC (permalink / raw)
  To: Yafang Shao
  Cc: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, bpf, Jiri Olsa

On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> If it fails to attach fentry, the allocated bpf trampoline image will be
> left in the system. That can be verified by checking /proc/kallsyms.
>
> This meamleak can be verified by a simple bpf program as follows,
>
>   SEC("fentry/trap_init")
>   int fentry_run()
>   {
>       return 0;
>   }
>
> It will fail to attach trap_init because this function is freed after
> kernel init, and then we can find the trampoline image is left in the
> system by checking /proc/kallsyms.
>   $ tail /proc/kallsyms
>   ffffffffc0613000 t bpf_trampoline_6442453466_1  [bpf]
>   ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]
>
>   $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'"
>   [2522] FUNC 'trap_init' type_id=119 linkage=static
>
>   $ echo $((6442453466 & 0x7fffffff))
>   2522
>
> Note that there are two left bpf trampoline images, that is because the
> libbpf will fallback to raw tracepoint if -EINVAL is returned.
>
> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Song Liu <song@kernel.org>
> Cc: Jiri Olsa <olsajiri@gmail.com>

Acked-by: Song Liu <song@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 2/3] bpf: Remove bpf trampoline selector
  2023-05-15 13:08 ` [PATCH bpf-next v2 2/3] bpf: Remove bpf trampoline selector Yafang Shao
@ 2023-05-15 15:53   ` Song Liu
  2023-05-15 20:42     ` Daniel Borkmann
  0 siblings, 1 reply; 12+ messages in thread
From: Song Liu @ 2023-05-15 15:53 UTC (permalink / raw)
  To: Yafang Shao
  Cc: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, bpf, Jiri Olsa

On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
> is only used to indicate how many times the bpf trampoline image are
> updated and been displayed in the trampoline ksym name. After the
> trampoline is freed, the selector will start from 0 again. So the
> selector is a useless value to the user. We can remove it.
> If the user want to check whether the bpf trampoline image has been updated
> or not, the user can compare the address. Each time the trampoline image
> is updated, the address will change consequently.
>
> Jiri pointed out antoher issue that perf is still using the old name
> "bpf_trampoline_%lu", so this change can fix the issue in perf.
>
> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Song Liu <song@kernel.org>
> Cc: Jiri Olsa <olsajiri@gmail.com>

Acked-by: Song Liu <song@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Show target_{obj,btf}_id in tracing link info
  2023-05-15 13:08 ` [PATCH bpf-next v2 3/3] bpf: Show target_{obj,btf}_id in tracing link info Yafang Shao
@ 2023-05-15 16:12   ` Song Liu
  2023-05-16  2:21     ` Yafang Shao
  0 siblings, 1 reply; 12+ messages in thread
From: Song Liu @ 2023-05-15 16:12 UTC (permalink / raw)
  To: Yafang Shao
  Cc: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, bpf

On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>

[...]

>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

The change looks good to me, except that we should split the change
into two commits.

Also, this doesn't seem to be related to the other two patches. So it is
a little weird to add it in v2.

Other than these.

Acked-by: Song Liu <song@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure
  2023-05-15 15:52   ` Song Liu
@ 2023-05-15 20:17     ` Daniel Borkmann
  2023-05-15 21:14       ` Song Liu
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Borkmann @ 2023-05-15 20:17 UTC (permalink / raw)
  To: Song Liu, Yafang Shao
  Cc: ast, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh,
	sdf, haoluo, jolsa, bpf, Jiri Olsa

On 5/15/23 5:52 PM, Song Liu wrote:
> On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>>
>> If it fails to attach fentry, the allocated bpf trampoline image will be
>> left in the system. That can be verified by checking /proc/kallsyms.
>>
>> This meamleak can be verified by a simple bpf program as follows,
>>
>>    SEC("fentry/trap_init")
>>    int fentry_run()
>>    {
>>        return 0;
>>    }
>>
>> It will fail to attach trap_init because this function is freed after
>> kernel init, and then we can find the trampoline image is left in the
>> system by checking /proc/kallsyms.
>>    $ tail /proc/kallsyms
>>    ffffffffc0613000 t bpf_trampoline_6442453466_1  [bpf]
>>    ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]
>>
>>    $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'"
>>    [2522] FUNC 'trap_init' type_id=119 linkage=static
>>
>>    $ echo $((6442453466 & 0x7fffffff))
>>    2522
>>
>> Note that there are two left bpf trampoline images, that is because the
>> libbpf will fallback to raw tracepoint if -EINVAL is returned.
>>
>> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> Cc: Song Liu <song@kernel.org>
>> Cc: Jiri Olsa <olsajiri@gmail.com>
> 
> Acked-by: Song Liu <song@kernel.org>

Won't this trigger a UAF for the case when progs are already running at
this address aka modify_fentry() fails where you would then also hit the
goto out_free path? This looks not correct to me.

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 2/3] bpf: Remove bpf trampoline selector
  2023-05-15 15:53   ` Song Liu
@ 2023-05-15 20:42     ` Daniel Borkmann
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Borkmann @ 2023-05-15 20:42 UTC (permalink / raw)
  To: Song Liu, Yafang Shao
  Cc: ast, andrii, kafai, songliubraving, yhs, john.fastabend, kpsingh,
	sdf, haoluo, jolsa, bpf, Jiri Olsa

On 5/15/23 5:53 PM, Song Liu wrote:
> On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>>
>> After commit e21aa341785c ("bpf: Fix fexit trampoline."), the selector
>> is only used to indicate how many times the bpf trampoline image are
>> updated and been displayed in the trampoline ksym name. After the
>> trampoline is freed, the selector will start from 0 again. So the
>> selector is a useless value to the user. We can remove it.
>> If the user want to check whether the bpf trampoline image has been updated
>> or not, the user can compare the address. Each time the trampoline image
>> is updated, the address will change consequently.
>>
>> Jiri pointed out antoher issue that perf is still using the old name
>> "bpf_trampoline_%lu", so this change can fix the issue in perf.
>>
>> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> Cc: Song Liu <song@kernel.org>
>> Cc: Jiri Olsa <olsajiri@gmail.com>
> 
> Acked-by: Song Liu <song@kernel.org>

Lgtm, I took this individual one for now.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure
  2023-05-15 20:17     ` Daniel Borkmann
@ 2023-05-15 21:14       ` Song Liu
  2023-05-15 21:49         ` Daniel Borkmann
  0 siblings, 1 reply; 12+ messages in thread
From: Song Liu @ 2023-05-15 21:14 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Song Liu, Yafang Shao, Alexei Starovoitov, Andrii Nakryiko,
	Martin Lau, Yonghong Song, John Fastabend, KP Singh,
	Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, Jiri Olsa



> On May 15, 2023, at 1:17 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
> On 5/15/23 5:52 PM, Song Liu wrote:
>> On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>>> 
>>> If it fails to attach fentry, the allocated bpf trampoline image will be
>>> left in the system. That can be verified by checking /proc/kallsyms.
>>> 
>>> This meamleak can be verified by a simple bpf program as follows,
>>> 
>>>   SEC("fentry/trap_init")
>>>   int fentry_run()
>>>   {
>>>       return 0;
>>>   }
>>> 
>>> It will fail to attach trap_init because this function is freed after
>>> kernel init, and then we can find the trampoline image is left in the
>>> system by checking /proc/kallsyms.
>>>   $ tail /proc/kallsyms
>>>   ffffffffc0613000 t bpf_trampoline_6442453466_1  [bpf]
>>>   ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]
>>> 
>>>   $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'"
>>>   [2522] FUNC 'trap_init' type_id=119 linkage=static
>>> 
>>>   $ echo $((6442453466 & 0x7fffffff))
>>>   2522
>>> 
>>> Note that there are two left bpf trampoline images, that is because the
>>> libbpf will fallback to raw tracepoint if -EINVAL is returned.
>>> 
>>> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>> Cc: Song Liu <song@kernel.org>
>>> Cc: Jiri Olsa <olsajiri@gmail.com>
>> Acked-by: Song Liu <song@kernel.org>
> 
> Won't this trigger a UAF for the case when progs are already running at
> this address aka modify_fentry() fails where you would then also hit the
> goto out_free path? This looks not correct to me.

I am not following. If modify_fentry() fails, we will not use the new 
image anywhere, no? Did I miss something?

Thanks,
Song

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure
  2023-05-15 21:14       ` Song Liu
@ 2023-05-15 21:49         ` Daniel Borkmann
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Borkmann @ 2023-05-15 21:49 UTC (permalink / raw)
  To: Song Liu
  Cc: Song Liu, Yafang Shao, Alexei Starovoitov, Andrii Nakryiko,
	Martin Lau, Yonghong Song, John Fastabend, KP Singh,
	Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, Jiri Olsa

On 5/15/23 11:14 PM, Song Liu wrote:
>> On May 15, 2023, at 1:17 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 5/15/23 5:52 PM, Song Liu wrote:
>>> On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>>>>
>>>> If it fails to attach fentry, the allocated bpf trampoline image will be
>>>> left in the system. That can be verified by checking /proc/kallsyms.
>>>>
>>>> This meamleak can be verified by a simple bpf program as follows,
>>>>
>>>>    SEC("fentry/trap_init")
>>>>    int fentry_run()
>>>>    {
>>>>        return 0;
>>>>    }
>>>>
>>>> It will fail to attach trap_init because this function is freed after
>>>> kernel init, and then we can find the trampoline image is left in the
>>>> system by checking /proc/kallsyms.
>>>>    $ tail /proc/kallsyms
>>>>    ffffffffc0613000 t bpf_trampoline_6442453466_1  [bpf]
>>>>    ffffffffc06c3000 t bpf_trampoline_6442453466_1  [bpf]
>>>>
>>>>    $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep "FUNC 'trap_init'"
>>>>    [2522] FUNC 'trap_init' type_id=119 linkage=static
>>>>
>>>>    $ echo $((6442453466 & 0x7fffffff))
>>>>    2522
>>>>
>>>> Note that there are two left bpf trampoline images, that is because the
>>>> libbpf will fallback to raw tracepoint if -EINVAL is returned.
>>>>
>>>> Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
>>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>>> Cc: Song Liu <song@kernel.org>
>>>> Cc: Jiri Olsa <olsajiri@gmail.com>
>>> Acked-by: Song Liu <song@kernel.org>
>>
>> Won't this trigger a UAF for the case when progs are already running at
>> this address aka modify_fentry() fails where you would then also hit the
>> goto out_free path? This looks not correct to me.
> 
> I am not following. If modify_fentry() fails, we will not use the new
> image anywhere, no? Did I miss something?

Hm, agree, I think I got confused with the again label earlier. Looks ok
indeed. Applied, thanks!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Show target_{obj,btf}_id in tracing link info
  2023-05-15 16:12   ` Song Liu
@ 2023-05-16  2:21     ` Yafang Shao
  0 siblings, 0 replies; 12+ messages in thread
From: Yafang Shao @ 2023-05-16  2:21 UTC (permalink / raw)
  To: Song Liu
  Cc: ast, daniel, andrii, kafai, songliubraving, yhs, john.fastabend,
	kpsingh, sdf, haoluo, jolsa, bpf

On Tue, May 16, 2023 at 12:13 AM Song Liu <song@kernel.org> wrote:
>
> On Mon, May 15, 2023 at 6:09 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
>
> [...]
>
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
> The change looks good to me, except that we should split the change
> into two commits.
>

Will split it and send them. Thanks for your review.

> Also, this doesn't seem to be related to the other two patches. So it is
> a little weird to add it in v2.
>
> Other than these.
>
> Acked-by: Song Liu <song@kernel.org>



-- 
Regards
Yafang

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-05-16  2:21 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-15 13:08 [PATCH bpf-next v2 0/3] bpf: bpf trampoline improvements Yafang Shao
2023-05-15 13:08 ` [PATCH bpf-next v2 1/3] bpf: Fix memleak due to fentry attach failure Yafang Shao
2023-05-15 15:52   ` Song Liu
2023-05-15 20:17     ` Daniel Borkmann
2023-05-15 21:14       ` Song Liu
2023-05-15 21:49         ` Daniel Borkmann
2023-05-15 13:08 ` [PATCH bpf-next v2 2/3] bpf: Remove bpf trampoline selector Yafang Shao
2023-05-15 15:53   ` Song Liu
2023-05-15 20:42     ` Daniel Borkmann
2023-05-15 13:08 ` [PATCH bpf-next v2 3/3] bpf: Show target_{obj,btf}_id in tracing link info Yafang Shao
2023-05-15 16:12   ` Song Liu
2023-05-16  2:21     ` Yafang Shao

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).