Re: [PATCH-for-6.0] net: tap: fix crash on hotplug

[Cole Robinson <crobinso@redhat.com>]

On 4/22/21 5:42 AM, Bin Meng wrote:
> On Thu, Apr 22, 2021 at 5:36 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Thu, 22 Apr 2021 at 05:29, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> On Thu, Apr 22, 2021 at 12:36 AM Philippe Mathieu-Daudé
>>> <philmd@redhat.com> wrote:
>>>>
>>>> Cc'ing Bin.
>>>>
>>>> On 4/21/21 5:22 PM, Cole Robinson wrote:
>>>>> Attempting to hotplug a tap nic with libvirt will crash qemu:
>>>>>
>>>>> $ sudo virsh attach-interface f32 network default
>>>>> error: Failed to attach interface
>>>>> error: Unable to read from monitor: Connection reset by peer
>>>>>
>>>>> 0x000055875b7f3a99 in tap_send (opaque=0x55875e39eae0) at ../net/tap.c:206
>>>>> 206           if (!s->nc.peer->do_not_pad) {
>>>>> gdb$ bt
>>>>>
>>>>> s->nc.peer may not be set at this point. This seems to be an
>>>>> expected case, as qemu_send_packet_* explicitly checks for NULL
>>>>> s->nc.peer later.
>>>>>
>>>>> Fix it by checking for s->nc.peer here too. Padding is applied if
>>>>> s->nc.peer is not set.
>>>>>
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1949786
>>>>> Fixes: 969e50b61a2
>>>>>
>>>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>>>> ---
>>>>> * Or should we skip padding if nc.peer is unset? I didn't dig into it
>>>>> * tap-win3.c and slirp.c may need a similar fix, but the slirp case
>>>>>   didn't crash in a simple test.
>>>>>
>>>>>  net/tap.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/tap.c b/net/tap.c
>>>>> index dd42ac6134..937559dbb8 100644
>>>>> --- a/net/tap.c
>>>>> +++ b/net/tap.c
>>>>> @@ -203,7 +203,7 @@ static void tap_send(void *opaque)
>>>>>              size -= s->host_vnet_hdr_len;
>>>>>          }
>>>>>
>>>>> -        if (!s->nc.peer->do_not_pad) {
>>>>> +        if (!s->nc.peer || !s->nc.peer->do_not_pad) {
>>>
>>> I think we should do:
>>>
>>> if (s->nc.peer && !s->nc.peer->do_not_pad)
>>
>> Yes. If there is no peer then the qemu_send_packet() that we're about
>> to do is going to discard the packet anyway, so there's no point in
>> padding it.
>>
>> Maybe consider
>>
>> static inline bool net_peer_needs_padding(NetClientState *nc)
>> {
>>     return nc->peer && !nc->peer->do_not_pad;
>> }
>>
>> since we want the same check in three places ?
> 
> Sounds good to me.
> 

I did not get to this today. Bin/Jason/anyone want to write the patch, I
will test it tomorrow (US EDT time). If not I'll write the patch tomorrow.

Thanks,
Cole