[bpf-next,1/2] bpf: Relax KF_ACQUIRE kfuncs strict type matching constraint on non-zero offset pointers

Commit Message

Juntong Deng Aug. 16, 2024, 1:23 p.m. UTC

Currently the non-zero offset pointer constraint for KF_TRUSTED_ARGS
kfuncs has been relaxed in commit 605c96997d89 ("bpf: relax zero fixed
offset constraint on KF_TRUSTED_ARGS/KF_RCU"), which means that non-zero
offset does not affect whether a pointer is valid.

But currently we still cannot pass non-zero offset pointers to
KF_ACQUIRE kfuncs. This is because KF_ACQUIRE kfuncs requires strict
type matching, but non-zero offset does not change the type of pointer,
which causes the ebpf program to be rejected by the verifier.

This can cause some problems, one example is that bpf_skb_peek_tail
kfunc [0] cannot be implemented by just passing in non-zero offset
pointers.

This patch makes KF_ACQUIRE kfuncs not require strict type matching
on non-zero offset pointers.

[0]: https://lore.kernel.org/bpf/[e-mail]/

Signed-off-by: Juntong Deng <[e-mail]>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexei Starovoitov Aug. 21, 2024, 6:02 p.m. UTC | #1

On Fri, Aug 16, 2024 at 6:24 AM Juntong Deng <[e-mail]> wrote:
>
> Currently the non-zero offset pointer constraint for KF_TRUSTED_ARGS
> kfuncs has been relaxed in commit 605c96997d89 ("bpf: relax zero fixed
> offset constraint on KF_TRUSTED_ARGS/KF_RCU"), which means that non-zero
> offset does not affect whether a pointer is valid.
>
> But currently we still cannot pass non-zero offset pointers to
> KF_ACQUIRE kfuncs. This is because KF_ACQUIRE kfuncs requires strict
> type matching, but non-zero offset does not change the type of pointer,
> which causes the ebpf program to be rejected by the verifier.
>
> This can cause some problems, one example is that bpf_skb_peek_tail
> kfunc [0] cannot be implemented by just passing in non-zero offset
> pointers.
>
> This patch makes KF_ACQUIRE kfuncs not require strict type matching
> on non-zero offset pointers.
>
> [0]: https://lore.kernel.org/bpf/[e-mail]/
>
> Signed-off-by: Juntong Deng <[e-mail]>
> ---
>  kernel/bpf/verifier.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index ebec74c28ae3..3a14002d24a0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11484,7 +11484,7 @@ static int process_kf_arg_ptr_to_btf_id(struct bpf_verifier_env *env,
>          * btf_struct_ids_match() to walk the struct at the 0th offset, and
>          * resolve types.
>          */
> -       if (is_kfunc_acquire(meta) ||
> +       if ((is_kfunc_acquire(meta) && !reg->off) ||

Agree that relaxing is fine and calling acquire kfunc like:
  bpf_kfunc_nested_acquire_test(&sk->sk_write_queue);

should be allowed,
but above check is strange, since
if offsetof(&sk_write_queue) == 0
it will disallow calling a kfunc.
I mean if the field is the first in the outer struct this
condition will force strict type match which will fail, right?

So should we remove the above is_kfunc_acquire() check instead?

pw-bot: cr

Juntong Deng Aug. 28, 2024, 8:12 p.m. UTC | #2

On 8/21/24 19:02, Alexei Starovoitov wrote:
> On Fri, Aug 16, 2024 at 6:24 AM Juntong Deng <[e-mail]> wrote:
>>
>> Currently the non-zero offset pointer constraint for KF_TRUSTED_ARGS
>> kfuncs has been relaxed in commit 605c96997d89 ("bpf: relax zero fixed
>> offset constraint on KF_TRUSTED_ARGS/KF_RCU"), which means that non-zero
>> offset does not affect whether a pointer is valid.
>>
>> But currently we still cannot pass non-zero offset pointers to
>> KF_ACQUIRE kfuncs. This is because KF_ACQUIRE kfuncs requires strict
>> type matching, but non-zero offset does not change the type of pointer,
>> which causes the ebpf program to be rejected by the verifier.
>>
>> This can cause some problems, one example is that bpf_skb_peek_tail
>> kfunc [0] cannot be implemented by just passing in non-zero offset
>> pointers.
>>
>> This patch makes KF_ACQUIRE kfuncs not require strict type matching
>> on non-zero offset pointers.
>>
>> [0]: https://lore.kernel.org/bpf/[e-mail]/
>>
>> Signed-off-by: Juntong Deng <[e-mail]>
>> ---
>>   kernel/bpf/verifier.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index ebec74c28ae3..3a14002d24a0 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -11484,7 +11484,7 @@ static int process_kf_arg_ptr_to_btf_id(struct bpf_verifier_env *env,
>>           * btf_struct_ids_match() to walk the struct at the 0th offset, and
>>           * resolve types.
>>           */
>> -       if (is_kfunc_acquire(meta) ||
>> +       if ((is_kfunc_acquire(meta) && !reg->off) ||
> 
> Agree that relaxing is fine and calling acquire kfunc like:
>    bpf_kfunc_nested_acquire_test(&sk->sk_write_queue);
> 
> should be allowed,
> but above check is strange, since
> if offsetof(&sk_write_queue) == 0
> it will disallow calling a kfunc.
> I mean if the field is the first in the outer struct this
> condition will force strict type match which will fail, right?
> 
> So should we remove the above is_kfunc_acquire() check instead?
> 
> pw-bot: cr

I agree that if strict type matching is not required for both zero
offset and non-zero pointers, then we can remove the strict type
matching for KF_ACQUIRE kfuncs.

I sent the version 2 patch set:
https://lore.kernel.org/bpf/[e-mail]/T/#t

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ebec74c28ae3..3a14002d24a0 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11484,7 +11484,7 @@  static int process_kf_arg_ptr_to_btf_id(struct bpf_verifier_env *env,
 	 * btf_struct_ids_match() to walk the struct at the 0th offset, and
 	 * resolve types.
 	 */
-	if (is_kfunc_acquire(meta) ||
+	if ((is_kfunc_acquire(meta) && !reg->off) ||
 	    (is_kfunc_release(meta) && reg->ref_obj_id) ||
 	    btf_type_ids_nocast_alias(&env->log, reg_btf, reg_ref_id, meta->btf, ref_id))
 		strict_type_match = true;