diff mbox series

[1/5] bpf: Allow non struct type for btf ctx access

Message ID 20191229143740.29143-2-jolsa@kernel.org
State RFC
Delegated to: BPF Maintainers
Headers show
Series bpf: Add trampoline helpers | expand

Commit Message

Jiri Olsa Dec. 29, 2019, 2:37 p.m. UTC
I'm not sure why the restriction was added,
but I can't access pointers to POD types like
const char * when probing vfs_read function.

Removing the check and allow non struct type
access in context.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/btf.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Yonghong Song Jan. 6, 2020, 9:36 p.m. UTC | #1
On 12/29/19 6:37 AM, Jiri Olsa wrote:
> I'm not sure why the restriction was added,
> but I can't access pointers to POD types like
> const char * when probing vfs_read function.
> 
> Removing the check and allow non struct type
> access in context.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   kernel/bpf/btf.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index ed2075884724..ae90f60ac1b8 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3712,12 +3712,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>   	/* skip modifiers */
>   	while (btf_type_is_modifier(t))
>   		t = btf_type_by_id(btf, t->type);
> -	if (!btf_type_is_struct(t)) {
> -		bpf_log(log,
> -			"func '%s' arg%d type %s is not a struct\n",
> -			tname, arg, btf_kind_str[BTF_INFO_KIND(t->info)]);
> -		return false;
> -	}

Hi, Jiri, the RFC looks great! Especially, you also referenced this will
give great performance boost for bcc scripts.

Could you provide more context on why the above change is needed?
The function btf_ctx_access is used to check validity of accessing
function parameters which are wrapped inside a structure, I am wondering
what kinds of accesses you tried to address here.

>   	bpf_log(log, "func '%s' arg%d has btf_id %d type %s '%s'\n",
>   		tname, arg, info->btf_id, btf_kind_str[BTF_INFO_KIND(t->info)],
>   		__btf_name_by_offset(btf, t->name_off));
>
Jiri Olsa Jan. 7, 2020, 12:13 p.m. UTC | #2
On Mon, Jan 06, 2020 at 09:36:17PM +0000, Yonghong Song wrote:
> 
> 
> On 12/29/19 6:37 AM, Jiri Olsa wrote:
> > I'm not sure why the restriction was added,
> > but I can't access pointers to POD types like
> > const char * when probing vfs_read function.
> > 
> > Removing the check and allow non struct type
> > access in context.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   kernel/bpf/btf.c | 6 ------
> >   1 file changed, 6 deletions(-)
> > 
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index ed2075884724..ae90f60ac1b8 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -3712,12 +3712,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >   	/* skip modifiers */
> >   	while (btf_type_is_modifier(t))
> >   		t = btf_type_by_id(btf, t->type);
> > -	if (!btf_type_is_struct(t)) {
> > -		bpf_log(log,
> > -			"func '%s' arg%d type %s is not a struct\n",
> > -			tname, arg, btf_kind_str[BTF_INFO_KIND(t->info)]);
> > -		return false;
> > -	}
> 
> Hi, Jiri, the RFC looks great! Especially, you also referenced this will
> give great performance boost for bcc scripts.
> 
> Could you provide more context on why the above change is needed?
> The function btf_ctx_access is used to check validity of accessing
> function parameters which are wrapped inside a structure, I am wondering
> what kinds of accesses you tried to address here.

when I was transforming opensnoop.py to use this I got fail in
there when I tried to access filename arg in do_sys_open

but actualy it seems this should get recognized earlier by:

          if (btf_type_is_int(t))
                /* accessing a scalar */
                return true;

I'm not sure why it did not pass for const char*, I'll check

jirka
Jiri Olsa Jan. 7, 2020, 3:50 p.m. UTC | #3
On Tue, Jan 07, 2020 at 01:13:23PM +0100, Jiri Olsa wrote:
> On Mon, Jan 06, 2020 at 09:36:17PM +0000, Yonghong Song wrote:
> > 
> > 
> > On 12/29/19 6:37 AM, Jiri Olsa wrote:
> > > I'm not sure why the restriction was added,
> > > but I can't access pointers to POD types like
> > > const char * when probing vfs_read function.
> > > 
> > > Removing the check and allow non struct type
> > > access in context.
> > > 
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >   kernel/bpf/btf.c | 6 ------
> > >   1 file changed, 6 deletions(-)
> > > 
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index ed2075884724..ae90f60ac1b8 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -3712,12 +3712,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > >   	/* skip modifiers */
> > >   	while (btf_type_is_modifier(t))
> > >   		t = btf_type_by_id(btf, t->type);
> > > -	if (!btf_type_is_struct(t)) {
> > > -		bpf_log(log,
> > > -			"func '%s' arg%d type %s is not a struct\n",
> > > -			tname, arg, btf_kind_str[BTF_INFO_KIND(t->info)]);
> > > -		return false;
> > > -	}
> > 
> > Hi, Jiri, the RFC looks great! Especially, you also referenced this will
> > give great performance boost for bcc scripts.
> > 
> > Could you provide more context on why the above change is needed?
> > The function btf_ctx_access is used to check validity of accessing
> > function parameters which are wrapped inside a structure, I am wondering
> > what kinds of accesses you tried to address here.
> 
> when I was transforming opensnoop.py to use this I got fail in
> there when I tried to access filename arg in do_sys_open
> 
> but actualy it seems this should get recognized earlier by:
> 
>           if (btf_type_is_int(t))
>                 /* accessing a scalar */
>                 return true;
> 
> I'm not sure why it did not pass for const char*, I'll check

