[bpf-next,v2,0/4] bpf: make trusted args nullable

Message

Vadim Fedorenko May 10, 2024, 12:28 p.m. UTC

Current verifier checks for the arg to be nullable after checking for
certain pointer types. It prevents programs to pass NULL to kfunc args
even if they are marked as nullable. This patchset adjusts verifier and
changes bpf crypto kfuncs to allow null for IV parameter which is
optional for some ciphers. Benchmark shows ~4% improvements when there
is no need to initialise 0-sized dynptr.

v2:
- adjust kdoc accordingly

Vadim Fedorenko (4):
  bpf: verifier: make kfuncs args nullalble
  bpf: crypto: make state and IV dynptr nullable
  selftests: bpf: crypto: use NULL instead of 0-sized dynptr
  selftests: bpf: crypto: adjust bench to use nullable IV

 kernel/bpf/crypto.c                           | 26 +++++++++----------
 kernel/bpf/verifier.c                         |  6 ++---
 .../selftests/bpf/progs/crypto_bench.c        | 10 +++----
 .../selftests/bpf/progs/crypto_sanity.c       | 16 +++---------
 4 files changed, 24 insertions(+), 34 deletions(-)

Comments

Eduard Zingerman May 21, 2024, 7:46 p.m. UTC | #1

On Fri, 2024-05-10 at 05:28 -0700, Vadim Fedorenko wrote:
> Current verifier checks for the arg to be nullable after checking for
> certain pointer types. It prevents programs to pass NULL to kfunc args
> even if they are marked as nullable. This patchset adjusts verifier and
> changes bpf crypto kfuncs to allow null for IV parameter which is
> optional for some ciphers. Benchmark shows ~4% improvements when there
> is no need to initialise 0-sized dynptr.
> 
> v2:
> - adjust kdoc accordingly
> 

Hi Vadim, sorry for late response.

I think that this patch-set looks good,
but I'd like to ask you to add a dedicated test to the following file:
tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c.
(crypto tests are for crypto and might be modified in the future
 w/o consideration of verifier pathways they currently test).

Also nullable dynptr sounds kind-of funny.
As far as I understand, performance gains are due to omission of extra
function call. Did you consider inlining for bpf_dynptr_from_mem()?
Same way it is done for bpf_kptr_xchg() in verifier.c:do_misc_fixups().

Thanks,
Eduard

Vadim Fedorenko May 22, 2024, 4:34 p.m. UTC | #2

On 22/05/2024 16:23, Eduard Zingerman wrote:
> On Wed, 2024-05-22 at 13:35 +0100, Vadim Fedorenko wrote:
> 
> [...]
> 
>> I'm not really sure how I can test it without fully replicating crypto
>> tests. We don't have any other kfuncs with nullable dynptrs yet to test,
>> maybe we should revisit this part once we have more functions with
>> nullable parameters.
> 
> kfuncs for testing could be defined in
> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:bpf_testmod.c,
> e.g. see bpf_testmod_test_mod_kfunc().

alright, thanks for the pointer, I'll add some tests with kfuncs.

Martin KaFai Lau May 22, 2024, 6:06 p.m. UTC | #3

On 5/22/24 9:34 AM, Vadim Fedorenko wrote:
> On 22/05/2024 16:23, Eduard Zingerman wrote:
>> On Wed, 2024-05-22 at 13:35 +0100, Vadim Fedorenko wrote:
>>
>> [...]
>>
>>> I'm not really sure how I can test it without fully replicating crypto
>>> tests. We don't have any other kfuncs with nullable dynptrs yet to test,
>>> maybe we should revisit this part once we have more functions with
>>> nullable parameters.
>>
>> kfuncs for testing could be defined in
>> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:bpf_testmod.c,
>> e.g. see bpf_testmod_test_mod_kfunc().
> 
> alright, thanks for the pointer, I'll add some tests with kfuncs.
> 
+1

pw-bot: cr