[PATCH bpf-next 0/2] enforce W^X for trampoline and dispatcher

[PATCH bpf-next 0/2] enforce W^X for trampoline and dispatcher

[Song Liu]

Jeff Layton reported CPA W^X warning linux-next [1]. It turns out to be
W^X issue with bpf trampoline and bpf dispatcher. Fix these by:

1. Use bpf_prog_pack for bpf_dispatcher;
2. Set memory permission properly with bpf trampoline.

[1] https://lore.kernel.org/lkml/c84cc27c1a5031a003039748c3c099732a718aec.camel@kernel.org/

Song Liu (2):
  bpf: use bpf_prog_pack for bpf_dispatcher
  bpf: Enforce W^X for bpf trampoline

 include/linux/bpf.h     |  2 +-
 include/linux/filter.h  |  5 +++++
 kernel/bpf/core.c       |  9 +++++++--
 kernel/bpf/dispatcher.c | 21 ++++++++++++++++++---
 kernel/bpf/trampoline.c | 22 +++++-----------------
 5 files changed, 36 insertions(+), 23 deletions(-)

--
2.30.2

[PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher

[Song Liu]

Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher
can share pages with bpf programs.

This also fixes CPA W^X warnning like:

CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ...

Signed-off-by: Song Liu <song@kernel.org>
---
 include/linux/bpf.h     |  1 +
 include/linux/filter.h  |  5 +++++
 kernel/bpf/core.c       |  9 +++++++--
 kernel/bpf/dispatcher.c | 21 ++++++++++++++++++---
 4 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index edd43edb27d6..a8d0cfe14372 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -946,6 +946,7 @@ struct bpf_dispatcher {
 	struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX];
 	int num_progs;
 	void *image;
+	void *rw_image;
 	u32 image_off;
 	struct bpf_ksym ksym;
 };
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 98e28126c24b..efc42a6e3aed 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max;
 
 typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
 
+void bpf_jit_fill_hole_with_zero(void *area, unsigned int size);
+
 struct bpf_binary_header *
 bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
 		     unsigned int alignment,
@@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp);
 struct bpf_binary_header *
 bpf_jit_binary_pack_hdr(const struct bpf_prog *fp);
 
+void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns);
+void bpf_prog_pack_free(struct bpf_binary_header *hdr);
+
 static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
 {
 	return list_empty(&fp->aux->ksym.lnode) ||
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index d1be78c28619..711fd293b6de 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -825,6 +825,11 @@ struct bpf_prog_pack {
 	unsigned long bitmap[];
 };
 
+void bpf_jit_fill_hole_with_zero(void *area, unsigned int size)
+{
+	memset(area, 0, size);
+}
+
 #define BPF_PROG_SIZE_TO_NBITS(size)	(round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE)
 
 static DEFINE_MUTEX(pack_mutex);
@@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
 	return pack;
 }
 
-static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
+void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
 {
 	unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size);
 	struct bpf_prog_pack *pack;
@@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn
 	return ptr;
 }
 
-static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
+void bpf_prog_pack_free(struct bpf_binary_header *hdr)
 {
 	struct bpf_prog_pack *pack = NULL, *tmp;
 	unsigned int nbits;
diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
index 2444bd15cc2d..8a10300854b6 100644
--- a/kernel/bpf/dispatcher.c
+++ b/kernel/bpf/dispatcher.c
@@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image)
 
 static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
 {
-	void *old, *new;
+	void *old, *new, *tmp;
 	u32 noff;
 	int err;
 
@@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
 	}
 
 	new = d->num_progs ? d->image + noff : NULL;
+	tmp = d->num_progs ? d->rw_image + noff : NULL;
 	if (new) {
-		if (bpf_dispatcher_prepare(d, new))
+		/* Prepare the dispatcher in d->rw_image. Then use
+		 * bpf_arch_text_copy to update d->image, which is RO+X.
+		 */
+		if (bpf_dispatcher_prepare(d, tmp))
+			return;
+		if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2)))
 			return;
 	}
 