it seems we don't check for pointer to scalar (just void),
which is the case in my example 'const char *filename'

I'll post this in v2 with other changes

jirka


---
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ed2075884724..650df4ed346e 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3633,7 +3633,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		    const struct bpf_prog *prog,
 		    struct bpf_insn_access_aux *info)
 {
-	const struct btf_type *t = prog->aux->attach_func_proto;
+	const struct btf_type *tp, *t = prog->aux->attach_func_proto;
 	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
 	struct btf *btf = bpf_prog_get_target_btf(prog);
 	const char *tname = prog->aux->attach_func_name;
@@ -3695,6 +3695,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		 */
 		return true;
 
+	tp = btf_type_by_id(btf, t->type);
+	/* skip modifiers */
+	while (btf_type_is_modifier(tp))
+		tp = btf_type_by_id(btf, tp->type);
+
+	if (btf_type_is_int(tp))
+		/* This is a pointer scalar.
+		 * It is the same as scalar from the verifier safety pov.
+		 */
+		return true;
+
 	/* this is a pointer to another type */
 	info->reg_type = PTR_TO_BTF_ID;
 	info->btf_id = t->type;
Yonghong Song Jan. 7, 2020, 5:55 p.m. UTC | #4
On 1/7/20 7:50 AM, Jiri Olsa wrote:
> On Tue, Jan 07, 2020 at 01:13:23PM +0100, Jiri Olsa wrote:
>> On Mon, Jan 06, 2020 at 09:36:17PM +0000, Yonghong Song wrote:
>>>
>>>
>>> On 12/29/19 6:37 AM, Jiri Olsa wrote:
>>>> I'm not sure why the restriction was added,
>>>> but I can't access pointers to POD types like
>>>> const char * when probing vfs_read function.
>>>>
>>>> Removing the check and allow non struct type
>>>> access in context.
>>>>
>>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>>> ---
>>>>    kernel/bpf/btf.c | 6 ------
>>>>    1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>>> index ed2075884724..ae90f60ac1b8 100644
>>>> --- a/kernel/bpf/btf.c
>>>> +++ b/kernel/bpf/btf.c
>>>> @@ -3712,12 +3712,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>>>    	/* skip modifiers */
>>>>    	while (btf_type_is_modifier(t))
>>>>    		t = btf_type_by_id(btf, t->type);
>>>> -	if (!btf_type_is_struct(t)) {
>>>> -		bpf_log(log,
>>>> -			"func '%s' arg%d type %s is not a struct\n",
>>>> -			tname, arg, btf_kind_str[BTF_INFO_KIND(t->info)]);
>>>> -		return false;
>>>> -	}
>>>
>>> Hi, Jiri, the RFC looks great! Especially, you also referenced this will
>>> give great performance boost for bcc scripts.
>>>
>>> Could you provide more context on why the above change is needed?
>>> The function btf_ctx_access is used to check validity of accessing
>>> function parameters which are wrapped inside a structure, I am wondering
>>> what kinds of accesses you tried to address here.
>>
>> when I was transforming opensnoop.py to use this I got fail in
>> there when I tried to access filename arg in do_sys_open
>>
>> but actualy it seems this should get recognized earlier by:
>>
>>            if (btf_type_is_int(t))
>>                  /* accessing a scalar */
>>                  return true;
>>
>> I'm not sure why it did not pass for const char*, I'll check
> 
> it seems we don't check for pointer to scalar (just void),
> which is the case in my example 'const char *filename'

Thanks for clarification. See some comments below.

> 
> I'll post this in v2 with other changes
> 
> jirka
> 
> 
> ---
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index ed2075884724..650df4ed346e 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3633,7 +3633,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>   		    const struct bpf_prog *prog,
>   		    struct bpf_insn_access_aux *info)
>   {
> -	const struct btf_type *t = prog->aux->attach_func_proto;
> +	const struct btf_type *tp, *t = prog->aux->attach_func_proto;
>   	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
>   	struct btf *btf = bpf_prog_get_target_btf(prog);
>   	const char *tname = prog->aux->attach_func_name;
> @@ -3695,6 +3695,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>   		 */
>   		return true;
>   
> +	tp = btf_type_by_id(btf, t->type);
> +	/* skip modifiers */
> +	while (btf_type_is_modifier(tp))
> +		tp = btf_type_by_id(btf, tp->type);
> +
> +	if (btf_type_is_int(tp))
> +		/* This is a pointer scalar.
> +		 * It is the same as scalar from the verifier safety pov.
> +		 */
> +		return true;

This should work since:
    - the int pointer will be treated as a scalar later on
    - bpf_probe_read() will be used to read the contents

I am wondering whether we should add proper verifier support
to allow pointer to int ctx access. There, users do not need
to use bpf_probe_read() to dereference the pointer.

Discussed with Martin, maybe somewhere in check_ptr_to_btf_access(),
before btf_struct_access(), checking if it is a pointer to int/enum,
it should just allow and return SCALAR_VALUE.

If you do verifier changes, please ensure bpf_probe_read() is not
needed any more. In bcc, you need to hack to prevent rewriter to
re-introduce bpf_probe_read() :-).

