Re: [Qemu-devel] [PATCH] net: prevent sending packets while guest is stopped

[Michael S. Tsirkin]

On Tue, Sep 02, 2014 at 05:25:53PM +0300, Michael S. Tsirkin wrote:
> On Tue, Sep 02, 2014 at 03:23:10PM +0100, Stefan Hajnoczi wrote:
> > Do not modify guest memory or devices when the guest is stopped.
> > Currently the netdevs still send packets while the guest is stopped if
> > their file descriptor was being monitored for write (e.g. the socket
> > write buffer filled before the guest was stopped).
> > 
> > Netdevs call qemu_flush_queued_packets() when the file descriptor
> > becomes writable again.  Don't resume packet processing when this
> > happens.
> > 
> > Instead we flush queues when the guest resumes.
> > 
> > Cc: address@hidden
> > Reported-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> 
> Reviewed-by: Michael S. Tsirkin <address@hidden>

Wait a second. I see a problem here.

> 
> > ---
> > Note this fixes the transmit side.  The receive side was recently fixed in
> > "net: Forbid dealing with packets when VM is not running".
> > 
> >  net/net.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/net/net.c b/net/net.c
> > index 6d930ea..74ec07a 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -47,6 +47,7 @@
> >  # define CONFIG_NET_BRIDGE
> >  #endif
> >  
> > +static VMChangeStateEntry *net_change_state_entry;
> >  static QTAILQ_HEAD(, NetClientState) net_clients;
> >  
> >  const char *host_net_devices[] = {
> > @@ -506,6 +507,11 @@ void qemu_purge_queued_packets(NetClientState *nc)
> >  
> >  void qemu_flush_queued_packets(NetClientState *nc)
> >  {
> > +    /* Guest memory and devices must not be modified while stopped */
> > +    if (!runstate_is_running()) {
> > +        return;
> > +    }
> > +
> >      nc->receive_disabled = 0;
> >  
> >      if (nc->peer && nc->peer->info->type == 
> > NET_CLIENT_OPTIONS_KIND_HUBPORT) {

virtio net really assumes that after qemu_flush_queued_packets
all packets have been consumed.
So if we migrate, we never resume and queued packets will never be
completed.

If we can not flush them out, so we really must go ahead and discard
queued packets.

And if you do this, no need for a state handler.

So NAK, sorry about noticing this late.

> > @@ -1168,6 +1174,25 @@ void qmp_set_link(const char *name, bool up, Error 
> > **errp)
> >      }
> >  }
> >  
> > +/* Kick net clients when guest resumes.  If a file descriptor was 
> > monitored for
> > + * writing before the guest was stopped, there will be nothing monitoring 
> > it
> > + * right now so a kick is required to get packets flowing again.
> > + */
> > +static void net_vm_change_state_handler(void *opaque, int running,
> > +                                        RunState state)
> > +{
> > +    NetClientState *nc;
> > +    NetClientState *tmp;
> > +
> > +    if (!running) {
> > +        return;
> > +    }
> > +
> > +    QTAILQ_FOREACH_SAFE(nc, &net_clients, next, tmp) {
> > +        qemu_flush_queued_packets(nc);
> > +    }
> > +}
> > +
> >  void net_cleanup(void)
> >  {
> >      NetClientState *nc;
> > @@ -1183,6 +1208,8 @@ void net_cleanup(void)
> >              qemu_del_net_client(nc);
> >          }
> >      }
> > +
> > +    qemu_del_vm_change_state_handler(net_change_state_entry);
> >  }
> >  
> >  void net_check_clients(void)
> > @@ -1268,6 +1295,9 @@ int net_init_clients(void)
> >  #endif
> >      }
> >  
> > +    net_change_state_entry =
> > +        qemu_add_vm_change_state_handler(net_vm_change_state_handler, 
> > NULL);
> > +
> >      QTAILQ_INIT(&net_clients);
> >  
> >      if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 
> > 1) == -1)
> > -- 
> > 1.9.3