| Message ID | 20220923211837.3044723-2-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | enforce W^X for trampoline and dispatcher | expand |
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.
+ 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
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.
> 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.
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.
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); }
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(-)