crypto: aesni - add ccm(aes) algorithm implementation

Add ccm(aes) implementation from linux-wireless mailing list (see
http://permalink.gmane.org/gmane.linux.kernel.wireless.general/126679).

This eliminates FPU context store/restore overhead existing in more general ccm_base(ctr(aes-aesni),aes-aesni) case in MAC calculation.

Suggested-by Ben Greear <[email protected]>

Co-developed-by Steve deRosier <[email protected]>

Signed-off-by Steve deRosier <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
v2: avoid the SIMD helper, as it produces an CRYPTO_ALG_ASYNC aead, which
is not usable by the 802.11 ccmp driver

  arch/x86/crypto/aesni-intel_glue.c | 406 +++++++++++++++++++-
  1 file changed, 404 insertions(+), 2 deletions(-)

arch/x86/crypto/aesni-intel_glue.c +404 -2

index ad8a7188a2bf..d90b03d9b420 100644
@@ -97,12 +97,13 @@ asmlinkage void aesni_cbc_dec(struct crypto_aes_ctx *ctx, u8 *out,
  #define AVX_GEN2_OPTSIZE 640
  #define AVX_GEN4_OPTSIZE 4096
 
+asmlinkage void aesni_ctr_enc(struct crypto_aes_ctx *ctx, u8 *out,
+			      const u8 *in, unsigned int len, u8 *iv);
+
  #ifdef CONFIG_X86_64
 
  static void (*aesni_ctr_enc_tfm)(struct crypto_aes_ctx *ctx, u8 *out,
  			      const u8 *in, unsigned int len, u8 *iv);
-asmlinkage void aesni_ctr_enc(struct crypto_aes_ctx *ctx, u8 *out,
-			      const u8 *in, unsigned int len, u8 *iv);
 
  asmlinkage void aesni_xts_crypt8(const struct crypto_aes_ctx *ctx, u8 *out,
  				 const u8 *in, bool enc, le128 *iv);

@@ -454,6 +455,377 @@ static int cbc_decrypt(struct skcipher_request *req)
  	return err;
  }
 
+static int aesni_ccm_setkey(struct crypto_aead *tfm, const u8 *in_key,
+			    unsigned int key_len)
+{
+	struct crypto_aes_ctx *ctx = crypto_aead_ctx(tfm);
+
+	return aes_set_key_common(crypto_aead_tfm(tfm), ctx, in_key, key_len);
+}
+
+static int aesni_ccm_setauthsize(struct crypto_aead *tfm, unsigned int authsize)
+{
+	if ((authsize & 1) || authsize < 4)
+		return -EINVAL;
+	return 0;
+}
+
+static int ccm_set_msg_len(u8 *block, unsigned int msglen, int csize)
+{
+	__be32 data;
+
+	memset(block, 0, csize);
+	block += csize;
+
+	if (csize >= 4)
+		csize = 4;
+	else if (msglen > (1 << (8 * csize)))
+		return -EOVERFLOW;
+
+	data = cpu_to_be32(msglen);
+	memcpy(block - csize, (u8 *)&data + 4 - csize, csize);
+
+	return 0;
+}
+
+static int ccm_init_mac(struct aead_request *req, u8 maciv[], u32 msglen)
+{
+	struct crypto_aead *aead = crypto_aead_reqtfm(req);
+	__be32 *n = (__be32 *)&maciv[AES_BLOCK_SIZE - 8];
+	u32 l = req->iv[0] + 1;
+
+	/* verify that CCM dimension 'L' is set correctly in the IV */
+	if (l < 2 || l > 8)
+		return -EINVAL;
+
+	/* verify that msglen can in fact be represented in L bytes */
+	if (l < 4 && msglen >> (8 * l))
+		return -EOVERFLOW;
+
+	/*
+	 * Even if the CCM spec allows L values of up to 8, the Linux cryptoapi
+	 * uses a u32 type to represent msglen so the top 4 bytes are always 0.
+	 */
+	n[0] = 0;
+	n[1] = cpu_to_be32(msglen);
+
+	memcpy(maciv, req->iv, AES_BLOCK_SIZE - l);
+
+	/*
+	 * Meaning of byte 0 according to CCM spec (RFC 3610/NIST 800-38C)
+	 * - bits 0..2	: max # of bytes required to represent msglen, minus 1
+	 *                (already set by caller)
+	 * - bits 3..5	: size of auth tag (1 => 4 bytes, 2 => 6 bytes, etc)
+	 * - bit 6	: indicates presence of authenticate-only data
+	 */
+	maciv[0] |= (crypto_aead_authsize(aead) - 2) << 2;
+	if (req->assoclen)
+		maciv[0] |= 0x40;
+
+	memset(&req->iv[AES_BLOCK_SIZE - l], 0, l);
+	return ccm_set_msg_len(maciv + AES_BLOCK_SIZE - l, msglen, l);
+}
+
+static int compute_mac(struct crypto_aes_ctx *ctx, u8 mac[], u8 *data, int n,
+		       unsigned int ilen, u8 *idata, bool do_simd)
+{
+	unsigned int bs = AES_BLOCK_SIZE;
+	u8 *odata = mac;
+	int datalen, getlen;
+
+	datalen = n;
+
+	/* first time in here, block may be partially filled. */
+	getlen = bs - ilen;
+	if (datalen >= getlen) {
+		memcpy(idata + ilen, data, getlen);
+
+		if (likely(do_simd)) {
+			aesni_cbc_enc(ctx, odata, idata, AES_BLOCK_SIZE, odata);
+		} else {
+			crypto_xor(odata, idata, AES_BLOCK_SIZE);
+			aes_encrypt(ctx, odata, odata);
+		}
+
+		datalen -= getlen;
+		data += getlen;
+		ilen = 0;
+	}
+
+	/* now encrypt rest of data */
+	while (datalen >= bs) {
+		if (likely(do_simd)) {
+			aesni_cbc_enc(ctx, odata, data, AES_BLOCK_SIZE, odata);
+		} else {
+			crypto_xor(odata, data, AES_BLOCK_SIZE);
+			aes_encrypt(ctx, odata, odata);
+		}
+
+		datalen -= bs;
+		data += bs;
+	}
+
+	/* check and see if there's leftover data that wasn't
+	 * enough to fill a block.
+	 */
+	if (datalen) {
+		memcpy(idata + ilen, data, datalen);
+		ilen += datalen;
+	}
+	return ilen;
+}
+
+static void ccm_calculate_auth_mac(struct aead_request *req,
+				   struct crypto_aes_ctx *ctx, u8 mac[],
+				   struct scatterlist *src,
+				   bool do_simd)
+{
+	unsigned int len = req->assoclen;
+	struct scatter_walk walk;
+	u8 idata[AES_BLOCK_SIZE];
+	unsigned int ilen;
+	struct {
+		__be16 l;
+		__be32 h;
+	} __packed *ltag = (void *)idata;
+
+	/* prepend the AAD with a length tag */
+	if (len < 0xff00) {
+		ltag->l = cpu_to_be16(len);
+		ilen = 2;
+	} else  {
+		ltag->l = cpu_to_be16(0xfffe);
+		ltag->h = cpu_to_be32(len);
+		ilen = 6;
+	}
+
+	scatterwalk_start(&walk, src);
+
+	while (len) {
+		u8 *src;
+		int n;
+
+		n = scatterwalk_clamp(&walk, len);
+		if (!n) {
+			scatterwalk_start(&walk, sg_next(walk.sg));
+			n = scatterwalk_clamp(&walk, len);
+		}
+		src = scatterwalk_map(&walk);
+
+		ilen = compute_mac(ctx, mac, src, n, ilen, idata, do_simd);
+		len -= n;
+
+		scatterwalk_unmap(src);
+		scatterwalk_advance(&walk, n);
+		scatterwalk_done(&walk, 0, len);
+	}
+
+	/* any leftover needs padding and then encrypted */
+	if (ilen) {
+		crypto_xor(mac, idata, ilen);
+		if (likely(do_simd))
+			aesni_enc(ctx, mac, mac);
+		else
+			aes_encrypt(ctx, mac, mac);
+	}
+}
+
+static int aesni_ccm_encrypt(struct aead_request *req)
+{
+	struct crypto_aead *aead = crypto_aead_reqtfm(req);
+	struct crypto_aes_ctx *ctx = aes_ctx(crypto_aead_ctx(aead));
+	bool const do_simd = crypto_simd_usable();
+	u8 __aligned(8) mac[AES_BLOCK_SIZE];
+	u8 __aligned(8) buf[AES_BLOCK_SIZE];
+	struct skcipher_walk walk;
+	u32 l = req->iv[0] + 1;
+	int err;
+
+	err = ccm_init_mac(req, mac, req->cryptlen);
+	if (err)
+		return err;
+
+	if (likely(do_simd)) {
+		kernel_fpu_begin();
+		aesni_enc(ctx, mac, mac);
+	} else {
+		aes_encrypt(ctx, mac, mac);
+	}
+
+	if (req->assoclen)
+		ccm_calculate_auth_mac(req, ctx, mac, req->src, do_simd);
+
+	req->iv[AES_BLOCK_SIZE - 1] = 0x1;
+	err = skcipher_walk_aead_encrypt(&walk, req, true);
+
+	while (walk.nbytes >= AES_BLOCK_SIZE) {
+		int len = walk.nbytes & AES_BLOCK_MASK;
+		int n;
+
+		for (n = 0; n < len; n += AES_BLOCK_SIZE) {
+			if (likely(do_simd)) {
+				aesni_cbc_enc(ctx, mac, walk.src.virt.addr + n,
+					      AES_BLOCK_SIZE, mac);
+			} else {
+				crypto_xor(mac, walk.src.virt.addr + n,
+					   AES_BLOCK_SIZE);
+				aes_encrypt(ctx, mac, mac);
+
+				aes_encrypt(ctx, buf, walk.iv);
+				crypto_inc(walk.iv, AES_BLOCK_SIZE);
+				crypto_xor_cpy(walk.dst.virt.addr + n,
+					       walk.src.virt.addr + n,
+					       buf, AES_BLOCK_SIZE);
+			}
+		}
+		if (likely(do_simd))
+			aesni_ctr_enc(ctx, walk.dst.virt.addr,
+				      walk.src.virt.addr, len, walk.iv);
+
+		err = skcipher_walk_done(&walk, walk.nbytes & ~AES_BLOCK_MASK);
+	}
+	if (walk.nbytes) {
+		if (likely(do_simd))
+			aesni_enc(ctx, buf, walk.iv);
+		else
+			aes_encrypt(ctx, buf, walk.iv);
+
+		crypto_xor(mac, walk.src.virt.addr, walk.nbytes);
+		crypto_xor_cpy(walk.dst.virt.addr, walk.src.virt.addr,
+			       buf, walk.nbytes);
+
+		if (likely(do_simd))
+			aesni_enc(ctx, mac, mac);
+		else
+			aes_encrypt(ctx, mac, mac);
+
+		err = skcipher_walk_done(&walk, 0);
+	}
+
+	if (err)
+		goto fail;
+
+	memset(walk.iv + AES_BLOCK_SIZE - l, 0, l);
+
+	if (likely(do_simd)) {
+		aesni_ctr_enc(ctx, mac, mac, AES_BLOCK_SIZE, walk.iv);
+	} else {
+		aes_encrypt(ctx, buf, walk.iv);
+		crypto_xor(mac, buf, AES_BLOCK_SIZE);
+	}
+
+	/* copy authtag to end of dst */
+	scatterwalk_map_and_copy(mac, req->dst, req->assoclen + req->cryptlen,
+				 crypto_aead_authsize(aead), 1);
+
+fail:
+	if (likely(do_simd))
+		kernel_fpu_end();
+	return err;
+}
+
+static int aesni_ccm_decrypt(struct aead_request *req)
+{
+	struct crypto_aead *aead = crypto_aead_reqtfm(req);
+	struct crypto_aes_ctx *ctx = aes_ctx(crypto_aead_ctx(aead));
+	unsigned int authsize = crypto_aead_authsize(aead);
+	bool const do_simd = crypto_simd_usable();
+	u8 __aligned(8) mac[AES_BLOCK_SIZE];
+	u8 __aligned(8) tag[AES_BLOCK_SIZE];
+	u8 __aligned(8) buf[AES_BLOCK_SIZE];
+	struct skcipher_walk walk;
+	u32 l = req->iv[0] + 1;
+	int err;
+
+	err = ccm_init_mac(req, mac, req->cryptlen - authsize);
+	if (err)
+		return err;
+
+	/* copy authtag from end of src */
+	scatterwalk_map_and_copy(tag, req->src,
+				 req->assoclen + req->cryptlen - authsize,
+				 authsize, 0);
+
+	if (likely(do_simd)) {
+		kernel_fpu_begin();
+		aesni_enc(ctx, mac, mac);
+	} else {
+		aes_encrypt(ctx, mac, mac);
+	}
+
+	if (req->assoclen)
+		ccm_calculate_auth_mac(req, ctx, mac, req->src, do_simd);
+
+	req->iv[AES_BLOCK_SIZE - 1] = 0x1;
+	err = skcipher_walk_aead_decrypt(&walk, req, true);
+
+	while (walk.nbytes >= AES_BLOCK_SIZE) {
+		int len = walk.nbytes & AES_BLOCK_MASK;
+		int n;
+
+		if (likely(do_simd))
+			aesni_ctr_enc(ctx, walk.dst.virt.addr,
+				      walk.src.virt.addr, len, walk.iv);
+
+		for (n = 0; n < len; n += AES_BLOCK_SIZE) {
+			if (likely(do_simd)) {
+				aesni_cbc_enc(ctx, mac, walk.dst.virt.addr + n,
+					      AES_BLOCK_SIZE, mac);
+			} else {
+				aes_encrypt(ctx, buf, walk.iv);
+				crypto_inc(walk.iv, AES_BLOCK_SIZE);
+				crypto_xor_cpy(walk.dst.virt.addr + n,
+					       walk.src.virt.addr + n,
+					       buf, AES_BLOCK_SIZE);
+
+				crypto_xor(mac, walk.dst.virt.addr + n,
+					   AES_BLOCK_SIZE);
+				aes_encrypt(ctx, mac, mac);
+			}
+		}
+
+		err = skcipher_walk_done(&walk, walk.nbytes & ~AES_BLOCK_MASK);
+	}
+	if (walk.nbytes) {
+		if (likely(do_simd))
+			aesni_enc(ctx, buf, walk.iv);
+		else
+			aes_encrypt(ctx, buf, walk.iv);
+
+		crypto_xor_cpy(walk.dst.virt.addr, walk.src.virt.addr,
+			       buf, walk.nbytes);
+		crypto_xor(mac, walk.dst.virt.addr, walk.nbytes);
+
+		if (likely(do_simd))
+			aesni_enc(ctx, mac, mac);
+		else
+			aes_encrypt(ctx, mac, mac);
+
+		err = skcipher_walk_done(&walk, 0);
+	}
+
+	if (err)
+		goto fail;
+
+	memset(walk.iv + AES_BLOCK_SIZE - l, 0, l);
+
+	if (likely(do_simd)) {
+		aesni_ctr_enc(ctx, mac, mac, AES_BLOCK_SIZE, walk.iv);
+	} else {
+		aes_encrypt(ctx, buf, walk.iv);
+		crypto_xor(mac, buf, AES_BLOCK_SIZE);
+	}
+
+	/* compare calculated auth tag with the stored one */
+	if (crypto_memneq(mac, tag, authsize))
+		err = -EBADMSG;
+
+fail:
+	if (likely(do_simd))
+		kernel_fpu_end();
+	return err;
+}
+
  #ifdef CONFIG_X86_64
  static void ctr_crypt_final(struct crypto_aes_ctx *ctx,
  			    struct skcipher_walk *walk)

@@ -1045,10 +1417,28 @@ static struct aead_alg aesni_aeads[] = { {
  		.cra_module		= THIS_MODULE,
  	},
  } };
