| Message ID | 20240118191811.50271-1-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3222bc997a24821ea4f96d1a9108dafeadc00cfb |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2,1/2] Revert "net: macsec: use skb_ensure_writable_head_tail to expand the skb" | expand |
2024-01-18, 11:18:06 -0800, Rahul Rameshbabu wrote: > This reverts commit b34ab3527b9622ca4910df24ff5beed5aa66c6b5. > > Using skb_ensure_writable_head_tail without a call to skb_unshare causes > the MACsec stack to operate on the original skb rather than a copy in the > macsec_encrypt path. This causes the buffer to be exceeded in space, and > leads to warnings generated by skb_put operations. Opting to revert this > change since skb_copy_expand is more efficient than > skb_ensure_writable_head_tail followed by a call to skb_unshare. Paolo, are you ok with this commit message? I agree it's a bit confusing but I can't think of anything clearer :( Other than those details on the commit message (the stack trace could also have been trimmed a bit), a revert sounds good to me.
On Sun, 2024-01-21 at 10:32 +0100, Sabrina Dubroca wrote: > 2024-01-18, 11:18:06 -0800, Rahul Rameshbabu wrote: > > This reverts commit b34ab3527b9622ca4910df24ff5beed5aa66c6b5. > > > > Using skb_ensure_writable_head_tail without a call to skb_unshare causes > > the MACsec stack to operate on the original skb rather than a copy in the > > macsec_encrypt path. This causes the buffer to be exceeded in space, and > > leads to warnings generated by skb_put operations. Opting to revert this > > change since skb_copy_expand is more efficient than > > skb_ensure_writable_head_tail followed by a call to skb_unshare. > > Paolo, are you ok with this commit message? I agree it's a bit > confusing but I can't think of anything clearer :( Yes, I re-read the relevant code and now the fix is clearer to me, thanks! I understand the intention is to drop patch 2/2. Could you please confirm that? If so, I can apply 1/2 without a repost. Thanks, Paolo
On Tue, 23 Jan, 2024 10:19:21 +0100 Paolo Abeni <pabeni@redhat.com> wrote: > On Sun, 2024-01-21 at 10:32 +0100, Sabrina Dubroca wrote: >> 2024-01-18, 11:18:06 -0800, Rahul Rameshbabu wrote: >> > This reverts commit b34ab3527b9622ca4910df24ff5beed5aa66c6b5. >> > >> > Using skb_ensure_writable_head_tail without a call to skb_unshare causes >> > the MACsec stack to operate on the original skb rather than a copy in the >> > macsec_encrypt path. This causes the buffer to be exceeded in space, and >> > leads to warnings generated by skb_put operations. Opting to revert this >> > change since skb_copy_expand is more efficient than >> > skb_ensure_writable_head_tail followed by a call to skb_unshare. >> >> Paolo, are you ok with this commit message? I agree it's a bit >> confusing but I can't think of anything clearer :( > > Yes, I re-read the relevant code and now the fix is clearer to me, > thanks! I tried thinking of how to rephrase that description a couple of times and could not come up with anything better. > > I understand the intention is to drop patch 2/2. > > Could you please confirm that? If so, I can apply 1/2 without a repost. Yes, the intention is to drop patch 2/2. It's a "defensive" patch that does not actually fix any issues from what I can tell. -- Thanks, Rahul Rameshbabu
Hello: This series was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 18 Jan 2024 11:18:06 -0800 you wrote: > This reverts commit b34ab3527b9622ca4910df24ff5beed5aa66c6b5. > > Using skb_ensure_writable_head_tail without a call to skb_unshare causes > the MACsec stack to operate on the original skb rather than a copy in the > macsec_encrypt path. This causes the buffer to be exceeded in space, and > leads to warnings generated by skb_put operations. Opting to revert this > change since skb_copy_expand is more efficient than > skb_ensure_writable_head_tail followed by a call to skb_unshare. > > [...] Here is the summary with links: - [net,v2,1/2] Revert "net: macsec: use skb_ensure_writable_head_tail to expand the skb" https://git.kernel.org/netdev/net/c/3222bc997a24 - [net,v2,2/2] net: macsec: Only require headroom/tailroom from offload implementer if .mdo_insert_tx_tag is implemented (no matching commit) You are awesome, thank you!
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index e34816638569..7f5426285c61 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -607,11 +607,26 @@ static struct sk_buff *macsec_encrypt(struct sk_buff *skb, return ERR_PTR(-EINVAL); } - ret = skb_ensure_writable_head_tail(skb, dev); - if (unlikely(ret < 0)) { - macsec_txsa_put(tx_sa); - kfree_skb(skb); - return ERR_PTR(ret); + if (unlikely(skb_headroom(skb) < MACSEC_NEEDED_HEADROOM || + skb_tailroom(skb) < MACSEC_NEEDED_TAILROOM)) { + struct sk_buff *nskb = skb_copy_expand(skb, + MACSEC_NEEDED_HEADROOM, + MACSEC_NEEDED_TAILROOM, + GFP_ATOMIC); + if (likely(nskb)) { + consume_skb(skb); + skb = nskb; + } else { + macsec_txsa_put(tx_sa); + kfree_skb(skb); + return ERR_PTR(-ENOMEM); + } + } else { + skb = skb_unshare(skb, GFP_ATOMIC); + if (!skb) { + macsec_txsa_put(tx_sa); + return ERR_PTR(-ENOMEM); + } } unprotected_len = skb->len;
This reverts commit b34ab3527b9622ca4910df24ff5beed5aa66c6b5. Using skb_ensure_writable_head_tail without a call to skb_unshare causes the MACsec stack to operate on the original skb rather than a copy in the macsec_encrypt path. This causes the buffer to be exceeded in space, and leads to warnings generated by skb_put operations. Opting to revert this change since skb_copy_expand is more efficient than skb_ensure_writable_head_tail followed by a call to skb_unshare. Log: ------------[ cut here ]------------ kernel BUG at net/core/skbuff.c:2464! invalid opcode: 0000 [#1] SMP KASAN CPU: 21 PID: 61997 Comm: iperf3 Not tainted 6.7.0-rc8_for_upstream_debug_2024_01_07_17_05 #1 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014 RIP: 0010:skb_put+0x113/0x190 Code: 03 0f b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 04 84 d2 75 70 3b 9d bc 00 00 00 77 0e 48 83 c4 08 4c 89 e8 5b 5d 41 5d c3 <0f> 0b 4c 8b 6c 24 20 89 74 24 04 e8 6d b7 f0 fe 8b 74 24 04 48 c7 RSP: 0018:ffff8882694e7278 EFLAGS: 00010202 RAX: 0000000000000025 RBX: 0000000000000100 RCX: 0000000000000001 RDX: 0000000000000000 RSI: 0000000000000010 RDI: ffff88816ae0bad4 RBP: ffff88816ae0ba60 R08: 0000000000000004 R09: 0000000000000004 R10: 0000000000000001 R11: 0000000000000001 R12: ffff88811ba5abfa R13: ffff8882bdecc100 R14: ffff88816ae0ba60 R15: ffff8882bdecc0ae FS: 00007fe54df02740(0000) GS:ffff88881f080000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fe54d92e320 CR3: 000000010a345003 CR4: 0000000000370eb0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> ? die+0x33/0x90 ? skb_put+0x113/0x190 ? do_trap+0x1b4/0x3b0 ? skb_put+0x113/0x190 ? do_error_trap+0xb6/0x180 ? skb_put+0x113/0x190 ? handle_invalid_op+0x2c/0x30 ? skb_put+0x113/0x190 ? exc_invalid_op+0x2b/0x40 ? asm_exc_invalid_op+0x16/0x20 ? skb_put+0x113/0x190 ? macsec_start_xmit+0x4e9/0x21d0 macsec_start_xmit+0x830/0x21d0 ? get_txsa_from_nl+0x400/0x400 ? lock_downgrade+0x690/0x690 ? dev_queue_xmit_nit+0x78b/0xae0 dev_hard_start_xmit+0x151/0x560 __dev_queue_xmit+0x1580/0x28f0 ? check_chain_key+0x1c5/0x490 ? netdev_core_pick_tx+0x2d0/0x2d0 ? __ip_queue_xmit+0x798/0x1e00 ? lock_downgrade+0x690/0x690 ? mark_held_locks+0x9f/0xe0 ip_finish_output2+0x11e4/0x2050 ? ip_mc_finish_output+0x520/0x520 ? ip_fragment.constprop.0+0x230/0x230 ? __ip_queue_xmit+0x798/0x1e00 __ip_queue_xmit+0x798/0x1e00 ? __skb_clone+0x57a/0x760 __tcp_transmit_skb+0x169d/0x3490 ? lock_downgrade+0x690/0x690 ? __tcp_select_window+0x1320/0x1320 ? mark_held_locks+0x9f/0xe0 ? lockdep_hardirqs_on_prepare+0x286/0x400 ? tcp_small_queue_check.isra.0+0x120/0x3d0 tcp_write_xmit+0x12b6/0x7100 ? skb_page_frag_refill+0x1e8/0x460 __tcp_push_pending_frames+0x92/0x320 tcp_sendmsg_locked+0x1ed4/0x3190 ? tcp_sendmsg_fastopen+0x650/0x650 ? tcp_sendmsg+0x1a/0x40 ? mark_held_locks+0x9f/0xe0 ? lockdep_hardirqs_on_prepare+0x286/0x400 tcp_sendmsg+0x28/0x40 ? inet_send_prepare+0x1b0/0x1b0 __sock_sendmsg+0xc5/0x190 sock_write_iter+0x222/0x380 ? __sock_sendmsg+0x190/0x190 ? kfree+0x96/0x130 vfs_write+0x842/0xbd0 ? kernel_write+0x530/0x530 ? __fget_light+0x51/0x220 ? __fget_light+0x51/0x220 ksys_write+0x172/0x1d0 ? update_socket_protocol+0x10/0x10 ? __x64_sys_read+0xb0/0xb0 ? lockdep_hardirqs_on_prepare+0x286/0x400 do_syscall_64+0x40/0xe0 entry_SYSCALL_64_after_hwframe+0x46/0x4e RIP: 0033:0x7fe54d9018b7 Code: 0f 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24 RSP: 002b:00007ffdbd4191d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 RAX: ffffffffffffffda RBX: 0000000000000025 RCX: 00007fe54d9018b7 RDX: 0000000000000025 RSI: 0000000000d9859c RDI: 0000000000000004 RBP: 0000000000d9859c R08: 0000000000000004 R09: 0000000000000000 R10: 00007fe54d80afe0 R11: 0000000000000246 R12: 0000000000000004 R13: 0000000000000025 R14: 00007fe54e00ec00 R15: 0000000000d982a0 </TASK> Modules linked in: 8021q garp mrp iptable_raw bonding vfio_pci rdma_ucm ib_umad mlx5_vfio_pci mlx5_ib vfio_pci_core vfio_iommu_type1 ib_uverbs vfio mlx5_core ip_gre nf_tables ipip tunnel4 ib_ipoib ip6_gre gre ip6_tunnel tunnel6 geneve openvswitch nsh xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xt_addrtype iptable_nat nf_nat br_netfilter rpcsec_gss_krb5 auth_rpcgss oid_registry overlay rpcrdma ib_iser libiscsi scsi_transport_iscsi rdma_cm iw_cm ib_cm ib_core zram zsmalloc fuse [last unloaded: ib_uverbs] ---[ end trace 0000000000000000 ]--- Cc: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com> Cc: David S. Miller <davem@davemloft.net> Cc: Sabrina Dubroca <sd@queasysnail.net> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> --- Notes: Testing: This revert is needed even if commit a73d8779d61a ("net: macsec: introduce mdo_insert_tx_tag") is fully reverted as well, meaning resolving the headroom/tailroom issues in that commit is not enough and a fresh sk_buff is needed. drivers/net/macsec.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)