@@ -140,9 +146,18 @@ void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
 
 	mutex_lock(&d->mutex);
 	if (!d->image) {
-		d->image = bpf_jit_alloc_exec_page();
+		d->image = bpf_prog_pack_alloc(PAGE_SIZE, bpf_jit_fill_hole_with_zero);
 		if (!d->image)
 			goto out;
+		d->rw_image = bpf_jit_alloc_exec(PAGE_SIZE);
+		if (!d->rw_image) {
+			u32 size = PAGE_SIZE;
+
+			bpf_arch_text_copy(d->image, &size, sizeof(size));
+			bpf_prog_pack_free((struct bpf_binary_header *)d->image);
+			d->image = NULL;
+			goto out;
+		}
 		bpf_image_ksym_add(d->image, &d->ksym);
 	}
 
-- 
2.30.2

[PATCH bpf-next 2/2] bpf: Enforce W^X for bpf trampoline

[Song Liu]

Mark the trampoline as RO+X after arch_prepare_bpf_trampoline, so that
the trampoine follows W^X rule strictly. This will turn off warnings like

CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ...

Also remove bpf_jit_alloc_exec_page(), since it is not used any more.

Signed-off-by: Song Liu <song@kernel.org>
---
 include/linux/bpf.h     |  1 -
 kernel/bpf/trampoline.c | 22 +++++-----------------
 2 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a8d0cfe14372..44a3d6b3c924 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1008,7 +1008,6 @@ int arch_prepare_bpf_dispatcher(void *image, s64 *funcs, int num_funcs);
 void bpf_dispatcher_change_prog(struct bpf_dispatcher *d, struct bpf_prog *from,
 				struct bpf_prog *to);
 /* Called only from JIT-enabled code, so there's no need for stubs. */
-void *bpf_jit_alloc_exec_page(void);
 void bpf_image_ksym_add(void *data, struct bpf_ksym *ksym);
 void bpf_image_ksym_del(struct bpf_ksym *ksym);
 void bpf_ksym_add(struct bpf_ksym *ksym);
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 41b67eb83ab3..6f7b939321d6 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -116,22 +116,6 @@ bool bpf_prog_has_trampoline(const struct bpf_prog *prog)
 		(ptype == BPF_PROG_TYPE_LSM && eatype == BPF_LSM_MAC);
 }
 
-void *bpf_jit_alloc_exec_page(void)
-{
-	void *image;
-
-	image = bpf_jit_alloc_exec(PAGE_SIZE);
-	if (!image)
-		return NULL;
-
-	set_vm_flush_reset_perms(image);
-	/* Keep image as writeable. The alternative is to keep flipping ro/rw
-	 * every time new program is attached or detached.
-	 */
-	set_memory_x((long)image, 1);
-	return image;
-}
-
 void bpf_image_ksym_add(void *data, struct bpf_ksym *ksym)
 {
 	ksym->start = (unsigned long) data;
@@ -404,9 +388,10 @@ static struct bpf_tramp_image *bpf_tramp_image_alloc(u64 key, u32 idx)
 		goto out_free_im;
 
 	err = -ENOMEM;
-	im->image = image = bpf_jit_alloc_exec_page();
+	im->image = image = bpf_jit_alloc_exec(PAGE_SIZE);
 	if (!image)
 		goto out_uncharge;
+	set_vm_flush_reset_perms(image);
 
 	err = percpu_ref_init(&im->pcref, __bpf_tramp_image_release, 0, GFP_KERNEL);
 	if (err)
@@ -483,6 +468,9 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
 	if (err < 0)
 		goto out;
 
+	set_memory_ro((long)im->image, 1);
+	set_memory_x((long)im->image, 1);
+
 	WARN_ON(tr->cur_image && tr->selector == 0);
 	WARN_ON(!tr->cur_image && tr->selector);
 	if (tr->cur_image)
-- 
2.30.2

Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher

[Alexei Starovoitov]

On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote:
>
> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher
> can share pages with bpf programs.
>
> This also fixes CPA W^X warnning like:
>
> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ...
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  include/linux/bpf.h     |  1 +
>  include/linux/filter.h  |  5 +++++
>  kernel/bpf/core.c       |  9 +++++++--
>  kernel/bpf/dispatcher.c | 21 ++++++++++++++++++---
>  4 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index edd43edb27d6..a8d0cfe14372 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -946,6 +946,7 @@ struct bpf_dispatcher {
>         struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX];
>         int num_progs;
>         void *image;
> +       void *rw_image;
>         u32 image_off;
>         struct bpf_ksym ksym;
>  };
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 98e28126c24b..efc42a6e3aed 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max;
>
>  typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
>
> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size);
> +
>  struct bpf_binary_header *
>  bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
>                      unsigned int alignment,
> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp);
>  struct bpf_binary_header *
>  bpf_jit_binary_pack_hdr(const struct bpf_prog *fp);
>
> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns);
> +void bpf_prog_pack_free(struct bpf_binary_header *hdr);
> +
>  static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
>  {
>         return list_empty(&fp->aux->ksym.lnode) ||
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index d1be78c28619..711fd293b6de 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -825,6 +825,11 @@ struct bpf_prog_pack {
>         unsigned long bitmap[];
>  };
>
> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size)
> +{
> +       memset(area, 0, size);
> +}
> +
>  #define BPF_PROG_SIZE_TO_NBITS(size)   (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE)
>
>  static DEFINE_MUTEX(pack_mutex);
> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
>         return pack;
>  }
>
> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
>  {
>         unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size);
>         struct bpf_prog_pack *pack;
> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn
>         return ptr;
>  }
>
> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
> +void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>  {
>         struct bpf_prog_pack *pack = NULL, *tmp;
>         unsigned int nbits;
> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
> index 2444bd15cc2d..8a10300854b6 100644
> --- a/kernel/bpf/dispatcher.c
> +++ b/kernel/bpf/dispatcher.c
> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image)
>
>  static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
>  {
> -       void *old, *new;
> +       void *old, *new, *tmp;
>         u32 noff;
>         int err;
>
> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
>         }
>
>         new = d->num_progs ? d->image + noff : NULL;
> +       tmp = d->num_progs ? d->rw_image + noff : NULL;
>         if (new) {
> -               if (bpf_dispatcher_prepare(d, new))
> +               /* Prepare the dispatcher in d->rw_image. Then use
> +                * bpf_arch_text_copy to update d->image, which is RO+X.
> +                */
> +               if (bpf_dispatcher_prepare(d, tmp))
> +                       return;
> +               if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2)))

