From: Jason Wang <jasowang@redhat.com> To: Xianting Tian <xianting.tian@linux.alibaba.com>, "Michael S. Tsirkin" <mst@redhat.com> Cc: davem@davemloft.net, kuba@kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] virtio_net: Remove BUG() to aviod machine dead Date: Tue, 25 May 2021 14:19:03 +0800 [thread overview] Message-ID: <72f284c6-b2f5-a395-a68f-afe801eb81be@redhat.com> (raw) In-Reply-To: <4aaf5125-ce75-c72a-4b4a-11c91cb85a72@linux.alibaba.com> 在 2021/5/19 下午10:18, Xianting Tian 写道: > thanks, I submit the patch as commented by Andrew > https://lkml.org/lkml/2021/5/18/256 > > 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)) { > dev->stats.tx_fifo_errors++; > if (net_ratelimit()) > dev_warn(&dev->dev, > "Unexpected TXQ (%d) queue failure: %d\n", > qnum, err); > dev->stats.tx_dropped++; > dev_kfree_skb_any(skb); > 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@linux.alibaba.com> >>> --- >>> 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. Thanks >> >> don't we want at least a WARN_ON? Or _ONCE? >> >>> if (vi->mergeable_rx_bufs) >>> hdr->num_buffers = 0; >>> -- >>> 2.17.1 >
WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com> To: Xianting Tian <xianting.tian@linux.alibaba.com>, "Michael S. Tsirkin" <mst@redhat.com> Cc: kuba@kernel.org, netdev@vger.kernel.org, davem@davemloft.net, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH] virtio_net: Remove BUG() to aviod machine dead Date: Tue, 25 May 2021 14:19:03 +0800 [thread overview] Message-ID: <72f284c6-b2f5-a395-a68f-afe801eb81be@redhat.com> (raw) In-Reply-To: <4aaf5125-ce75-c72a-4b4a-11c91cb85a72@linux.alibaba.com> 在 2021/5/19 下午10:18, Xianting Tian 写道: > thanks, I submit the patch as commented by Andrew > https://lkml.org/lkml/2021/5/18/256 > > 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)) { > dev->stats.tx_fifo_errors++; > if (net_ratelimit()) > dev_warn(&dev->dev, > "Unexpected TXQ (%d) queue failure: %d\n", > qnum, err); > dev->stats.tx_dropped++; > dev_kfree_skb_any(skb); > 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@linux.alibaba.com> >>> --- >>> 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. Thanks >> >> don't we want at least a WARN_ON? Or _ONCE? >> >>> if (vi->mergeable_rx_bufs) >>> hdr->num_buffers = 0; >>> -- >>> 2.17.1 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-05-25 6:19 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-18 9:46 [PATCH] virtio_net: Remove BUG() to aviod machine dead Xianting Tian 2021-05-18 9:54 ` Michael S. Tsirkin 2021-05-18 9:54 ` Michael S. Tsirkin 2021-05-19 14:18 ` Xianting Tian 2021-05-25 6:19 ` Jason Wang [this message] 2021-05-25 6:19 ` Jason Wang 2021-06-02 5:59 ` Leon Romanovsky 2021-06-02 5:59 ` Leon Romanovsky 2021-06-02 7:14 ` Jason Wang 2021-06-02 7:14 ` Jason Wang 2021-06-02 12:54 ` Leon Romanovsky 2021-06-02 12:54 ` Leon Romanovsky 2021-05-20 7:35 ` Stefano Garzarella 2021-05-20 7:35 ` Stefano Garzarella
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=72f284c6-b2f5-a395-a68f-afe801eb81be@redhat.com \ --to=jasowang@redhat.com \ --cc=davem@davemloft.net \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=virtualization@lists.linux-foundation.org \ --cc=xianting.tian@linux.alibaba.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.