[net-next,01/12] bpf: verifier: set reg_type on context accesses in second pass

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(-)

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