I don't think we can create a dispatcher with one ip
and then copy over into a different location.
See emit_bpf_dispatcher() -> emit_cond_near_jump()
It's a relative offset jump.

Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher

[Song Liu]

+ Björn Töpel 

> On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote:
>> 
>> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher
>> can share pages with bpf programs.
>> 
>> This also fixes CPA W^X warnning like:
>> 
>> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ...
>> 
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> include/linux/bpf.h     |  1 +
>> include/linux/filter.h  |  5 +++++
>> kernel/bpf/core.c       |  9 +++++++--
>> kernel/bpf/dispatcher.c | 21 ++++++++++++++++++---
>> 4 files changed, 31 insertions(+), 5 deletions(-)
>> 
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index edd43edb27d6..a8d0cfe14372 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -946,6 +946,7 @@ struct bpf_dispatcher {
>>        struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX];
>>        int num_progs;
>>        void *image;
>> +       void *rw_image;
>>        u32 image_off;
>>        struct bpf_ksym ksym;
>> };
>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>> index 98e28126c24b..efc42a6e3aed 100644
>> --- a/include/linux/filter.h
>> +++ b/include/linux/filter.h
>> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max;
>> 
>> typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
>> 
>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size);
>> +
>> struct bpf_binary_header *
>> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
>>                     unsigned int alignment,
>> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp);
>> struct bpf_binary_header *
>> bpf_jit_binary_pack_hdr(const struct bpf_prog *fp);
>> 
>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns);
>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr);
>> +
>> static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
>> {
>>        return list_empty(&fp->aux->ksym.lnode) ||
>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>> index d1be78c28619..711fd293b6de 100644
>> --- a/kernel/bpf/core.c
>> +++ b/kernel/bpf/core.c
>> @@ -825,6 +825,11 @@ struct bpf_prog_pack {
>>        unsigned long bitmap[];
>> };
>> 
>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size)
>> +{
>> +       memset(area, 0, size);
>> +}
>> +
>> #define BPF_PROG_SIZE_TO_NBITS(size)   (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE)
>> 
>> static DEFINE_MUTEX(pack_mutex);
>> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
>>        return pack;
>> }
>> 
>> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
>> {
>>        unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size);
>>        struct bpf_prog_pack *pack;
>> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn
>>        return ptr;
>> }
>> 
>> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>> {
>>        struct bpf_prog_pack *pack = NULL, *tmp;
>>        unsigned int nbits;
>> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
>> index 2444bd15cc2d..8a10300854b6 100644
>> --- a/kernel/bpf/dispatcher.c
>> +++ b/kernel/bpf/dispatcher.c
>> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image)
>> 
>> static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
>> {
>> -       void *old, *new;
>> +       void *old, *new, *tmp;
>>        u32 noff;
>>        int err;
>> 
>> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
>>        }
>> 
>>        new = d->num_progs ? d->image + noff : NULL;
>> +       tmp = d->num_progs ? d->rw_image + noff : NULL;
>>        if (new) {
>> -               if (bpf_dispatcher_prepare(d, new))
>> +               /* Prepare the dispatcher in d->rw_image. Then use
>> +                * bpf_arch_text_copy to update d->image, which is RO+X.
>> +                */
>> +               if (bpf_dispatcher_prepare(d, tmp))
>> +                       return;
>> +               if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2)))
> 
> I don't think we can create a dispatcher with one ip
> and then copy over into a different location.
> See emit_bpf_dispatcher() -> emit_cond_near_jump()
> It's a relative offset jump.