> +
>   	/* this is a pointer to another type */
>   	info->reg_type = PTR_TO_BTF_ID;
>   	info->btf_id = t->type;
>
Yonghong Song Jan. 7, 2020, 6:28 p.m. UTC | #5
On 1/7/20 9:55 AM, Yonghong Song wrote:
> 
> 
> On 1/7/20 7:50 AM, Jiri Olsa wrote:
>> On Tue, Jan 07, 2020 at 01:13:23PM +0100, Jiri Olsa wrote:
>>> On Mon, Jan 06, 2020 at 09:36:17PM +0000, Yonghong Song wrote:
>>>>
>>>>
>>>> On 12/29/19 6:37 AM, Jiri Olsa wrote:
>>>>> I'm not sure why the restriction was added,
>>>>> but I can't access pointers to POD types like
>>>>> const char * when probing vfs_read function.
>>>>>
>>>>> Removing the check and allow non struct type
>>>>> access in context.
>>>>>
>>>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>>>> ---
>>>>>     kernel/bpf/btf.c | 6 ------
>>>>>     1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>>>> index ed2075884724..ae90f60ac1b8 100644
>>>>> --- a/kernel/bpf/btf.c
>>>>> +++ b/kernel/bpf/btf.c
>>>>> @@ -3712,12 +3712,6 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>>>>     	/* skip modifiers */
>>>>>     	while (btf_type_is_modifier(t))
>>>>>     		t = btf_type_by_id(btf, t->type);
>>>>> -	if (!btf_type_is_struct(t)) {
>>>>> -		bpf_log(log,
>>>>> -			"func '%s' arg%d type %s is not a struct\n",
>>>>> -			tname, arg, btf_kind_str[BTF_INFO_KIND(t->info)]);
>>>>> -		return false;
>>>>> -	}
>>>>
>>>> Hi, Jiri, the RFC looks great! Especially, you also referenced this will
>>>> give great performance boost for bcc scripts.
>>>>
>>>> Could you provide more context on why the above change is needed?
>>>> The function btf_ctx_access is used to check validity of accessing
>>>> function parameters which are wrapped inside a structure, I am wondering
>>>> what kinds of accesses you tried to address here.
>>>
>>> when I was transforming opensnoop.py to use this I got fail in
>>> there when I tried to access filename arg in do_sys_open
>>>
>>> but actualy it seems this should get recognized earlier by:
>>>
>>>             if (btf_type_is_int(t))
>>>                   /* accessing a scalar */
>>>                   return true;
>>>
>>> I'm not sure why it did not pass for const char*, I'll check
>>
>> it seems we don't check for pointer to scalar (just void),
>> which is the case in my example 'const char *filename'
> 
> Thanks for clarification. See some comments below.
> 
>>
>> I'll post this in v2 with other changes
>>
>> jirka
>>
>>
>> ---
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index ed2075884724..650df4ed346e 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -3633,7 +3633,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>    		    const struct bpf_prog *prog,
>>    		    struct bpf_insn_access_aux *info)
>>    {
>> -	const struct btf_type *t = prog->aux->attach_func_proto;
>> +	const struct btf_type *tp, *t = prog->aux->attach_func_proto;
>>    	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
>>    	struct btf *btf = bpf_prog_get_target_btf(prog);
>>    	const char *tname = prog->aux->attach_func_name;
>> @@ -3695,6 +3695,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>>    		 */
>>    		return true;
>>    
>> +	tp = btf_type_by_id(btf, t->type);
>> +	/* skip modifiers */
>> +	while (btf_type_is_modifier(tp))
>> +		tp = btf_type_by_id(btf, tp->type);
>> +
>> +	if (btf_type_is_int(tp))
>> +		/* This is a pointer scalar.
>> +		 * It is the same as scalar from the verifier safety pov.
>> +		 */
>> +		return true;
> 
> This should work since:
>      - the int pointer will be treated as a scalar later on
>      - bpf_probe_read() will be used to read the contents
> 
> I am wondering whether we should add proper verifier support
> to allow pointer to int ctx access. There, users do not need
> to use bpf_probe_read() to dereference the pointer.
> 
> Discussed with Martin, maybe somewhere in check_ptr_to_btf_access(),
> before btf_struct_access(), checking if it is a pointer to int/enum,
> it should just allow and return SCALAR_VALUE.

double checked check_ptr_to_btf_access() and btf_struct_access().
btf_struct_access() already returns SCALAR_VALUE for pointer to 
int/enum. So verifier change is probably not needed.

In your above code, could you do
    btf_type_is_int(t) || btf_type_is_enum(t)
which will cover pointer to enum as well?

> 
> If you do verifier changes, please ensure bpf_probe_read() is not
> needed any more. In bcc, you need to hack to prevent rewriter to
> re-introduce bpf_probe_read() :-).
> 
>> +
>>    	/* this is a pointer to another type */
>>    	info->reg_type = PTR_TO_BTF_ID;
>>    	info->btf_id = t->type;
>>
Jiri Olsa Jan. 8, 2020, 2:38 p.m. UTC | #6
On Tue, Jan 07, 2020 at 06:28:11PM +0000, Yonghong Song wrote:

SNIP

> >> index ed2075884724..650df4ed346e 100644
> >> --- a/kernel/bpf/btf.c
> >> +++ b/kernel/bpf/btf.c
> >> @@ -3633,7 +3633,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >>    		    const struct bpf_prog *prog,
> >>    		    struct bpf_insn_access_aux *info)
> >>    {
> >> -	const struct btf_type *t = prog->aux->attach_func_proto;
> >> +	const struct btf_type *tp, *t = prog->aux->attach_func_proto;
> >>    	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
> >>    	struct btf *btf = bpf_prog_get_target_btf(prog);
> >>    	const char *tname = prog->aux->attach_func_name;
> >> @@ -3695,6 +3695,17 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >>    		 */
> >>    		return true;
> >>    
> >> +	tp = btf_type_by_id(btf, t->type);
> >> +	/* skip modifiers */
> >> +	while (btf_type_is_modifier(tp))
> >> +		tp = btf_type_by_id(btf, tp->type);
> >> +
> >> +	if (btf_type_is_int(tp))
> >> +		/* This is a pointer scalar.
> >> +		 * It is the same as scalar from the verifier safety pov.
> >> +		 */
> >> +		return true;
> > 
> > This should work since:
> >      - the int pointer will be treated as a scalar later on
> >      - bpf_probe_read() will be used to read the contents
> > 
> > I am wondering whether we should add proper verifier support
> > to allow pointer to int ctx access. There, users do not need
> > to use bpf_probe_read() to dereference the pointer.
> > 
> > Discussed with Martin, maybe somewhere in check_ptr_to_btf_access(),
> > before btf_struct_access(), checking if it is a pointer to int/enum,
> > it should just allow and return SCALAR_VALUE.
> 
> double checked check_ptr_to_btf_access() and btf_struct_access().
> btf_struct_access() already returns SCALAR_VALUE for pointer to 
> int/enum. So verifier change is probably not needed.

ok, great

> 
> In your above code, could you do
>     btf_type_is_int(t) || btf_type_is_enum(t)
> which will cover pointer to enum as well?

sure, I'll include that

thanks,
jirka
diff mbox series

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ed2075884724..ae90f60ac1b8 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3712,12 +3712,6 @@  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	/* skip modifiers */
 	while (btf_type_is_modifier(t))
 		t = btf_type_by_id(btf, t->type);
-	if (!btf_type_is_struct(t)) {
-		bpf_log(log,
-			"func '%s' arg%d type %s is not a struct\n",
-			tname, arg, btf_kind_str[BTF_INFO_KIND(t->info)]);
-		return false;
-	}
 	bpf_log(log, "func '%s' arg%d has btf_id %d type %s '%s'\n",
 		tname, arg, info->btf_id, btf_kind_str[BTF_INFO_KIND(t->info)],
 		__btf_name_by_offset(btf, t->name_off));