Message ID | 20171012173418.4029-2-jakub.kicinski@netronome.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Series | nfp: bpf: support direct packet access | expand |
On 10/12/2017 07:34 PM, Jakub Kicinski wrote: > Use a simplified is_valid_access() callback when verifier > is used for program analysis by non-host JITs. This allows > us to teach the verifier about packet start and packet end > offsets for direct packet access. > > We can extend the callback as needed but for most packet > processing needs there isn't much more the offloads may > require. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Reviewed-by: Simon Horman <simon.horman@netronome.com> > --- > CC: alexei.starovoitov@gmail.com > CC: daniel@iogearbox.net > > kernel/bpf/verifier.c | 43 +++++++++++++++++++++++++++++++++++++------ > 1 file changed, 37 insertions(+), 6 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 2cdbcc4f8f6b..9755279d94cb 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -813,6 +813,36 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off, > return err; > } > > +static bool analyzer_is_valid_access(struct bpf_verifier_env *env, int off, > + struct bpf_insn_access_aux *info) > +{ > + switch (env->prog->type) { > + case BPF_PROG_TYPE_XDP: > + switch (off) { > + case offsetof(struct xdp_buff, data): > + info->reg_type = PTR_TO_PACKET; > + return true; > + case offsetof(struct xdp_buff, data_end): > + info->reg_type = PTR_TO_PACKET_END; > + return true; > + } > + return false; > + case BPF_PROG_TYPE_SCHED_CLS: > + switch (off) { > + case offsetof(struct sk_buff, data): > + info->reg_type = PTR_TO_PACKET; > + return true; > + case offsetof(struct sk_buff, cb) + > + offsetof(struct bpf_skb_data_end, data_end): > + info->reg_type = PTR_TO_PACKET_END; > + return true; > + } > + return false; > + default: > + return false; > + } > +} > + > /* check access to 'struct bpf_context' fields. Supports fixed offsets only */ > static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, int size, > enum bpf_access_type t, enum bpf_reg_type *reg_type) > @@ -821,12 +851,13 @@ static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, > .reg_type = *reg_type, > }; > > - /* for analyzer ctx accesses are already validated and converted */ > - if (env->analyzer_ops) > - return 0; > - > - if (env->prog->aux->ops->is_valid_access && > - env->prog->aux->ops->is_valid_access(off, size, t, &info)) { > + if (env->analyzer_ops) { > + if (analyzer_is_valid_access(env, off, &info)) { > + *reg_type = info.reg_type; Is there some specific issue with the is_valid_access() callbacks that you need to do this (I couldn't parse that out of the commit message)? It would be nice to keep the reg_type setting in one place, meaning the callbacks themselves, so we wouldn't need to maintain this in multiple places. > + return 0; > + } > + } else if (env->prog->aux->ops->is_valid_access && > + env->prog->aux->ops->is_valid_access(off, size, t, &info)) { > /* A non zero info.ctx_field_size indicates that this field is a > * candidate for later verifier transformation to load the whole > * field and then apply a mask when accessed with a narrower >
On Thu, 12 Oct 2017 22:43:10 +0200, Daniel Borkmann wrote: > On 10/12/2017 07:34 PM, Jakub Kicinski wrote: > > Use a simplified is_valid_access() callback when verifier > > is used for program analysis by non-host JITs. This allows > > us to teach the verifier about packet start and packet end > > offsets for direct packet access. > > > > We can extend the callback as needed but for most packet > > processing needs there isn't much more the offloads may > > require. > > > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > Reviewed-by: Simon Horman <simon.horman@netronome.com> > > --- > > CC: alexei.starovoitov@gmail.com > > CC: daniel@iogearbox.net > > > > kernel/bpf/verifier.c | 43 +++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 37 insertions(+), 6 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 2cdbcc4f8f6b..9755279d94cb 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -813,6 +813,36 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off, > > return err; > > } > > > > +static bool analyzer_is_valid_access(struct bpf_verifier_env *env, int off, > > + struct bpf_insn_access_aux *info) > > +{ > > + switch (env->prog->type) { > > + case BPF_PROG_TYPE_XDP: > > + switch (off) { > > + case offsetof(struct xdp_buff, data): > > + info->reg_type = PTR_TO_PACKET; > > + return true; > > + case offsetof(struct xdp_buff, data_end): > > + info->reg_type = PTR_TO_PACKET_END; > > + return true; > > + } > > + return false; > > + case BPF_PROG_TYPE_SCHED_CLS: > > + switch (off) { > > + case offsetof(struct sk_buff, data): > > + info->reg_type = PTR_TO_PACKET; > > + return true; > > + case offsetof(struct sk_buff, cb) + > > + offsetof(struct bpf_skb_data_end, data_end): > > + info->reg_type = PTR_TO_PACKET_END; > > + return true; > > + } > > + return false; > > + default: > > + return false; > > + } > > +} > > + > > /* check access to 'struct bpf_context' fields. Supports fixed offsets only */ > > static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, int size, > > enum bpf_access_type t, enum bpf_reg_type *reg_type) > > @@ -821,12 +851,13 @@ static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, > > .reg_type = *reg_type, > > }; > > > > - /* for analyzer ctx accesses are already validated and converted */ > > - if (env->analyzer_ops) > > - return 0; > > - > > - if (env->prog->aux->ops->is_valid_access && > > - env->prog->aux->ops->is_valid_access(off, size, t, &info)) { > > + if (env->analyzer_ops) { > > + if (analyzer_is_valid_access(env, off, &info)) { > > + *reg_type = info.reg_type; > > Is there some specific issue with the is_valid_access() callbacks that you > need to do this (I couldn't parse that out of the commit message)? Do you mean why not just call is_valid_access()? The offsets are translated, so is_valid_access() will use user space __sk_buff's offsets while we have the kernel's sk_buff offsets here... > It would be nice to keep the reg_type setting in one place, meaning > the callbacks themselves, so we wouldn't need to maintain this in > multiple places. Hm.. I though this was the smallest and simplest change. I could translate the offsets but that seems wobbly. Or try to consolidate the call into the same if () branch? Not sure.. As a bonus info I discovered there is a bug in -net with how things are converted. We allow arithmetic on context pointers but then only look at the insn.off in the converter... I'm working on a fix.
On 10/12/2017 10:56 PM, Jakub Kicinski wrote: > On Thu, 12 Oct 2017 22:43:10 +0200, Daniel Borkmann wrote: [...] >> It would be nice to keep the reg_type setting in one place, meaning >> the callbacks themselves, so we wouldn't need to maintain this in >> multiple places. > > Hm.. I though this was the smallest and simplest change. I could > translate the offsets but that seems wobbly. Or try to consolidate the > call into the same if () branch? Not sure.. Different callbacks for post-verification would be good at min as it would allow to keep all the context access info in one place for a given type at least. > As a bonus info I discovered there is a bug in -net with how things are > converted. We allow arithmetic on context pointers but then only > look at the insn.off in the converter... I'm working on a fix. Ohh well, good catch, indeed! :( Can you also add coverage to the bpf selftests for this? Thanks, Daniel
On Thu, 12 Oct 2017 23:33:21 +0200, Daniel Borkmann wrote: > On 10/12/2017 10:56 PM, Jakub Kicinski wrote: > > On Thu, 12 Oct 2017 22:43:10 +0200, Daniel Borkmann wrote: > [...] > >> It would be nice to keep the reg_type setting in one place, meaning > >> the callbacks themselves, so we wouldn't need to maintain this in > >> multiple places. > > > > Hm.. I though this was the smallest and simplest change. I could > > translate the offsets but that seems wobbly. Or try to consolidate the > > call into the same if () branch? Not sure.. > > Different callbacks for post-verification would be good at min as it > would allow to keep all the context access info in one place for a > given type at least. Sorry to be clear - you're suggesting adding a new callback to struct bpf_verifier_ops, or swapping the struct bpf_verifier_ops for a special post-verification one? > > As a bonus info I discovered there is a bug in -net with how things are > > converted. We allow arithmetic on context pointers but then only > > look at the insn.off in the converter... I'm working on a fix. > > Ohh well, good catch, indeed! :( Can you also add coverage to the > bpf selftests for this? Will do!
On 10/12/2017 11:39 PM, Jakub Kicinski wrote: > On Thu, 12 Oct 2017 23:33:21 +0200, Daniel Borkmann wrote: >> On 10/12/2017 10:56 PM, Jakub Kicinski wrote: >>> On Thu, 12 Oct 2017 22:43:10 +0200, Daniel Borkmann wrote: >> [...] >>>> It would be nice to keep the reg_type setting in one place, meaning >>>> the callbacks themselves, so we wouldn't need to maintain this in >>>> multiple places. >>> >>> Hm.. I though this was the smallest and simplest change. I could >>> translate the offsets but that seems wobbly. Or try to consolidate the >>> call into the same if () branch? Not sure.. >> >> Different callbacks for post-verification would be good at min as it >> would allow to keep all the context access info in one place for a >> given type at least. > > Sorry to be clear - you're suggesting adding a new callback to struct > bpf_verifier_ops, or swapping the struct bpf_verifier_ops for a > special post-verification one? Either way is fine by me. >>> As a bonus info I discovered there is a bug in -net with how things are >>> converted. We allow arithmetic on context pointers but then only >>> look at the insn.off in the converter... I'm working on a fix. >> >> Ohh well, good catch, indeed! :( Can you also add coverage to the >> bpf selftests for this? > > Will do! Thanks, Daniel
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 2cdbcc4f8f6b..9755279d94cb 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -813,6 +813,36 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off, return err; } +static bool analyzer_is_valid_access(struct bpf_verifier_env *env, int off, + struct bpf_insn_access_aux *info) +{ + switch (env->prog->type) { + case BPF_PROG_TYPE_XDP: + switch (off) { + case offsetof(struct xdp_buff, data): + info->reg_type = PTR_TO_PACKET; + return true; + case offsetof(struct xdp_buff, data_end): + info->reg_type = PTR_TO_PACKET_END; + return true; + } + return false; + case BPF_PROG_TYPE_SCHED_CLS: + switch (off) { + case offsetof(struct sk_buff, data): + info->reg_type = PTR_TO_PACKET; + return true; + case offsetof(struct sk_buff, cb) + + offsetof(struct bpf_skb_data_end, data_end): + info->reg_type = PTR_TO_PACKET_END; + return true; + } + return false; + default: + return false; + } +} + /* check access to 'struct bpf_context' fields. Supports fixed offsets only */ static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, int size, enum bpf_access_type t, enum bpf_reg_type *reg_type) @@ -821,12 +851,13 @@ static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, .reg_type = *reg_type, }; - /* for analyzer ctx accesses are already validated and converted */ - if (env->analyzer_ops) - return 0; - - if (env->prog->aux->ops->is_valid_access && - env->prog->aux->ops->is_valid_access(off, size, t, &info)) { + if (env->analyzer_ops) { + if (analyzer_is_valid_access(env, off, &info)) { + *reg_type = info.reg_type; + return 0; + } + } else if (env->prog->aux->ops->is_valid_access && + env->prog->aux->ops->is_valid_access(off, size, t, &info)) { /* A non zero info.ctx_field_size indicates that this field is a * candidate for later verifier transformation to load the whole * field and then apply a mask when accessed with a narrower