Re: [PATCH] virtio_net: Remove BUG() to aviod machine dead

From: Jason Wang
Date: Tue May 25 2021 - 02:19:23 EST

在 2021/5/19 下午10:18, Xianting Tian 写道:
thanks, I submit the patch as commented by Andrew

Actually, if xmit_skb() returns error, below code will give a warning with error code.

    /* Try to transmit */
    err = xmit_skb(sq, skb);

    /* This should not happen! */
    if (unlikely(err)) {
        if (net_ratelimit())
                 "Unexpected TXQ (%d) queue failure: %d\n",
                 qnum, err);
        return NETDEV_TX_OK;

在 2021/5/18 下午5:54, Michael S. Tsirkin 写道:
typo in subject

On Tue, May 18, 2021 at 05:46:56PM +0800, Xianting Tian wrote:
When met error, we output a print to avoid a BUG().

So you don't explain why you need to remove BUG(). I think it deserve a BUG().

Signed-off-by: Xianting Tian <xianting.tian@xxxxxxxxxxxxxxxxx>
  drivers/net/virtio_net.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c921ebf3ae82..a66174d13e81 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1647,9 +1647,8 @@ static int xmit_skb(struct send_queue *sq, struct
sk_buff *skb)
          hdr = skb_vnet_hdr(skb);

      if (virtio_net_hdr_from_skb(skb, &hdr->hdr,
-                    virtio_is_little_endian(vi->vdev), false,
-                    0))
-        BUG();
+                virtio_is_little_endian(vi->vdev), false, 0))
+        return -EPROTO;

why EPROTO? can you add some comments to explain what is going on pls?

is this related to a malicious hypervisor thing?

I think not if I was not wrong.

Each sources (either userspace or device), the skb should be built through virtio_net_hdr_to_skb() which means the validation has already been done.

If we it fails here, it's a real bug.


don't we want at least a WARN_ON? Or _ONCE?

      if (vi->mergeable_rx_bufs)
          hdr->num_buffers = 0;