Re: [PATCH v2 06/25] tcp: authopt: Compute packet signatures

[Leonard Crestez <cdleonard@gmail.com>]

On 11/5/21 3:53 AM, Dmitry Safonov wrote:
> On 11/1/21 16:34, Leonard Crestez wrote:
> [..]
>> +static int skb_shash_frags(struct shash_desc *desc,
>> +			   struct sk_buff *skb)
>> +{
>> +	struct sk_buff *frag_iter;
>> +	int err, i;
>> +
>> +	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
>> +		skb_frag_t *f = &skb_shinfo(skb)->frags[i];
>> +		u32 p_off, p_len, copied;
>> +		struct page *p;
>> +		u8 *vaddr;
>> +
>> +		skb_frag_foreach_page(f, skb_frag_off(f), skb_frag_size(f),
>> +				      p, p_off, p_len, copied) {
>> +			vaddr = kmap_atomic(p);
>> +			err = crypto_shash_update(desc, vaddr + p_off, p_len);
>> +			kunmap_atomic(vaddr);
>> +			if (err)
>> +				return err;
>> +		}
>> +	}
>> +
>> +	skb_walk_frags(skb, frag_iter) {
>> +		err = skb_shash_frags(desc, frag_iter);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> This seems quite sub-optimal: IIUC, shash should only be used for small
> amount of hashing. That's why tcp-md5 uses ahash with scatterlists.

There is indeed no good reason to prefer shash over ahash. Despite the 
"async" in the name it's possible to use it in atomic context.

> Which drives me to the question: why not reuse tcp_md5sig_pool code?
> 
> And it seems that you can avoid TCP_AUTHOPT_ALG_* enum and just supply
> to crypto the string from socket option (like xfrm does).
> 
> Here is my idea:
> https://lore.kernel.org/all/20211105014953.972946-6-dima@arista.com/T/#u

Making the md5 pool more generic and reusing it can work.

This "pool" mechanism is really just a workaround for the crypto API not 
supporting the allocation of a hash in softirq context. It would make a 
lot sense for this functionality to be part of the crypto layer itself.

Looking at your generic tcp_sig_crypto there is nothing actually 
specific to TCP in there: it's just an ahash and a scratch buffer per-cpu.

I don't understand the interest in using arbitrary crypto algorithms 
beyond RFC5926, this series is already complex enough. Other than 
increasing the complexity of crypto allocation there are various stack 
allocations which would need to be up to the maximum size of a TCP options.

--
Regards,
Leonard