On Fri, Sep 23, 2022 at 5:51 PM Song Liu <songliubraving@xxxxxx> wrote:
>
>
>
> > On Sep 23, 2022, at 4:23 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Fri, Sep 23, 2022 at 4:18 PM Song Liu <songliubraving@xxxxxx> wrote:
> >>
> >> + Björn Töpel
> >>
> >>> On Sep 23, 2022, at 3:00 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> >>>
> >>> On Fri, Sep 23, 2022 at 2:18 PM Song Liu <song@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>> ---
> >>>> 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.