Hmm... Yeah, this makes sense. But somehow vmtest doesn't 
show any issue with this. Is there a better way to test this?

Thanks,
Song

Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher

[Alexei Starovoitov]

On Fri, Sep 23, 2022 at 4:18 PM Song Liu <songliubraving@fb.com> wrote:
>
> + Björn Töpel
>
> > On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote:
> >>
> >> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher
> >> can share pages with bpf programs.
> >>
> >> This also fixes CPA W^X warnning like:
> >>
> >> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ...
> >>
> >> Signed-off-by: Song Liu <song@kernel.org>
> >> ---
> >> include/linux/bpf.h     |  1 +
> >> include/linux/filter.h  |  5 +++++
> >> kernel/bpf/core.c       |  9 +++++++--
> >> kernel/bpf/dispatcher.c | 21 ++++++++++++++++++---
> >> 4 files changed, 31 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >> index edd43edb27d6..a8d0cfe14372 100644
> >> --- a/include/linux/bpf.h
> >> +++ b/include/linux/bpf.h
> >> @@ -946,6 +946,7 @@ struct bpf_dispatcher {
> >>        struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX];
> >>        int num_progs;
> >>        void *image;
> >> +       void *rw_image;
> >>        u32 image_off;
> >>        struct bpf_ksym ksym;
> >> };
> >> diff --git a/include/linux/filter.h b/include/linux/filter.h
> >> index 98e28126c24b..efc42a6e3aed 100644
> >> --- a/include/linux/filter.h
> >> +++ b/include/linux/filter.h
> >> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max;
> >>
> >> typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
> >>
> >> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size);
> >> +
> >> struct bpf_binary_header *
> >> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
> >>                     unsigned int alignment,
> >> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp);
> >> struct bpf_binary_header *
> >> bpf_jit_binary_pack_hdr(const struct bpf_prog *fp);
> >>
> >> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns);
> >> +void bpf_prog_pack_free(struct bpf_binary_header *hdr);
> >> +
> >> static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
> >> {
> >>        return list_empty(&fp->aux->ksym.lnode) ||
> >> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> >> index d1be78c28619..711fd293b6de 100644
> >> --- a/kernel/bpf/core.c
> >> +++ b/kernel/bpf/core.c
> >> @@ -825,6 +825,11 @@ struct bpf_prog_pack {
> >>        unsigned long bitmap[];
> >> };
> >>
> >> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size)
> >> +{
> >> +       memset(area, 0, size);
> >> +}
> >> +
> >> #define BPF_PROG_SIZE_TO_NBITS(size)   (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE)
> >>
> >> static DEFINE_MUTEX(pack_mutex);
> >> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
> >>        return pack;
> >> }
> >>
> >> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
> >> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
> >> {
> >>        unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size);
> >>        struct bpf_prog_pack *pack;
> >> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn
> >>        return ptr;
> >> }
> >>
> >> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
> >> +void bpf_prog_pack_free(struct bpf_binary_header *hdr)
> >> {
> >>        struct bpf_prog_pack *pack = NULL, *tmp;
> >>        unsigned int nbits;
> >> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
> >> index 2444bd15cc2d..8a10300854b6 100644
> >> --- a/kernel/bpf/dispatcher.c
> >> +++ b/kernel/bpf/dispatcher.c
> >> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image)
> >>
> >> static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
> >> {
> >> -       void *old, *new;
> >> +       void *old, *new, *tmp;
> >>        u32 noff;
> >>        int err;
> >>
> >> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
> >>        }
> >>
> >>        new = d->num_progs ? d->image + noff : NULL;
> >> +       tmp = d->num_progs ? d->rw_image + noff : NULL;
> >>        if (new) {
> >> -               if (bpf_dispatcher_prepare(d, new))
> >> +               /* Prepare the dispatcher in d->rw_image. Then use
> >> +                * bpf_arch_text_copy to update d->image, which is RO+X.
> >> +                */
> >> +               if (bpf_dispatcher_prepare(d, tmp))
> >> +                       return;
> >> +               if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2)))
> >
> > I don't think we can create a dispatcher with one ip
> > and then copy over into a different location.
> > See emit_bpf_dispatcher() -> emit_cond_near_jump()
> > It's a relative offset jump.
>
> Hmm... Yeah, this makes sense. But somehow vmtest doesn't
> show any issue with this. Is there a better way to test this?

