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

[Bin Meng <bmeng.cn@gmail.com>]

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.

Regards,
Bin