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

[Song Liu <songliubraving@fb.com>]

+ 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