test_xdp*.sh should surely trigger it,
but I'm surprised the regular test_run doesn't trigger it.
We call bpf_prog_run_xdp() there.
We've added
        if (repeat > 1)
                bpf_prog_change_xdp(NULL, prog);

there to reduce test_progs time. Maybe it reduced test coverage too much.

Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher

[Song Liu]

> On Sep 23, 2022, at 4:23 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Fri, Sep 23, 2022 at 4:18 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> + Björn Töpel
>> 
>>> On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>> 
>>> On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote:
>>>> 
>>>> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher
>>>> can share pages with bpf programs.
>>>> 
>>>> This also fixes CPA W^X warnning like:
>>>> 
>>>> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ...
>>>> 
>>>> Signed-off-by: Song Liu <song@kernel.org>
>>>> ---
>>>> include/linux/bpf.h     |  1 +
>>>> include/linux/filter.h  |  5 +++++
>>>> kernel/bpf/core.c       |  9 +++++++--
>>>> kernel/bpf/dispatcher.c | 21 ++++++++++++++++++---
>>>> 4 files changed, 31 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>> index edd43edb27d6..a8d0cfe14372 100644
>>>> --- a/include/linux/bpf.h
>>>> +++ b/include/linux/bpf.h
>>>> @@ -946,6 +946,7 @@ struct bpf_dispatcher {
>>>>       struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX];
>>>>       int num_progs;
>>>>       void *image;
>>>> +       void *rw_image;
>>>>       u32 image_off;
>>>>       struct bpf_ksym ksym;
>>>> };
>>>> diff --git a/include/linux/filter.h b/include/linux/filter.h
>>>> index 98e28126c24b..efc42a6e3aed 100644
>>>> --- a/include/linux/filter.h
>>>> +++ b/include/linux/filter.h
>>>> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max;
>>>> 
>>>> typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
>>>> 
>>>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size);
>>>> +
>>>> struct bpf_binary_header *
>>>> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
>>>>                    unsigned int alignment,
>>>> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp);
>>>> struct bpf_binary_header *
>>>> bpf_jit_binary_pack_hdr(const struct bpf_prog *fp);
>>>> 
>>>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns);
>>>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr);
>>>> +
>>>> static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
>>>> {
>>>>       return list_empty(&fp->aux->ksym.lnode) ||
>>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>>>> index d1be78c28619..711fd293b6de 100644
>>>> --- a/kernel/bpf/core.c
>>>> +++ b/kernel/bpf/core.c
>>>> @@ -825,6 +825,11 @@ struct bpf_prog_pack {
>>>>       unsigned long bitmap[];
>>>> };
>>>> 
>>>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size)
>>>> +{
>>>> +       memset(area, 0, size);
>>>> +}
>>>> +
>>>> #define BPF_PROG_SIZE_TO_NBITS(size)   (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE)
>>>> 
>>>> static DEFINE_MUTEX(pack_mutex);
>>>> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
>>>>       return pack;
>>>> }
>>>> 
>>>> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
>>>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
>>>> {
>>>>       unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size);
>>>>       struct bpf_prog_pack *pack;
>>>> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn
>>>>       return ptr;
>>>> }
>>>> 
>>>> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>>>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr)
>>>> {
>>>>       struct bpf_prog_pack *pack = NULL, *tmp;
>>>>       unsigned int nbits;
>>>> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
>>>> index 2444bd15cc2d..8a10300854b6 100644
>>>> --- a/kernel/bpf/dispatcher.c
>>>> +++ b/kernel/bpf/dispatcher.c
>>>> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image)
>>>> 
>>>> static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
>>>> {
>>>> -       void *old, *new;
>>>> +       void *old, *new, *tmp;
>>>>       u32 noff;
>>>>       int err;
>>>> 
>>>> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
>>>>       }
>>>> 
>>>>       new = d->num_progs ? d->image + noff : NULL;
>>>> +       tmp = d->num_progs ? d->rw_image + noff : NULL;
>>>>       if (new) {
>>>> -               if (bpf_dispatcher_prepare(d, new))
>>>> +               /* Prepare the dispatcher in d->rw_image. Then use
>>>> +                * bpf_arch_text_copy to update d->image, which is RO+X.
>>>> +                */
>>>> +               if (bpf_dispatcher_prepare(d, tmp))
>>>> +                       return;
>>>> +               if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2)))
>>> 
>>> I don't think we can create a dispatcher with one ip
>>> and then copy over into a different location.
>>> See emit_bpf_dispatcher() -> emit_cond_near_jump()
>>> It's a relative offset jump.
>> 
>> Hmm... Yeah, this makes sense. But somehow vmtest doesn't
>> show any issue with this. Is there a better way to test this?
> 
> test_xdp*.sh should surely trigger it,