+
  #else
  static struct aead_alg aesni_aeads[0];
  #endif
 
+static struct aead_alg aesni_ccm_aead = {
+	.base.cra_name		= "ccm(aes)",
+	.base.cra_driver_name	= "ccm-aesni",
+	.base.cra_priority	= 400,
+	.base.cra_blocksize	= 1,
+	.base.cra_ctxsize	= sizeof(struct crypto_aes_ctx),
+	.base.cra_module	= THIS_MODULE,
+
+	.setkey			= aesni_ccm_setkey,
+	.setauthsize		= aesni_ccm_setauthsize,
+	.encrypt		= aesni_ccm_encrypt,
+	.decrypt		= aesni_ccm_decrypt,
+	.ivsize			= AES_BLOCK_SIZE,
+	.chunksize		= AES_BLOCK_SIZE,
+	.maxauthsize		= AES_BLOCK_SIZE,
+};
+
  static struct simd_aead_alg *aesni_simd_aeads[ARRAY_SIZE(aesni_aeads)];
 
  static const struct x86_cpu_id aesni_cpu_id[] = {

@@ -1098,8 +1488,17 @@ static int __init aesni_init(void)
  	if (err)
  		goto unregister_skciphers;
 
+	if (IS_ENABLED(CONFIG_X86_64)) {
+		err = crypto_register_aead(&aesni_ccm_aead);
+		if (err)
+			goto unregister_aeads;
+	}
+
  	return 0;
 
+unregister_aeads:
+	simd_unregister_aeads(aesni_aeads, ARRAY_SIZE(aesni_aeads),
+			      aesni_simd_aeads);
  unregister_skciphers:
  	simd_unregister_skciphers(aesni_skciphers, ARRAY_SIZE(aesni_skciphers),
  				  aesni_simd_skciphers);

@@ -1110,6 +1509,9 @@ static int __init aesni_init(void)
 
  static void __exit aesni_exit(void)
  {
+	if (IS_ENABLED(CONFIG_X86_64))
+		crypto_unregister_aead(&aesni_ccm_aead);
+
  	simd_unregister_aeads(aesni_aeads, ARRAY_SIZE(aesni_aeads),
  			      aesni_simd_aeads);
  	simd_unregister_skciphers(aesni_skciphers, ARRAY_SIZE(aesni_skciphers),

Add ccm(aes) implementation from linux-wireless mailing list (see
http://permalink.gmane.org/gmane.linux.kernel.wireless.general/126679).

This eliminates FPU context store/restore overhead existing in more general ccm_base(ctr(aes-aesni),aes-aesni) case in MAC calculation.

Suggested-by Ben Greear <[email protected]>

Co-developed-by Steve deRosier <[email protected]>

Signed-off-by Steve deRosier <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
v2: avoid the SIMD helper, as it produces an CRYPTO_ALG_ASYNC aead, which
is not usable by the 802.11 ccmp driver

Quoted 6 lines · Ard Biesheuvel

Add ccm(aes) implementation from linux-wireless mailing list (see
http://permalink.gmane.org/gmane.linux.kernel.wireless.general/126679).

This eliminates FPU context store/restore overhead existing in more general ccm_base(ctr(aes-aesni),aes-aesni) case in MAC calculation.

Quoted 7 lines

Suggested-by Ben Greear <[email protected]>

Co-developed-by Steve deRosier <[email protected]>

Signed-off-by Steve deRosier <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
v2: avoid the SIMD helper, as it produces an CRYPTO_ALG_ASYNC aead, which
is not usable by the 802.11 ccmp driver

Sorry, but this is not the way to go. Please fix wireless to use the async interface instead.

No one wanted to do this for the last 6+ years, so I don't think it is likely to happen any time soon. If the patch is better than
existing behaviour, please let it into the kernel. And it is certainly better in my test case.

Quoted 6 lines · Ard Biesheuvel

Add ccm(aes) implementation from linux-wireless mailing list (see
http://permalink.gmane.org/gmane.linux.kernel.wireless.general/126679).

This eliminates FPU context store/restore overhead existing in more general ccm_base(ctr(aes-aesni),aes-aesni) case in MAC calculation.

Quoted 7 lines

Suggested-by Ben Greear <[email protected]>

Co-developed-by Steve deRosier <[email protected]>

Signed-off-by Steve deRosier <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
v2: avoid the SIMD helper, as it produces an CRYPTO_ALG_ASYNC aead, which
is not usable by the 802.11 ccmp driver

Sorry, but this is not the way to go. Please fix wireless to use the async interface instead.

This is not the first time this has come up. The point is that CCMP in the wireless stack is not used in 99% of the cases, given that any wifi hardware built in the last ~10 years can do it in hardware. Only in exceptional cases, such as Ben's, is there a need for exercising this interface.

Also, care to explain why we have synchronous AEADs in the first place if they are not supposed to be used?

Quoted 6 lines · Ard Biesheuvel

This is not the first time this has come up. The point is that CCMP in the wireless stack is not used in 99% of the cases, given that any wifi hardware built in the last ~10 years can do it in hardware. Only in exceptional cases, such as Ben's, is there a need for exercising this interface.

Either it matters or it doesn't. If it doesn't matter why are we having this dicussion at all? If it does then fixing just one direction makes no sense.

Quoted 2 lines

Also, care to explain why we have synchronous AEADs in the first place if they are not supposed to be used?

Sync AEADs would make sense if you were dealing with a very small amount of data, e.g., one block.

Quoted 6 lines · Ard Biesheuvel

This is not the first time this has come up. The point is that CCMP in the wireless stack is not used in 99% of the cases, given that any wifi hardware built in the last ~10 years can do it in hardware. Only in exceptional cases, such as Ben's, is there a need for exercising this interface.

Either it matters or it doesn't. If it doesn't matter why are we having this dicussion at all? If it does then fixing just one direction makes no sense.

Quoted 2 lines

Also, care to explain why we have synchronous AEADs in the first place if they are not supposed to be used?

Sync AEADs would make sense if you were dealing with a very small amount of data, e.g., one block.

What do you mean by just one direction? Ben just confirmed a

substantial speedup for his use case, which requires the use of software encryption even on hardware that could support doing it in hardware, and that software encryption currently only supports the synchronous interface.

Quoted 2 lines · Ard Biesheuvel

What do you mean by just one direction? Ben just confirmed a

The TX direction generally executes in process context, which would benefit from an accelerated sync implementation. The RX side on the other hand is always in softirq context.

Quoted 4 lines

substantial speedup for his use case, which requires the use of software encryption even on hardware that could support doing it in hardware, and that software encryption currently only supports the synchronous interface.

The problem is that the degradation would come at the worst time, when the system is loaded. IOW when you get an interrupt during your TX path and get RX traffic that's when you'll take the fallback path.

Quoted 5 lines

The problem is that the degradation would come at the worst time, when the system is loaded. IOW when you get an interrupt during your TX path and get RX traffic that's when you'll take the fallback path.

I can see how in the general case, this is something you would prefer to avoid. However, on SMP x86_64 systems that implement AES-NI (which runs at ~1 cycle per byte), I don't see this as a real problem for this driver.

What we could do is expose both versions, where the async version has a slightly higher priority, so that all users that do support the async interface will get it, and the wifi stack can use the sync interface instead.

Quoted 6 lines · Ard Biesheuvel

Quoted 5 lines

The problem is that the degradation would come at the worst time, when the system is loaded. IOW when you get an interrupt during your TX path and get RX traffic that's when you'll take the fallback path.

Quoted 4 lines

I can see how in the general case, this is something you would prefer to avoid. However, on SMP x86_64 systems that implement AES-NI (which runs at ~1 cycle per byte), I don't see this as a real problem for this driver.

AES-NI is 1 cycle per byte but the fallback is not.

Quoted 4 lines

What we could do is expose both versions, where the async version has a slightly higher priority, so that all users that do support the async interface will get it, and the wifi stack can use the sync interface instead.

No we've already tried this with IPsec and it doesn't work. That's why the async path exists in aesni.

Wireless is no different to IPsec in this respect.

True. But the fallback only gets executed if the scheduler is stupid enough to schedule the TX task onto the core that is overloaded doing RX softirqs. So in the general case, both TX and RX will be using AES-NI instructions (unless the CCMP is done in hardware which is the most common case by far)

Wireless is very different. Wifi uses a medium that is fundamentally shared, and so the load it can induce is bounded. There is no way a wifi interface is going to saturate a 64-bit AES-NI core doing CCMP in software.

Given the above, can't we be pragmatic here? This code addresses a niche use case, which is not affected by the general concerns regarding async crypto.

Quoted 6 lines · Ard Biesheuvel

True. But the fallback only gets executed if the scheduler is stupid enough to schedule the TX task onto the core that is overloaded doing RX softirqs. So in the general case, both TX and RX will be using AES-NI instructions (unless the CCMP is done in hardware which is the most common case by far)

I don't think this makes sense. TX is typically done in response to RX so the natural alignment is for it to be on the same CPU.

Quoted 4 lines

Wireless is very different. Wifi uses a medium that is fundamentally shared, and so the load it can induce is bounded. There is no way a wifi interface is going to saturate a 64-bit AES-NI core doing CCMP in software.

This sounds pretty tenuous. In any case, even if wireless itself doesn't get you, there could be loads running on top of it, for example, IPsec.

Quoted 3 lines

Given the above, can't we be pragmatic here? This code addresses a niche use case, which is not affected by the general concerns regarding async crypto.

We already have a framework for acceleration that works properly in aesni, I don't want to see us introduce another broken model within the same driver.

So either just leave the whole thing along or do it properly by making wireless async.

You just explained that TX typically runs in process context, whereas RX is handled in softirq context. So how exactly are these going to end up on the same core?

Yes, but IPsec will not use the synchronous interface.

Fair enough. But it is unfortunate that we cannot support Ben's use case without a lot of additional work that serves no purpose otherwise.

Quoted 4 lines · Ard Biesheuvel

You just explained that TX typically runs in process context, whereas RX is handled in softirq context. So how exactly are these going to end up on the same core?

When you receive a TCP packet via RX, this should wake up your user- space thread on the same CPU as otherwise you'll pay extra cost on pulling the data into memory again.

Quoted 1 line

Yes, but IPsec will not use the synchronous interface.

That doesn't matter when the underlying wireless code is using the sync interface. An async user is completely capable of making the aesni code-path unavailable to the sync user.

Quoted 3 lines

Fair enough. But it is unfortunate that we cannot support Ben's use case without a lot of additional work that serves no purpose otherwise.

To the contrary, I think to fully support Ben's use case you must use the async code path. Otherwise sure you'll see good numbers when you do simple benchmarks like netperf, but when you really load up the system it just falls apart.

Quoted 6 lines · Ard Biesheuvel

Quoted 5 lines

The problem is that the degradation would come at the worst time, when the system is loaded. IOW when you get an interrupt during your TX path and get RX traffic that's when you'll take the fallback path.

Quoted 4 lines

I can see how in the general case, this is something you would prefer to avoid. However, on SMP x86_64 systems that implement AES-NI (which runs at ~1 cycle per byte), I don't see this as a real problem for this driver.

AES-NI is 1 cycle per byte but the fallback is not.

Quoted 4 lines

What we could do is expose both versions, where the async version has a slightly higher priority, so that all users that do support the async interface will get it, and the wifi stack can use the sync interface instead.

No we've already tried this with IPsec and it doesn't work. That's why the async path exists in aesni.

Wireless is no different to IPsec in this respect.

Cheers,
--
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

One thing I realized just now is that in the current situation, all the synchronous skciphers already degrade like this.

I.e., in Ben's case, without the special ccm implementation, ccm(aes) will resolve to ccm(ctr(aesni),cbcmac(aesni)), which is instantiated as a sync skcipher using the ctr and ccm/cbcmac templates built on top of the AES-NI cipher (not skcipher). This cipher will also fall back to suboptimal scalar code if the SIMD is in use in process context.

Quoted 9 lines · Ard Biesheuvel

One thing I realized just now is that in the current situation, all the synchronous skciphers already degrade like this.

I.e., in Ben's case, without the special ccm implementation, ccm(aes) will resolve to ccm(ctr(aesni),cbcmac(aesni)), which is instantiated as a sync skcipher using the ctr and ccm/cbcmac templates built on top of the AES-NI cipher (not skcipher). This cipher will also fall back to suboptimal scalar code if the SIMD is in use in process context.

Sure, your patch is not making it any worse. But I don't think the extra code is worth it considering that you're still going to be running into that slow fallback path all the time.

Much better to fix the wireless code to actually go async.

Quoted 11 lines · Herbert Xu

Quoted 9 lines · Ard Biesheuvel

One thing I realized just now is that in the current situation, all the synchronous skciphers already degrade like this.

I.e., in Ben's case, without the special ccm implementation, ccm(aes) will resolve to ccm(ctr(aesni),cbcmac(aesni)), which is instantiated as a sync skcipher using the ctr and ccm/cbcmac templates built on top of the AES-NI cipher (not skcipher). This cipher will also fall back to suboptimal scalar code if the SIMD is in use in process context.

Quoted 3 lines

Sure, your patch is not making it any worse. But I don't think the extra code is worth it considering that you're still going to be running into that slow fallback path all the time.

How can we test this assumption? I see 3x performance gain, so it is not hitting the fallback path much in my case. What traffic pattern and protocol do you think will cause the slow fallback path to happen often enough to make this patch not helpful?

Quoted 1 line

Much better to fix the wireless code to actually go async.

This will not happen any time soon, so better to make incremental improvement in the crypt code.

I would argue that these are orthogonal. My patch improves both the accelerated and the fallback path, given that the latter does not have to walk the input data twice anymore, and go through 3 layers of templates and the associated indirect calls for each 16 bytes of input.

Quoted 6 lines · Ard Biesheuvel

I would argue that these are orthogonal. My patch improves both the accelerated and the fallback path, given that the latter does not have to walk the input data twice anymore, and go through 3 layers of templates and the associated indirect calls for each 16 bytes of input.

As I told your before, your patch introduces a new model into aesni that is different to every other algorithm there for the sole purpose of catering for legacy hardware in a subsystem that refuses to do the right thing.

That is not acceptable.

But we should probably start policing this a bit more. For instance, we now have

drivers/net/macsec.c:

/* Pick a sync gcm(aes) cipher to ensure order is preserved. */ tfm = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);

(btw the comment is bogus, right?)

TLS_SW does the same thing in net/tls/tls_device_fallback.c.

Async is obviously needed for h/w accelerators, but could we perhaps do better for s/w SIMD algorithms? Those are by far the most widely used ones.

Quoted 11 lines · Ard Biesheuvel

But we should probably start policing this a bit more. For instance, we now have

drivers/net/macsec.c:

/* Pick a sync gcm(aes) cipher to ensure order is preserved. */ tfm = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);

(btw the comment is bogus, right?)

TLS_SW does the same thing in net/tls/tls_device_fallback.c.

Short of us volunteering to write code for every user out there I don't see a way out.

Quoted 3 lines

Async is obviously needed for h/w accelerators, but could we perhaps do better for s/w SIMD algorithms? Those are by far the most widely used ones.

If you can come up with a way that avoids the cryptd model without using a fallback obviously that would be the ultimate solution.

Quoted 21 lines · Herbert Xu

Quoted 11 lines · Ard Biesheuvel

But we should probably start policing this a bit more. For instance, we now have

drivers/net/macsec.c:

/* Pick a sync gcm(aes) cipher to ensure order is preserved. */ tfm = crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC);

(btw the comment is bogus, right?)

TLS_SW does the same thing in net/tls/tls_device_fallback.c.

Short of us volunteering to write code for every user out there I don't see a way out.

Quoted 3 lines

Async is obviously needed for h/w accelerators, but could we perhaps do better for s/w SIMD algorithms? Those are by far the most widely used ones.

Quoted 3 lines

If you can come up with a way that avoids the cryptd model without using a fallback obviously that would be the ultimate solution.

Could we disable softirq handling in these regions?

So the question is then how granular these kernel mode SIMD regions need to be to avoid excessive latencies in softirq handling.

Quoted 3 lines · Ard Biesheuvel

So the question is then how granular these kernel mode SIMD regions need to be to avoid excessive latencies in softirq handling.

Can you get some real world numbers on what the latency is like?

Then we could take it to the scheduler folks and see if they're OK with it.

Thanks,

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Quoted 8 lines · Herbert Xu

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Indeed. Those are just add/add conflicts because both patches introduce new code into the same set of files. The resolution is generally to keep both sides.

As for Herbert's objection: I will note here that in the meantime, arm64 now has gotten rid of the scalar fallbacks entirely in AEAD and skipcher implementations, because those are only callable in task or softirq context, and the arm64 SIMD wrappers now disable softirq processing. This means that the condition that results in the fallback being needed can no longer occur, making the SIMD helper dead code on arm64.

I suppose we might do the same thing on x86, but since the kernel mode SIMD handling is highly arch specific, you'd really need to raise this with the x86 maintainers.

Quoted 8 lines · Herbert Xu

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Indeed. Those are just add/add conflicts because both patches introduce new code into the same set of files. The resolution is generally to keep both sides.

As for Herbert's objection: I will note here that in the meantime, arm64 now has gotten rid of the scalar fallbacks entirely in AEAD and skipcher implementations, because those are only callable in task or softirq context, and the arm64 SIMD wrappers now disable softirq processing. This means that the condition that results in the fallback being needed can no longer occur, making the SIMD helper dead code on arm64.

I suppose we might do the same thing on x86, but since the kernel mode SIMD handling is highly arch specific, you'd really need to raise this with the x86 maintainers.

Quoted 27 lines · Ard Biesheuvel

Quoted 8 lines · Herbert Xu

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Indeed. Those are just add/add conflicts because both patches introduce new code into the same set of files. The resolution is generally to keep both sides.

As for Herbert's objection: I will note here that in the meantime, arm64 now has gotten rid of the scalar fallbacks entirely in AEAD and skipcher implementations, because those are only callable in task or softirq context, and the arm64 SIMD wrappers now disable softirq processing. This means that the condition that results in the fallback being needed can no longer occur, making the SIMD helper dead code on arm64.

Quoted 4 lines

I suppose we might do the same thing on x86, but since the kernel mode SIMD handling is highly arch specific, you'd really need to raise this with the x86 maintainers.

Hello Ard,

Could you please review the attached patch to make sure I merged it properly? My concern is the cleanup section and/or some problems I might have introduced related to the similarly named code that was added upstream.

Quoted 34 lines · Ben Greear

Quoted 27 lines · Ard Biesheuvel

Quoted 8 lines · Herbert Xu

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Indeed. Those are just add/add conflicts because both patches introduce new code into the same set of files. The resolution is generally to keep both sides.

As for Herbert's objection: I will note here that in the meantime, arm64 now has gotten rid of the scalar fallbacks entirely in AEAD and skipcher implementations, because those are only callable in task or softirq context, and the arm64 SIMD wrappers now disable softirq processing. This means that the condition that results in the fallback being needed can no longer occur, making the SIMD helper dead code on arm64.

Quoted 4 lines

I suppose we might do the same thing on x86, but since the kernel mode SIMD handling is highly arch specific, you'd really need to raise this with the x86 maintainers.

Quoted 6 lines

Hello Ard,

Could you please review the attached patch to make sure I merged it properly? My concern is the cleanup section and/or some problems I might have introduced related to the similarly named code that was added upstream.

I don't think the logic is quite right, although it rarely matter.

I've pushed my version here - it invokes the static call for CTR so it will use the faster AVX version if the CPU supports it.

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=aesni-ccm-v6.1

Quoted 42 lines · Ard Biesheuvel

Quoted 34 lines · Ben Greear

Quoted 27 lines · Ard Biesheuvel

Quoted 8 lines · Herbert Xu

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Indeed. Those are just add/add conflicts because both patches introduce new code into the same set of files. The resolution is generally to keep both sides.

As for Herbert's objection: I will note here that in the meantime, arm64 now has gotten rid of the scalar fallbacks entirely in AEAD and skipcher implementations, because those are only callable in task or softirq context, and the arm64 SIMD wrappers now disable softirq processing. This means that the condition that results in the fallback being needed can no longer occur, making the SIMD helper dead code on arm64.

Quoted 4 lines

I suppose we might do the same thing on x86, but since the kernel mode SIMD handling is highly arch specific, you'd really need to raise this with the x86 maintainers.

Quoted 6 lines

Hello Ard,

Could you please review the attached patch to make sure I merged it properly? My concern is the cleanup section and/or some problems I might have introduced related to the similarly named code that was added upstream.

Quoted 6 lines

I don't think the logic is quite right, although it rarely matter.

I've pushed my version here - it invokes the static call for CTR so it will use the faster AVX version if the CPU supports it.

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=aesni-ccm-v6.1

Hello Ard,

It looks like something changed again in the intel-aesni logic for 6.6 kernel. I was able to do a small change to the patch to get it to compile, but the kernel crashes when I bring up a wlan port in 6.6. When I remove the aesni patch, the station comes up without crashing. The aesni patch worked fine in 6.5 as far as I can tell.

I'm attaching my slightly modified version of the patch you sent previous. If you have time to investigate this it would be much appreciated.

Thanks,
Ben

Quoted 57 lines · Ben Greear

Quoted 42 lines · Ard Biesheuvel

Quoted 34 lines · Ben Greear

Quoted 27 lines · Ard Biesheuvel

Quoted 8 lines · Herbert Xu

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Indeed. Those are just add/add conflicts because both patches introduce new code into the same set of files. The resolution is generally to keep both sides.

As for Herbert's objection: I will note here that in the meantime, arm64 now has gotten rid of the scalar fallbacks entirely in AEAD and skipcher implementations, because those are only callable in task or softirq context, and the arm64 SIMD wrappers now disable softirq processing. This means that the condition that results in the fallback being needed can no longer occur, making the SIMD helper dead code on arm64.

Quoted 4 lines

I suppose we might do the same thing on x86, but since the kernel mode SIMD handling is highly arch specific, you'd really need to raise this with the x86 maintainers.

Quoted 6 lines

Hello Ard,

Could you please review the attached patch to make sure I merged it properly? My concern is the cleanup section and/or some problems I might have introduced related to the similarly named code that was added upstream.

Quoted 6 lines

I don't think the logic is quite right, although it rarely matter.

I've pushed my version here - it invokes the static call for CTR so it will use the faster AVX version if the CPU supports it.

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=aesni-ccm-v6.1

Hello Ard,

It looks like something changed again in the intel-aesni logic for 6.6 kernel. I was able to do a small change to the patch to get it to compile, but the kernel crashes when I bring up a wlan port in 6.6. When I remove the aesni patch, the station comes up without crashing. The aesni patch worked fine in 6.5 as far as I can tell.

Quoted 5 lines

I'm attaching my slightly modified version of the patch you sent previous. If you have time to investigate this it would be much appreciated.

Thanks,
Ben

If this patch is useful, shouldn't it be upstreamed?

It was rejected by Herbert on the basis that the wireless stack should be converted to use the async API instead.

Quoted 62 lines · Eric Biggers

Quoted 60 lines · Ben Greear

Quoted 42 lines · Ard Biesheuvel

Quoted 34 lines · Ben Greear

Quoted 27 lines · Ard Biesheuvel

Quoted 8 lines · Herbert Xu

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Indeed. Those are just add/add conflicts because both patches introduce new code into the same set of files. The resolution is generally to keep both sides.

As for Herbert's objection: I will note here that in the meantime, arm64 now has gotten rid of the scalar fallbacks entirely in AEAD and skipcher implementations, because those are only callable in task or softirq context, and the arm64 SIMD wrappers now disable softirq processing. This means that the condition that results in the fallback being needed can no longer occur, making the SIMD helper dead code on arm64.

Quoted 4 lines

I suppose we might do the same thing on x86, but since the kernel mode SIMD handling is highly arch specific, you'd really need to raise this with the x86 maintainers.

Quoted 6 lines

Hello Ard,

Could you please review the attached patch to make sure I merged it properly? My concern is the cleanup section and/or some problems I might have introduced related to the similarly named code that was added upstream.

Quoted 6 lines

I don't think the logic is quite right, although it rarely matter.

I've pushed my version here - it invokes the static call for CTR so it will use the faster AVX version if the CPU supports it.

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=aesni-ccm-v6.1

Hello Ard,

It looks like something changed again in the intel-aesni logic for 6.6 kernel. I was able to do a small change to the patch to get it to compile, but the kernel crashes when I bring up a wlan port in 6.6. When I remove the aesni patch, the station comes up without crashing. The aesni patch worked fine in 6.5 as far as I can tell.

I'm attaching my slightly modified version of the patch you sent previous. If you have time to investigate this it would be much appreciated.

Quoted 5 lines

Quoted 2 lines

Thanks,
Ben

If this patch is useful, shouldn't it be upstreamed?

It was rejected by Herbert on the basis that the wireless stack should be converted to use the async API instead.

Quoted 69 lines · Ard Biesheuvel

Quoted 67 lines · Eric Biggers

Quoted 57 lines · Ben Greear

Quoted 42 lines · Ard Biesheuvel

Quoted 34 lines · Ben Greear

Quoted 27 lines · Ard Biesheuvel

Quoted 8 lines · Herbert Xu

Quoted 5 lines · Ben Greear

While rebasing my patches onto 6.1-rc4, I noticed my aesni for ccm(aes) patch didn't apply cleanly, and I found this patch described below is applied now. Does this upstream patch mean that aesni is already supported upstream now? Or is it specific to whatever xctr is? If so,
any chance the patch is wanted upstream now?

Quoted 5 lines

AFAICS the xctr patch has nothing to do with what you were trying to achieve with wireless. My objection still stands with regards to wireless, we should patch wireless to use the async crypto interface and not hack around it in the Crypto API.

Indeed. Those are just add/add conflicts because both patches introduce new code into the same set of files. The resolution is generally to keep both sides.

As for Herbert's objection: I will note here that in the meantime, arm64 now has gotten rid of the scalar fallbacks entirely in AEAD and skipcher implementations, because those are only callable in task or softirq context, and the arm64 SIMD wrappers now disable softirq processing. This means that the condition that results in the fallback being needed can no longer occur, making the SIMD helper dead code on arm64.

Quoted 4 lines

I suppose we might do the same thing on x86, but since the kernel mode SIMD handling is highly arch specific, you'd really need to raise this with the x86 maintainers.

Quoted 6 lines

Hello Ard,

Could you please review the attached patch to make sure I merged it properly? My concern is the cleanup section and/or some problems I might have introduced related to the similarly named code that was added upstream.

Quoted 6 lines

I don't think the logic is quite right, although it rarely matter.

I've pushed my version here - it invokes the static call for CTR so it will use the faster AVX version if the CPU supports it.

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=aesni-ccm-v6.1

Hello Ard,

It looks like something changed again in the intel-aesni logic for 6.6 kernel. I was able to do a small change to the patch to get it to compile, but the kernel crashes when I bring up a wlan port in 6.6. When I remove the aesni patch, the station comes up without crashing. The aesni patch worked fine in 6.5 as far as I can tell.

Quoted 5 lines

I'm attaching my slightly modified version of the patch you sent previous. If you have time to investigate this it would be much appreciated.

Thanks,
Ben

If this patch is useful, shouldn't it be upstreamed?

Quoted 2 lines

It was rejected by Herbert on the basis that the wireless stack should be converted to use the async API instead.

Hello,

I'm still dragging this patch along (see attached). I notice that there was a big re-write of the aesni logic in 6.11. Anyone know if this patch would need to be (or should be) modified to work well with the new upstream code?

Quoted 13 lines · Ben Greear

Quoted 11 lines · Herbert Xu

Quoted 9 lines · Ard Biesheuvel

One thing I realized just now is that in the current situation, all the synchronous skciphers already degrade like this.

I.e., in Ben's case, without the special ccm implementation, ccm(aes) will resolve to ccm(ctr(aesni),cbcmac(aesni)), which is instantiated as a sync skcipher using the ctr and ccm/cbcmac templates built on top of the AES-NI cipher (not skcipher). This cipher will also fall back to suboptimal scalar code if the SIMD is in use in process context.

Quoted 9 lines

Quoted 3 lines

Sure, your patch is not making it any worse. But I don't think the extra code is worth it considering that you're still going to be running into that slow fallback path all the time.

How can we test this assumption? I see 3x performance gain, so it is not hitting the fallback path much in my case. What traffic pattern and protocol do you think will cause the slow fallback path to happen often enough to make this patch not helpful?

Is there a way to verify Herbert's assertion that TX and RX tend to be handled by the same core? I am not a networking guy, but that seems dubious to me.

You could add a pr_warn_ratelimited() inside the fallback path and see if it ever gets called at all under various loads.