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