text_xdp*.sh seem to give same result w/ and w/o the set (on top
of bpf-next). For example, ./test_xdp_redirect.sh works just fine. 
(And I think it shouldn't.)


> but I'm surprised the regular test_run doesn't trigger it.
> We call bpf_prog_run_xdp() there.
> We've added
>        if (repeat > 1)
>                bpf_prog_change_xdp(NULL, prog);

I removed this from test_run.c, but that didn't change vmtest. 

Song


> 
> there to reduce test_progs time. Maybe it reduced test coverage too much.

Re: [PATCH bpf-next 1/2] bpf: use bpf_prog_pack for bpf_dispatcher

[Alexei Starovoitov]

On Fri, Sep 23, 2022 at 5:51 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 23, 2022, at 4:23 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Sep 23, 2022 at 4:18 PM Song Liu <songliubraving@fb.com> wrote:
> >>
> >> + Björn Töpel
> >>
> >>> On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >>>
> >>> On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@kernel.org> wrote:
> >>>>
> >>>> Allocate bpf_dispatcher with bpf_prog_pack_alloc so that bpf_dispatcher
> >>>> can share pages with bpf programs.
> >>>>
> >>>> This also fixes CPA W^X warnning like:
> >>>>
> >>>> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: ...
> >>>>
> >>>> Signed-off-by: Song Liu <song@kernel.org>
> >>>> ---
> >>>> include/linux/bpf.h     |  1 +
> >>>> include/linux/filter.h  |  5 +++++
> >>>> kernel/bpf/core.c       |  9 +++++++--
> >>>> kernel/bpf/dispatcher.c | 21 ++++++++++++++++++---
> >>>> 4 files changed, 31 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >>>> index edd43edb27d6..a8d0cfe14372 100644
> >>>> --- a/include/linux/bpf.h
> >>>> +++ b/include/linux/bpf.h
> >>>> @@ -946,6 +946,7 @@ struct bpf_dispatcher {
> >>>>       struct bpf_dispatcher_prog progs[BPF_DISPATCHER_MAX];
> >>>>       int num_progs;
> >>>>       void *image;
> >>>> +       void *rw_image;
> >>>>       u32 image_off;
> >>>>       struct bpf_ksym ksym;
> >>>> };
> >>>> diff --git a/include/linux/filter.h b/include/linux/filter.h
> >>>> index 98e28126c24b..efc42a6e3aed 100644
> >>>> --- a/include/linux/filter.h
> >>>> +++ b/include/linux/filter.h
> >>>> @@ -1023,6 +1023,8 @@ extern long bpf_jit_limit_max;
> >>>>
> >>>> typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
> >>>>
> >>>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size);
> >>>> +
> >>>> struct bpf_binary_header *
> >>>> bpf_jit_binary_alloc(unsigned int proglen, u8 **image_ptr,
> >>>>                    unsigned int alignment,
> >>>> @@ -1035,6 +1037,9 @@ void bpf_jit_free(struct bpf_prog *fp);
> >>>> struct bpf_binary_header *
> >>>> bpf_jit_binary_pack_hdr(const struct bpf_prog *fp);
> >>>>
> >>>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns);
> >>>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr);
> >>>> +
> >>>> static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp)
> >>>> {
> >>>>       return list_empty(&fp->aux->ksym.lnode) ||
> >>>> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> >>>> index d1be78c28619..711fd293b6de 100644
> >>>> --- a/kernel/bpf/core.c
> >>>> +++ b/kernel/bpf/core.c
> >>>> @@ -825,6 +825,11 @@ struct bpf_prog_pack {
> >>>>       unsigned long bitmap[];
> >>>> };
> >>>>
> >>>> +void bpf_jit_fill_hole_with_zero(void *area, unsigned int size)
> >>>> +{
> >>>> +       memset(area, 0, size);
> >>>> +}
> >>>> +
> >>>> #define BPF_PROG_SIZE_TO_NBITS(size)   (round_up(size, BPF_PROG_CHUNK_SIZE) / BPF_PROG_CHUNK_SIZE)
> >>>>
> >>>> static DEFINE_MUTEX(pack_mutex);
> >>>> @@ -864,7 +869,7 @@ static struct bpf_prog_pack *alloc_new_pack(bpf_jit_fill_hole_t bpf_fill_ill_ins
> >>>>       return pack;
> >>>> }
> >>>>
> >>>> -static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
> >>>> +void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns)
> >>>> {
> >>>>       unsigned int nbits = BPF_PROG_SIZE_TO_NBITS(size);
> >>>>       struct bpf_prog_pack *pack;
> >>>> @@ -905,7 +910,7 @@ static void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insn
> >>>>       return ptr;
> >>>> }
> >>>>
> >>>> -static void bpf_prog_pack_free(struct bpf_binary_header *hdr)
> >>>> +void bpf_prog_pack_free(struct bpf_binary_header *hdr)
> >>>> {
> >>>>       struct bpf_prog_pack *pack = NULL, *tmp;
> >>>>       unsigned int nbits;
> >>>> diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c
> >>>> index 2444bd15cc2d..8a10300854b6 100644
> >>>> --- a/kernel/bpf/dispatcher.c
> >>>> +++ b/kernel/bpf/dispatcher.c
> >>>> @@ -104,7 +104,7 @@ static int bpf_dispatcher_prepare(struct bpf_dispatcher *d, void *image)
> >>>>
> >>>> static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
> >>>> {
> >>>> -       void *old, *new;
> >>>> +       void *old, *new, *tmp;
> >>>>       u32 noff;
> >>>>       int err;
> >>>>
> >>>> @@ -117,8 +117,14 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
> >>>>       }
> >>>>
> >>>>       new = d->num_progs ? d->image + noff : NULL;
> >>>> +       tmp = d->num_progs ? d->rw_image + noff : NULL;
> >>>>       if (new) {
> >>>> -               if (bpf_dispatcher_prepare(d, new))
> >>>> +               /* Prepare the dispatcher in d->rw_image. Then use
> >>>> +                * bpf_arch_text_copy to update d->image, which is RO+X.
> >>>> +                */
> >>>> +               if (bpf_dispatcher_prepare(d, tmp))
> >>>> +                       return;
> >>>> +               if (IS_ERR(bpf_arch_text_copy(new, tmp, PAGE_SIZE / 2)))
> >>>
> >>> I don't think we can create a dispatcher with one ip
> >>> and then copy over into a different location.
> >>> See emit_bpf_dispatcher() -> emit_cond_near_jump()
> >>> It's a relative offset jump.
> >>
> >> Hmm... Yeah, this makes sense. But somehow vmtest doesn't
> >> show any issue with this. Is there a better way to test this?
> >
> > test_xdp*.sh should surely trigger it,
>
> text_xdp*.sh seem to give same result w/ and w/o the set (on top
> of bpf-next). For example, ./test_xdp_redirect.sh works just fine.
> (And I think it shouldn't.)
>
>
> > but I'm surprised the regular test_run doesn't trigger it.
> > We call bpf_prog_run_xdp() there.
> > We've added
> >        if (repeat > 1)
> >                bpf_prog_change_xdp(NULL, prog);
>
> I removed this from test_run.c, but that didn't change vmtest.

Something is broken. That relative jump isn't being triggered.