On Tue, Nov 26, 2024 at 11:50:17AM +0800, Jason Wang wrote:
On Tue, Nov 26, 2024 at 10:42 AM Koichiro Den koichiro.den@canonical.com wrote:
When virtnet_close is followed by virtnet_open, there is a slight chance that some TX completions remain unconsumed. Those are handled during the first NAPI poll, but since dql_reset occurs just beforehand, it can lead to a crash [1].
This issue can be reproduced by running: `while :; do ip l set DEV down; ip l set DEV up; done` under heavy network TX load from inside of the machine.
To fix this, drain unconsumed TX completions if any before dql_reset, allowing BQL to start cleanly.
------------[ cut here ]------------ kernel BUG at lib/dynamic_queue_limits.c:99! Oops: invalid opcode: 0000 [#1] PREEMPT SMP NOPTI CPU: 7 UID: 0 PID: 1598 Comm: ip Tainted: G N 6.12.0net-next_main+ #2 Tainted: [N]=TEST Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), \ BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014 RIP: 0010:dql_completed+0x26b/0x290 Code: b7 c2 49 89 e9 44 89 da 89 c6 4c 89 d7 e8 ed 17 47 00 58 65 ff 0d 4d 27 90 7e 0f 85 fd fe ff ff e8 ea 53 8d ff e9 f3 fe ff ff <0f> 0b 01 d2 44 89 d1 29 d1 ba 00 00 00 00 0f 48 ca e9 28 ff ff ff RSP: 0018:ffffc900002b0d08 EFLAGS: 00010297 RAX: 0000000000000000 RBX: ffff888102398c80 RCX: 0000000080190009 RDX: 0000000000000000 RSI: 000000000000006a RDI: 0000000000000000 RBP: ffff888102398c00 R08: 0000000000000000 R09: 0000000000000000 R10: 00000000000000ca R11: 0000000000015681 R12: 0000000000000001 R13: ffffc900002b0d68 R14: ffff88811115e000 R15: ffff8881107aca40 FS: 00007f41ded69500(0000) GS:ffff888667dc0000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000556ccc2dc1a0 CR3: 0000000104fd8003 CR4: 0000000000772ef0 PKRU: 55555554 Call Trace:
<IRQ> ? die+0x32/0x80 ? do_trap+0xd9/0x100 ? dql_completed+0x26b/0x290 ? dql_completed+0x26b/0x290 ? do_error_trap+0x6d/0xb0 ? dql_completed+0x26b/0x290 ? exc_invalid_op+0x4c/0x60 ? dql_completed+0x26b/0x290 ? asm_exc_invalid_op+0x16/0x20 ? dql_completed+0x26b/0x290 __free_old_xmit+0xff/0x170 [virtio_net] free_old_xmit+0x54/0xc0 [virtio_net] virtnet_poll+0xf4/0xe30 [virtio_net] ? __update_load_avg_cfs_rq+0x264/0x2d0 ? update_curr+0x35/0x260 ? reweight_entity+0x1be/0x260 __napi_poll.constprop.0+0x28/0x1c0 net_rx_action+0x329/0x420 ? enqueue_hrtimer+0x35/0x90 ? trace_hardirqs_on+0x1d/0x80 ? kvm_sched_clock_read+0xd/0x20 ? sched_clock+0xc/0x30 ? kvm_sched_clock_read+0xd/0x20 ? sched_clock+0xc/0x30 ? sched_clock_cpu+0xd/0x1a0 handle_softirqs+0x138/0x3e0 do_softirq.part.0+0x89/0xc0 </IRQ> <TASK> __local_bh_enable_ip+0xa7/0xb0 virtnet_open+0xc8/0x310 [virtio_net] __dev_open+0xfa/0x1b0 __dev_change_flags+0x1de/0x250 dev_change_flags+0x22/0x60 do_setlink.isra.0+0x2df/0x10b0 ? rtnetlink_rcv_msg+0x34f/0x3f0 ? netlink_rcv_skb+0x54/0x100 ? netlink_unicast+0x23e/0x390 ? netlink_sendmsg+0x21e/0x490 ? ____sys_sendmsg+0x31b/0x350 ? avc_has_perm_noaudit+0x67/0xf0 ? cred_has_capability.isra.0+0x75/0x110 ? __nla_validate_parse+0x5f/0xee0 ? __pfx___probestub_irq_enable+0x3/0x10 ? __create_object+0x5e/0x90 ? security_capable+0x3b/0x70 rtnl_newlink+0x784/0xaf0 ? avc_has_perm_noaudit+0x67/0xf0 ? cred_has_capability.isra.0+0x75/0x110 ? stack_depot_save_flags+0x24/0x6d0 ? __pfx_rtnl_newlink+0x10/0x10 rtnetlink_rcv_msg+0x34f/0x3f0 ? do_syscall_64+0x6c/0x180 ? entry_SYSCALL_64_after_hwframe+0x76/0x7e ? __pfx_rtnetlink_rcv_msg+0x10/0x10 netlink_rcv_skb+0x54/0x100 netlink_unicast+0x23e/0x390 netlink_sendmsg+0x21e/0x490 ____sys_sendmsg+0x31b/0x350 ? copy_msghdr_from_user+0x6d/0xa0 ___sys_sendmsg+0x86/0xd0 ? __pte_offset_map+0x17/0x160 ? preempt_count_add+0x69/0xa0 ? __call_rcu_common.constprop.0+0x147/0x610 ? preempt_count_add+0x69/0xa0 ? preempt_count_add+0x69/0xa0 ? _raw_spin_trylock+0x13/0x60 ? trace_hardirqs_on+0x1d/0x80 __sys_sendmsg+0x66/0xc0 do_syscall_64+0x6c/0x180 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7f41defe5b34 Code: 15 e1 12 0f 00 f7 d8 64 89 02 b8 ff ff ff ff eb bf 0f 1f 44 00 00 f3 0f 1e fa 80 3d 35 95 0f 00 00 74 13 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 4c c3 0f 1f 00 55 48 89 e5 48 83 ec 20 89 55 RSP: 002b:00007ffe5336ecc8 EFLAGS: 00000202 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f41defe5b34 RDX: 0000000000000000 RSI: 00007ffe5336ed30 RDI: 0000000000000003 RBP: 00007ffe5336eda0 R08: 0000000000000010 R09: 0000000000000001 R10: 00007ffe5336f6f9 R11: 0000000000000202 R12: 0000000000000003 R13: 0000000067452259 R14: 0000556ccc28b040 R15: 0000000000000000 </TASK> [...] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---
Fixes: c8bd1f7f3e61 ("virtio_net: add support for Byte Queue Limits") Cc: stable@vger.kernel.org # v6.11+ Signed-off-by: Koichiro Den koichiro.den@canonical.com
drivers/net/virtio_net.c | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 64c87bb48a41..3e36c0470600 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -513,7 +513,7 @@ static struct sk_buff *virtnet_skb_append_frag(struct sk_buff *head_skb, struct sk_buff *curr_skb, struct page *page, void *buf, int len, int truesize); -static void virtnet_xsk_completed(struct send_queue *sq, int num); +static void virtnet_xsk_completed(struct send_queue *sq, int num, bool drain);
enum virtnet_xmit_type { VIRTNET_XMIT_TYPE_SKB, @@ -580,7 +580,8 @@ static void sg_fill_dma(struct scatterlist *sg, dma_addr_t addr, u32 len) }
static void __free_old_xmit(struct send_queue *sq, struct netdev_queue *txq,
bool in_napi, struct virtnet_sq_free_stats *stats)
bool in_napi, struct virtnet_sq_free_stats *stats,bool drain){ struct xdp_frame *frame; struct sk_buff *skb; @@ -620,7 +621,8 @@ static void __free_old_xmit(struct send_queue *sq, struct netdev_queue *txq, break; } }
netdev_tx_completed_queue(txq, stats->napi_packets, stats->napi_bytes);
if (!drain)netdev_tx_completed_queue(txq, stats->napi_packets, stats->napi_bytes);}
static void virtnet_free_old_xmit(struct send_queue *sq, @@ -628,10 +630,21 @@ static void virtnet_free_old_xmit(struct send_queue *sq, bool in_napi, struct virtnet_sq_free_stats *stats) {
__free_old_xmit(sq, txq, in_napi, stats);
__free_old_xmit(sq, txq, in_napi, stats, false); if (stats->xsk)
virtnet_xsk_completed(sq, stats->xsk);
virtnet_xsk_completed(sq, stats->xsk, false);+}
+static void virtnet_drain_old_xmit(struct send_queue *sq,
struct netdev_queue *txq)+{
struct virtnet_sq_free_stats stats = {0};__free_old_xmit(sq, txq, false, &stats, true);if (stats.xsk)virtnet_xsk_completed(sq, stats.xsk, true);}
Are we sure this can drain the queue? Note that the device is not stopped.
Thanks for reviewing. netif_tx_wake_queue can be invoked before the "drain" point I added e.g. via virtnet_config_changed_work, so it seems that I need to ensure it's stopped (DRV_XOFF) before the "drain" and wake it afterwards. Please let me know if I’m mistaken.
/* Converting between virtqueue no. and kernel tx/rx queue no. @@ -1499,7 +1512,8 @@ static bool virtnet_xsk_xmit(struct send_queue *sq, struct xsk_buff_pool *pool, /* Avoid to wakeup napi meanless, so call __free_old_xmit instead of * free_old_xmit(). */
__free_old_xmit(sq, netdev_get_tx_queue(dev, sq - vi->sq), true, &stats);
__free_old_xmit(sq, netdev_get_tx_queue(dev, sq - vi->sq), true,&stats, false); if (stats.xsk) xsk_tx_completed(sq->xsk_pool, stats.xsk);@@ -1556,10 +1570,13 @@ static int virtnet_xsk_wakeup(struct net_device *dev, u32 qid, u32 flag) return 0; }
-static void virtnet_xsk_completed(struct send_queue *sq, int num) +static void virtnet_xsk_completed(struct send_queue *sq, int num, bool drain) { xsk_tx_completed(sq->xsk_pool, num);
if (drain)return;/* If this is called by rx poll, start_xmit and xdp xmit we should * wakeup the tx napi to consume the xsk tx queue, because the tx * interrupt may not be triggered.@@ -3041,6 +3058,7 @@ static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index) {
struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, qp_index); struct net_device *dev = vi->dev; int err;@@ -3054,7 +3072,10 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index) if (err < 0) goto err_xdp_reg_mem_model;
netdev_tx_reset_queue(netdev_get_tx_queue(vi->dev, qp_index));
/* Drain any unconsumed TX skbs transmitted before the last virtnet_close */virtnet_drain_old_xmit(&vi->sq[qp_index], txq);netdev_tx_reset_queue(txq); virtnet_napi_enable(vi->rq[qp_index].vq, &vi->rq[qp_index].napi); virtnet_napi_tx_enable(vi, vi->sq[qp_index].vq, &vi->sq[qp_index].napi);-- 2.43.0
Thanks