Re: [PATCH bpf-next 1/2] bpf: add skcipher API support to TC/XDP programs

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 26.10.2023 22:47, Jakub Kicinski wrote:
> On Wed, 25 Oct 2023 18:59:37 -0700 Vadim Fedorenko wrote:
>> Add crypto API support to BPF to be able to decrypt or encrypt packets
>> in TC/XDP BPF programs. Only symmetric key ciphers are supported for
>> now. Special care should be taken for initialization part of crypto algo
>> because crypto_alloc_sync_skcipher() doesn't work with preemtion
>> disabled, it can be run only in sleepable BPF program. Also async crypto
>> is not supported because of the very same issue - TC/XDP BPF programs
>> are not sleepable.
> 
> Do CC crypto@ for the next version, please.

Sure

>> +/**
>> + * struct bpf_crypto_skcipher_ctx - refcounted BPF sync skcipher context structure
>> + * @tfm:	The pointer to crypto_sync_skcipher struct.
>> + * @rcu:	The RCU head used to free the crypto context with RCU safety.
>> + * @usage:	Object reference counter. When the refcount goes to 0, the
>> + *		memory is released back to the BPF allocator, which provides
>> + *		RCU safety.
>> + */
>> +
> 
> spurious newline?

yeah, will fix it

>> +struct bpf_crypto_skcipher_ctx {
> 
>> +/**
>> + * bpf_crypto_skcipher_ctx_acquire() - Acquire a reference to a BPF crypto context.
> 
> The contexts are refcounted and can be placed in maps?

Yes, the idea was to avoid allocation of algo object and setting the key on hot
path. And for now there is no way to allocate crypto cipher object in TC/XDP
hook because it uses GFP_KERNEL and delayed module load.

> Does anything prevent them from being used simultaneously
> by difference CPUs?

The algorithm configuration and the key can be used by different CPUs
simultaneously

>> +	case BPF_DYNPTR_TYPE_SKB:
>> +		return skb_pointer_if_linear(ptr->data, ptr->offset, __bpf_dynptr_size(ptr));
> 
> dynptr takes care of checking if skb can be written to?

dynptr is used to take care of size checking, but this particular part is used
to provide plain buffer from skb. I'm really sure if we can (or should) encrypt
or decrypt in-place, so API now assumes that src and dst are different buffers.