Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] virtio_net: Remove BUG() to aviod machine dead
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
References: <a351fbe1-0233-8515-2927-adc826a7fb94@linux.alibaba.com>
 <20210518055336-mutt-send-email-mst@kernel.org>
 <4aaf5125-ce75-c72a-4b4a-11c91cb85a72@linux.alibaba.com>
From:   Jason Wang <jasowang@redhat.com>
Message-ID: <72f284c6-b2f5-a395-a68f-afe801eb81be@redhat.com>
Date:   Tue, 25 May 2021 14:19:03 +0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.10.2
MIME-Version: 1.0
In-Reply-To: <4aaf5125-ce75-c72a-4b4a-11c91cb85a72@linux.alibaba.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


在 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
>