While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
need to check the program type of the target program to grant the read /
write access to the packet data.
The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
placeholder for those, we need this extended check for those target
programs to actually work while using this option.
Tested this with a freplace xdp program. Without this patch, the
verifier fails with error 'cannot write into packet'.
Signed-off-by: Udip Pant <[email protected]>
---
kernel/bpf/verifier.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ef938f17b944..4d7604430994 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
const struct bpf_call_arg_meta *meta,
enum bpf_access_type t)
{
- switch (env->prog->type) {
+ struct bpf_prog *prog = env->prog;
+ enum bpf_prog_type prog_type = prog->aux->linked_prog ?
+ prog->aux->linked_prog->type : prog->type;
+
+ switch (prog_type) {
/* Program types only with direct read access go here! */
case BPF_PROG_TYPE_LWT_IN:
case BPF_PROG_TYPE_LWT_OUT:
--
2.24.1
On 8/20/20 5:28 PM, Udip Pant wrote:
> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
> need to check the program type of the target program to grant the read /
> write access to the packet data.
>
> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
> placeholder for those, we need this extended check for those target
> programs to actually work while using this option.
>
> Tested this with a freplace xdp program. Without this patch, the
> verifier fails with error 'cannot write into packet'.
>
> Signed-off-by: Udip Pant <[email protected]>
> ---
> kernel/bpf/verifier.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ef938f17b944..4d7604430994 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
> const struct bpf_call_arg_meta *meta,
> enum bpf_access_type t)
> {
> - switch (env->prog->type) {
> + struct bpf_prog *prog = env->prog;
> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
> + prog->aux->linked_prog->type : prog->type;
I checked the verifier code. There are several places where
prog->type is checked and EXT program type will behave differently
from the linked program.
Maybe abstract the the above logic to one static function like
static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
{
return prog->aux->linked_prog ? prog->aux->linked_prog->type
: prog->type;
}
This function can then be used in different places to give the resolved
prog type.
Besides here checking pkt access permission,
another possible places to consider is return value
in function check_return_code(). Currently,
for EXT program, the result value can be anything. This may need to
be enforced. Could you take a look? It could be others as well.
You can take a look at verifier.c by searching "prog->type".
> +
> + switch (prog_type) {
> /* Program types only with direct read access go here! */
> case BPF_PROG_TYPE_LWT_IN:
> case BPF_PROG_TYPE_LWT_OUT:
>
On 8/20/20 11:13 PM, Yonghong Song wrote:
>
>
> On 8/20/20 5:28 PM, Udip Pant wrote:
>> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
>> need to check the program type of the target program to grant the read /
>> write access to the packet data.
>>
>> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
>> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
>> placeholder for those, we need this extended check for those target
>> programs to actually work while using this option.
>>
>> Tested this with a freplace xdp program. Without this patch, the
>> verifier fails with error 'cannot write into packet'.
>>
>> Signed-off-by: Udip Pant <[email protected]>
>> ---
>> kernel/bpf/verifier.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index ef938f17b944..4d7604430994 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct
>> bpf_verifier_env *env,
>> const struct bpf_call_arg_meta *meta,
>> enum bpf_access_type t)
>> {
>> - switch (env->prog->type) {
>> + struct bpf_prog *prog = env->prog;
>> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
>> + prog->aux->linked_prog->type : prog->type;
>
> I checked the verifier code. There are several places where
> prog->type is checked and EXT program type will behave differently
> from the linked program.
>
> Maybe abstract the the above logic to one static function like
>
> static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
> {
> return prog->aux->linked_prog ? prog->aux->linked_prog->type
> : prog->type;
> }
>
> This function can then be used in different places to give the resolved
> prog type.
>
> Besides here checking pkt access permission,
> another possible places to consider is return value
> in function check_return_code(). Currently,
> for EXT program, the result value can be anything. This may need to
> be enforced. Could you take a look? It could be others as well.
> You can take a look at verifier.c by searching "prog->type".
Note that if the EXT program tries to replace a global subprogram,
then return value cannot be enforced, just as what Patch #2 example shows.
>
>> +
>> + switch (prog_type) {
>> /* Program types only with direct read access go here! */
>> case BPF_PROG_TYPE_LWT_IN:
>> case BPF_PROG_TYPE_LWT_OUT:
>>
> On 8/20/20, 11:17 PM, "Yonghong Song" <[email protected]> wrote:
>
>
>
> On 8/20/20 11:13 PM, Yonghong Song wrote:
>>
>>
>> On 8/20/20 5:28 PM, Udip Pant wrote:
>>> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
>>> need to check the program type of the target program to grant the read /
>>> write access to the packet data.
>>>
>>> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
>>> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
>>> placeholder for those, we need this extended check for those target
>>> programs to actually work while using this option.
>>>
>>> Tested this with a freplace xdp program. Without this patch, the
>>> verifier fails with error 'cannot write into packet'.
>>>
>>> Signed-off-by: Udip Pant <[email protected]>
>>> ---
>>> kernel/bpf/verifier.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index ef938f17b944..4d7604430994 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct
>>> bpf_verifier_env *env,
>>> const struct bpf_call_arg_meta *meta,
>>> enum bpf_access_type t)
>>> {
>>> - switch (env->prog->type) {
>>> + struct bpf_prog *prog = env->prog;
>>> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
>>> + prog->aux->linked_prog->type : prog->type;
>>
>> I checked the verifier code. There are several places where
>> prog->type is checked and EXT program type will behave differently
>> from the linked program.
>>
>> Maybe abstract the the above logic to one static function like
>>
>> static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
>> {
>> return prog->aux->linked_prog ? prog->aux->linked_prog->type
>> : prog->type;
>> }
>>
Sure.
>> This function can then be used in different places to give the resolved
>> prog type.
>>
>> Besides here checking pkt access permission,
>> another possible places to consider is return value
>> in function check_return_code(). Currently,
>> for EXT program, the result value can be anything. This may need to
>> be enforced. Could you take a look? It could be others as well.
>> You can take a look at verifier.c by searching "prog->type".
>
Yeah there are few other places in the verifier where it decides without resolving for the 'extended' type. But I am not too sure if it makes sense to extend this logic as part of this commit. For example, as you mentioned, in the check_return_code() it explicitly ignores the return type for the EXT prog (kernel/bpf/verifier.c#L7446). Likewise, I noticed similar issue inside the check_ld_abs(), where it checks for may_access_skb(env->prog->type).
I'm happy to extend this logic there as well if deemed appropriate.
> Note that if the EXT program tries to replace a global subprogram,
> then return value cannot be enforced, just as what Patch #2 example shows.
>
>>
>>> +
>>> + switch (prog_type) {
>>> /* Program types only with direct read access go here! */
>>> case BPF_PROG_TYPE_LWT_IN:
>>> case BPF_PROG_TYPE_LWT_OUT:
>>>
On 8/21/20 12:07 PM, Udip Pant wrote:
>
>
> > On 8/20/20, 11:17 PM, "Yonghong Song" <[email protected]> wrote:
>>
>>
>>
>> On 8/20/20 11:13 PM, Yonghong Song wrote:
>>>
>>>
>>> On 8/20/20 5:28 PM, Udip Pant wrote:
>>>> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
>>>> need to check the program type of the target program to grant the read /
>>>> write access to the packet data.
>>>>
>>>> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
>>>> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
>>>> placeholder for those, we need this extended check for those target
>>>> programs to actually work while using this option.
>>>>
>>>> Tested this with a freplace xdp program. Without this patch, the
>>>> verifier fails with error 'cannot write into packet'.
>>>>
>>>> Signed-off-by: Udip Pant <[email protected]>
>>>> ---
>>>> kernel/bpf/verifier.c | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index ef938f17b944..4d7604430994 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct
>>>> bpf_verifier_env *env,
>>>> const struct bpf_call_arg_meta *meta,
>>>> enum bpf_access_type t)
>>>> {
>>>> - switch (env->prog->type) {
>>>> + struct bpf_prog *prog = env->prog;
>>>> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
>>>> + prog->aux->linked_prog->type : prog->type;
>>>
>>> I checked the verifier code. There are several places where
>>> prog->type is checked and EXT program type will behave differently
>>> from the linked program.
>>>
>>> Maybe abstract the the above logic to one static function like
>>>
>>> static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
>>> {
>>> return prog->aux->linked_prog ? prog->aux->linked_prog->type
>>> : prog->type;
>>> }
>>>
>
> Sure.
>
>>> This function can then be used in different places to give the resolved
>>> prog type.
>>>
>>> Besides here checking pkt access permission,
>>> another possible places to consider is return value
>>> in function check_return_code(). Currently,
>>> for EXT program, the result value can be anything. This may need to
>>> be enforced. Could you take a look? It could be others as well.
>>> You can take a look at verifier.c by searching "prog->type".
>>
>
> Yeah there are few other places in the verifier where it decides without resolving for the 'extended' type. But I am not too sure if it makes sense to extend this logic as part of this commit. For example, as you mentioned, in the check_return_code() it explicitly ignores the return type for the EXT prog (kernel/bpf/verifier.c#L7446). Likewise, I noticed similar issue inside the check_ld_abs(), where it checks for may_access_skb(env->prog->type).
>
> I'm happy to extend this logic there as well if deemed appropriate.
Thanks. I would like to see the verifier parity between original program
and replace program. That is, if the original program and the replace
program are the same, they should be both either accepted or rejected
by verifier. Yes, this may imply more changes e.g., check_return_code()
or check_ld_abs() than your original patch.
Alexei or Daniel, what is your opinion on this?
>
>> Note that if the EXT program tries to replace a global subprogram,
>> then return value cannot be enforced, just as what Patch #2 example shows.
>>
>>>
>>>> +
>>>> + switch (prog_type) {
>>>> /* Program types only with direct read access go here! */
>>>> case BPF_PROG_TYPE_LWT_IN:
>>>> case BPF_PROG_TYPE_LWT_OUT:
>>>>
>
On Fri, Aug 21, 2020 at 1:53 PM Yonghong Song <[email protected]> wrote:
>
>
>
> On 8/21/20 12:07 PM, Udip Pant wrote:
> >
> >
> > > On 8/20/20, 11:17 PM, "Yonghong Song" <[email protected]> wrote:
> >>
> >>
> >>
> >> On 8/20/20 11:13 PM, Yonghong Song wrote:
> >>>
> >>>
> >>> On 8/20/20 5:28 PM, Udip Pant wrote:
> >>>> While using dynamic program extension (of type BPF_PROG_TYPE_EXT), we
> >>>> need to check the program type of the target program to grant the read /
> >>>> write access to the packet data.
> >>>>
> >>>> The BPF_PROG_TYPE_EXT type can be used to extend types such as XDP, SKB
> >>>> and others. Since the BPF_PROG_TYPE_EXT program type on itself is just a
> >>>> placeholder for those, we need this extended check for those target
> >>>> programs to actually work while using this option.
> >>>>
> >>>> Tested this with a freplace xdp program. Without this patch, the
> >>>> verifier fails with error 'cannot write into packet'.
> >>>>
> >>>> Signed-off-by: Udip Pant <[email protected]>
> >>>> ---
> >>>> kernel/bpf/verifier.c | 6 +++++-
> >>>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >>>> index ef938f17b944..4d7604430994 100644
> >>>> --- a/kernel/bpf/verifier.c
> >>>> +++ b/kernel/bpf/verifier.c
> >>>> @@ -2629,7 +2629,11 @@ static bool may_access_direct_pkt_data(struct
> >>>> bpf_verifier_env *env,
> >>>> const struct bpf_call_arg_meta *meta,
> >>>> enum bpf_access_type t)
> >>>> {
> >>>> - switch (env->prog->type) {
> >>>> + struct bpf_prog *prog = env->prog;
> >>>> + enum bpf_prog_type prog_type = prog->aux->linked_prog ?
> >>>> + prog->aux->linked_prog->type : prog->type;
> >>>
> >>> I checked the verifier code. There are several places where
> >>> prog->type is checked and EXT program type will behave differently
> >>> from the linked program.
> >>>
> >>> Maybe abstract the the above logic to one static function like
> >>>
> >>> static enum bpf_prog_type resolved_prog_type(struct bpf_prog *prog)
> >>> {
> >>> return prog->aux->linked_prog ? prog->aux->linked_prog->type
> >>> : prog->type;
> >>> }
> >>>
> >
> > Sure.
> >
> >>> This function can then be used in different places to give the resolved
> >>> prog type.
> >>>
> >>> Besides here checking pkt access permission,
> >>> another possible places to consider is return value
> >>> in function check_return_code(). Currently,
> >>> for EXT program, the result value can be anything. This may need to
> >>> be enforced. Could you take a look? It could be others as well.
> >>> You can take a look at verifier.c by searching "prog->type".
> >>
> >
> > Yeah there are few other places in the verifier where it decides without resolving for the 'extended' type. But I am not too sure if it makes sense to extend this logic as part of this commit. For example, as you mentioned, in the check_return_code() it explicitly ignores the return type for the EXT prog (kernel/bpf/verifier.c#L7446). Likewise, I noticed similar issue inside the check_ld_abs(), where it checks for may_access_skb(env->prog->type).
> >
> > I'm happy to extend this logic there as well if deemed appropriate.
>
> Thanks. I would like to see the verifier parity between original program
> and replace program. That is, if the original program and the replace
> program are the same, they should be both either accepted or rejected
> by verifier. Yes, this may imply more changes e.g., check_return_code()
> or check_ld_abs() than your original patch.
> Alexei or Daniel, what is your opinion on this?
The set was already marked as 'changes requested' in patchworks.
That's an indication that maintainers agree with the feedback :)
In this particular case it certainly makes sense to address all cases
instead of doing them one at a time.