Patchwork [v2,3/6] net: make netclient re-entrant with refcnt

login
register
mail settings
Submitter pingfan liu
Date June 13, 2013, 9:03 a.m.
Message ID <1371114186-8854-4-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/251013/
State New
Headers show

Comments

pingfan liu - June 13, 2013, 9:03 a.m.
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

With refcnt, NetClientState's user can run agaist deleter.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/core/qdev-properties-system.c | 14 ++++++++++++
 include/net/net.h                |  3 +++
 net/hub.c                        |  3 +++
 net/net.c                        | 47 +++++++++++++++++++++++++++++++++++++---
 net/slirp.c                      |  3 ++-
 5 files changed, 66 insertions(+), 4 deletions(-)
Stefan Hajnoczi - June 18, 2013, 12:41 p.m.
On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> With refcnt, NetClientState's user can run agaist deleter.

Please split this into two patches:

1. net_clients lock
2. NetClientState refcount

> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/core/qdev-properties-system.c | 14 ++++++++++++
>  include/net/net.h                |  3 +++
>  net/hub.c                        |  3 +++
>  net/net.c                        | 47 +++++++++++++++++++++++++++++++++++++---
>  net/slirp.c                      |  3 ++-
>  5 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 0eada32..41cc7e6 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -302,6 +302,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
>          return;
>      }
>  
> +    /* inc ref, released when unset property */
>      hubport = net_hub_port_find(id);
>      if (!hubport) {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> @@ -311,11 +312,24 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
>      *ptr = hubport;
>  }
>  
> +static void release_vlan(Object *obj, const char *name, void *opaque)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    NICPeers *peers_ptr = qdev_get_prop_ptr(dev, prop);
> +    NetClientState **ptr = &peers_ptr->ncs[0];
> +
> +    if (*ptr) {
> +        netclient_unref(*ptr);
> +    }
> +}
> +
>  PropertyInfo qdev_prop_vlan = {
>      .name  = "vlan",
>      .print = print_vlan,
>      .get   = get_vlan,
>      .set   = set_vlan,
> +    .release = release_vlan,
>  };
>  
>  int qdev_prop_set_drive(DeviceState *dev, const char *name,

What about the netdev property?  I don't see any refcount code there.

> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>              qemu_del_net_client(nc);
>          }
>      }
> +    qemu_mutex_destroy(&net_clients_lock);

Why is it okay to iterate over net_clients here without the lock?
pingfan liu - June 20, 2013, 9:14 a.m.
On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> With refcnt, NetClientState's user can run agaist deleter.
>
> Please split this into two patches:
>
> 1. net_clients lock
> 2. NetClientState refcount
>
Ok.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/core/qdev-properties-system.c | 14 ++++++++++++
>>  include/net/net.h                |  3 +++
>>  net/hub.c                        |  3 +++
>>  net/net.c                        | 47 +++++++++++++++++++++++++++++++++++++---
>>  net/slirp.c                      |  3 ++-
>>  5 files changed, 66 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
>> index 0eada32..41cc7e6 100644
>> --- a/hw/core/qdev-properties-system.c
>> +++ b/hw/core/qdev-properties-system.c
>> @@ -302,6 +302,7 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
>>          return;
>>      }
>>
>> +    /* inc ref, released when unset property */
>>      hubport = net_hub_port_find(id);
>>      if (!hubport) {
>>          error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> @@ -311,11 +312,24 @@ static void set_vlan(Object *obj, Visitor *v, void *opaque,
>>      *ptr = hubport;
>>  }
>>
>> +static void release_vlan(Object *obj, const char *name, void *opaque)
>> +{
>> +    DeviceState *dev = DEVICE(obj);
>> +    Property *prop = opaque;
>> +    NICPeers *peers_ptr = qdev_get_prop_ptr(dev, prop);
>> +    NetClientState **ptr = &peers_ptr->ncs[0];
>> +
>> +    if (*ptr) {
>> +        netclient_unref(*ptr);
>> +    }
>> +}
>> +
>>  PropertyInfo qdev_prop_vlan = {
>>      .name  = "vlan",
>>      .print = print_vlan,
>>      .get   = get_vlan,
>>      .set   = set_vlan,
>> +    .release = release_vlan,
>>  };
>>
>>  int qdev_prop_set_drive(DeviceState *dev, const char *name,
>
> What about the netdev property?  I don't see any refcount code there.
>
Yes, the release of netdev and vlan property should all free its
backend. Will add the code.
>> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>>              qemu_del_net_client(nc);
>>          }
>>      }
>> +    qemu_mutex_destroy(&net_clients_lock);
>
> Why is it okay to iterate over net_clients here without the lock?

 atexit(&net_cleanup); So no other racers exist.

Thx & Regards,
Pingfan
Stefan Hajnoczi - July 1, 2013, 11:50 a.m.
On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
> >>              qemu_del_net_client(nc);
> >>          }
> >>      }
> >> +    qemu_mutex_destroy(&net_clients_lock);
> >
> > Why is it okay to iterate over net_clients here without the lock?
> 
>  atexit(&net_cleanup); So no other racers exist.

What about dataplane?  The device may not be reset when net_cleanup runs.

It's best not to make assumptions, taking the lock is easy.

Stefan
pingfan liu - July 3, 2013, 3:41 a.m.
On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
>> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
>> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>> >>              qemu_del_net_client(nc);
>> >>          }
>> >>      }
>> >> +    qemu_mutex_destroy(&net_clients_lock);
>> >
>> > Why is it okay to iterate over net_clients here without the lock?
>>
>>  atexit(&net_cleanup); So no other racers exist.
>
> What about dataplane?  The device may not be reset when net_cleanup runs.
>
Does the func registered by atexit run after all of the other threads terminate?
> It's best not to make assumptions, taking the lock is easy.
>
Yes, assumptions are not reliable. I will take the lock for the next version.

Thx & regards,
Pingfan
> Stefan
Stefan Hajnoczi - July 3, 2013, 7:49 a.m.
On Wed, Jul 03, 2013 at 11:41:19AM +0800, liu ping fan wrote:
> On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
> >> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> >> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
> >> >>              qemu_del_net_client(nc);
> >> >>          }
> >> >>      }
> >> >> +    qemu_mutex_destroy(&net_clients_lock);
> >> >
> >> > Why is it okay to iterate over net_clients here without the lock?
> >>
> >>  atexit(&net_cleanup); So no other racers exist.
> >
> > What about dataplane?  The device may not be reset when net_cleanup runs.
> >
> Does the func registered by atexit run after all of the other threads terminate?

I imagine that atexit(3) runs while detached threads are still alive,
but I'm not sure about the exact rules.  The pthread specification links
I found online didn't state the rules.

Stefan
pingfan liu - July 3, 2013, 7:54 a.m.
On Wed, Jul 3, 2013 at 3:49 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Jul 03, 2013 at 11:41:19AM +0800, liu ping fan wrote:
>> On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
>> >> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> >> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
>> >> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
>> >> >>              qemu_del_net_client(nc);
>> >> >>          }
>> >> >>      }
>> >> >> +    qemu_mutex_destroy(&net_clients_lock);
>> >> >
>> >> > Why is it okay to iterate over net_clients here without the lock?
>> >>
>> >>  atexit(&net_cleanup); So no other racers exist.
>> >
>> > What about dataplane?  The device may not be reset when net_cleanup runs.
>> >
>> Does the func registered by atexit run after all of the other threads terminate?
>
> I imagine that atexit(3) runs while detached threads are still alive,
> but I'm not sure about the exact rules.  The pthread specification links
> I found online didn't state the rules.
>
Haha, finally, got some hint for this.  pthread_exit(3) says:
       After  the  last  thread  in  a  process terminates, the
process terminates as by calling exit(3) with an exit status of zero;
thus, process-shared
       resources are released and functions registered using atexit(3)
are called.

Regards,
Pingfan

> Stefan
Stefan Hajnoczi - July 3, 2013, 12:01 p.m.
On Wed, Jul 03, 2013 at 03:54:44PM +0800, liu ping fan wrote:
> On Wed, Jul 3, 2013 at 3:49 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Wed, Jul 03, 2013 at 11:41:19AM +0800, liu ping fan wrote:
> >> On Mon, Jul 1, 2013 at 7:50 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Thu, Jun 20, 2013 at 05:14:56PM +0800, liu ping fan wrote:
> >> >> On Tue, Jun 18, 2013 at 8:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> >> > On Thu, Jun 13, 2013 at 05:03:03PM +0800, Liu Ping Fan wrote:
> >> >> >> @@ -1109,6 +1146,7 @@ void net_cleanup(void)
> >> >> >>              qemu_del_net_client(nc);
> >> >> >>          }
> >> >> >>      }
> >> >> >> +    qemu_mutex_destroy(&net_clients_lock);
> >> >> >
> >> >> > Why is it okay to iterate over net_clients here without the lock?
> >> >>
> >> >>  atexit(&net_cleanup); So no other racers exist.
> >> >
> >> > What about dataplane?  The device may not be reset when net_cleanup runs.
> >> >
> >> Does the func registered by atexit run after all of the other threads terminate?
> >
> > I imagine that atexit(3) runs while detached threads are still alive,
> > but I'm not sure about the exact rules.  The pthread specification links
> > I found online didn't state the rules.
> >
> Haha, finally, got some hint for this.  pthread_exit(3) says:
>        After  the  last  thread  in  a  process terminates, the
> process terminates as by calling exit(3) with an exit status of zero;
> thus, process-shared
>        resources are released and functions registered using atexit(3)
> are called.

That's only true for non-detached threads.  A program can exit while
detached threads are running.

Stefan

Patch

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 0eada32..41cc7e6 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -302,6 +302,7 @@  static void set_vlan(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
+    /* inc ref, released when unset property */
     hubport = net_hub_port_find(id);
     if (!hubport) {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE,
@@ -311,11 +312,24 @@  static void set_vlan(Object *obj, Visitor *v, void *opaque,
     *ptr = hubport;
 }
 
+static void release_vlan(Object *obj, const char *name, void *opaque)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    NICPeers *peers_ptr = qdev_get_prop_ptr(dev, prop);
+    NetClientState **ptr = &peers_ptr->ncs[0];
+
+    if (*ptr) {
+        netclient_unref(*ptr);
+    }
+}
+
 PropertyInfo qdev_prop_vlan = {
     .name  = "vlan",
     .print = print_vlan,
     .get   = get_vlan,
     .set   = set_vlan,
+    .release = release_vlan,
 };
 
 int qdev_prop_set_drive(DeviceState *dev, const char *name,
diff --git a/include/net/net.h b/include/net/net.h
index ea46f13..1a31d1b 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -64,6 +64,7 @@  typedef struct NetClientInfo {
 } NetClientInfo;
 
 struct NetClientState {
+    int ref;
     NetClientInfo *info;
     int link_down;
     QTAILQ_ENTRY(NetClientState) next;
@@ -92,6 +93,8 @@  typedef struct NICState {
 NetClientState *qemu_find_netdev(const char *id);
 int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
                                  NetClientOptionsKind type, int max);
+void netclient_ref(NetClientState *nc);
+void netclient_unref(NetClientState *nc);
 NetClientState *qemu_new_net_client(NetClientInfo *info,
                                     NetClientState *peer,
                                     const char *model,
diff --git a/net/hub.c b/net/hub.c
index df32074..9c6c559 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -201,6 +201,7 @@  NetClientState *net_hub_find_client_by_name(int hub_id, const char *name)
                 peer = port->nc.peer;
 
                 if (peer && strcmp(peer->name, name) == 0) {
+                    netclient_ref(peer);
                     return peer;
                 }
             }
@@ -223,6 +224,7 @@  NetClientState *net_hub_port_find(int hub_id)
             QLIST_FOREACH(port, &hub->ports, next) {
                 nc = port->nc.peer;
                 if (!nc) {
+                    netclient_ref(&port->nc);
                     return &(port->nc);
                 }
             }
@@ -231,6 +233,7 @@  NetClientState *net_hub_port_find(int hub_id)
     }
 
     nc = net_hub_add_port(hub_id, NULL);
+    netclient_ref(nc);
     return nc;
 }
 
diff --git a/net/net.c b/net/net.c
index 717db12..478a719 100644
--- a/net/net.c
+++ b/net/net.c
@@ -45,6 +45,7 @@ 
 # define CONFIG_NET_BRIDGE
 #endif
 
+static QemuMutex net_clients_lock;
 static QTAILQ_HEAD(, NetClientState) net_clients;
 
 int default_net = 1;
@@ -165,6 +166,7 @@  static char *assign_name(NetClientState *nc1, const char *model)
     char buf[256];
     int id = 0;
 
+    qemu_mutex_lock(&net_clients_lock);
     QTAILQ_FOREACH(nc, &net_clients, next) {
         if (nc == nc1) {
             continue;
@@ -173,6 +175,7 @@  static char *assign_name(NetClientState *nc1, const char *model)
             id++;
         }
     }
+    qemu_mutex_unlock(&net_clients_lock);
 
     snprintf(buf, sizeof(buf), "%s.%d", model, id);
 
@@ -203,9 +206,13 @@  static void qemu_net_client_setup(NetClientState *nc,
         assert(!peer->peer);
         nc->peer = peer;
         peer->peer = nc;
+        netclient_ref(peer);
+        netclient_ref(nc);
     }
     qemu_mutex_init(&nc->peer_lock);
+    qemu_mutex_lock(&net_clients_lock);
     QTAILQ_INSERT_TAIL(&net_clients, nc, next);
+    qemu_mutex_unlock(&net_clients_lock);
 
     nc->send_queue = qemu_new_net_queue(nc);
     nc->destructor = destructor;
@@ -221,6 +228,7 @@  NetClientState *qemu_new_net_client(NetClientInfo *info,
     assert(info->size >= sizeof(NetClientState));
 
     nc = g_malloc0(info->size);
+    netclient_ref(nc);
     qemu_net_client_setup(nc, info, peer, model, name,
                           qemu_net_client_destructor);
 
@@ -281,7 +289,9 @@  void *qemu_get_nic_opaque(NetClientState *nc)
 
 static void qemu_cleanup_net_client(NetClientState *nc)
 {
+    qemu_mutex_lock(&net_clients_lock);
     QTAILQ_REMOVE(&net_clients, nc, next);
+    qemu_mutex_unlock(&net_clients_lock);
 
     if (nc->info->cleanup) {
         nc->info->cleanup(nc);
@@ -303,6 +313,18 @@  static void qemu_free_net_client(NetClientState *nc)
     }
 }
 
+void netclient_ref(NetClientState *nc)
+{
+    __sync_add_and_fetch(&nc->ref, 1);
+}
+
+void netclient_unref(NetClientState *nc)
+{
+    if (__sync_sub_and_fetch(&nc->ref, 1) == 0) {
+        qemu_free_net_client(nc);
+    }
+}
+
 /* elimate the reference and sync with exit of rx/tx action.
  * And flush out peer's queue.
  */
@@ -331,8 +353,10 @@  static void qemu_net_client_detach_flush(NetClientState *nc)
     nc->peer = NULL;
     if (peer) {
         qemu_net_queue_purge(peer->send_queue, nc);
+        netclient_unref(peer);
     }
     qemu_mutex_unlock(&nc->peer_lock);
+    netclient_unref(nc);
 }
 
 void qemu_del_net_client(NetClientState *nc)
@@ -378,7 +402,7 @@  void qemu_del_net_client(NetClientState *nc)
     for (i = 0; i < queues; i++) {
         qemu_net_client_detach_flush(ncs[i]);
         qemu_cleanup_net_client(ncs[i]);
-        qemu_free_net_client(ncs[i]);
+        netclient_unref(ncs[i]);
     }
 }
 
@@ -389,7 +413,7 @@  void qemu_del_nic(NICState *nic)
     /* If this is a peer NIC and peer has already been deleted, free it now. */
     if (nic->peer_deleted) {
         for (i = 0; i < queues; i++) {
-            qemu_free_net_client(nic->pending_peer[i]);
+            netclient_unref(nic->pending_peer[i]);
         }
     }
 
@@ -398,7 +422,7 @@  void qemu_del_nic(NICState *nic)
 
         qemu_net_client_detach_flush(nc);
         qemu_cleanup_net_client(nc);
-        qemu_free_net_client(nc);
+        netclient_unref(nc);
     }
 
     g_free(nic->pending_peer);
@@ -409,6 +433,7 @@  void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
 {
     NetClientState *nc;
 
+    qemu_mutex_lock(&net_clients_lock);
     QTAILQ_FOREACH(nc, &net_clients, next) {
         if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
             if (nc->queue_index == 0) {
@@ -416,6 +441,7 @@  void qemu_foreach_nic(qemu_nic_foreach func, void *opaque)
             }
         }
     }
+    qemu_mutex_unlock(&net_clients_lock);
 }
 
 int qemu_can_send_packet_nolock(NetClientState *sender)
@@ -630,13 +656,17 @@  NetClientState *qemu_find_netdev(const char *id)
 {
     NetClientState *nc;
 
+    qemu_mutex_lock(&net_clients_lock);
     QTAILQ_FOREACH(nc, &net_clients, next) {
         if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC)
             continue;
         if (!strcmp(nc->name, id)) {
+            netclient_ref(nc);
+            qemu_mutex_unlock(&net_clients_lock);
             return nc;
         }
     }
+    qemu_mutex_unlock(&net_clients_lock);
 
     return NULL;
 }
@@ -647,6 +677,7 @@  int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
     NetClientState *nc;
     int ret = 0;
 
+    qemu_mutex_lock(&net_clients_lock);
     QTAILQ_FOREACH(nc, &net_clients, next) {
         if (nc->info->type == type) {
             continue;
@@ -658,6 +689,7 @@  int qemu_find_net_clients_except(const char *id, NetClientState **ncs,
             ret++;
         }
     }
+    qemu_mutex_unlock(&net_clients_lock);
 
     return ret;
 }
@@ -963,9 +995,11 @@  void net_host_device_remove(Monitor *mon, const QDict *qdict)
     }
     if (!net_host_check_device(nc->model)) {
         monitor_printf(mon, "invalid host network device %s\n", device);
+        netclient_unref(nc);
         return;
     }
     qemu_del_net_client(nc);
+    netclient_unref(nc);
 }
 
 void netdev_add(QemuOpts *opts, Error **errp)
@@ -1021,6 +1055,7 @@  void qmp_netdev_del(const char *id, Error **errp)
     }
 
     qemu_del_net_client(nc);
+    netclient_unref(nc);
     qemu_opts_del(opts);
 }
 
@@ -1039,6 +1074,7 @@  void do_info_network(Monitor *mon, const QDict *qdict)
 
     net_hub_info(mon);
 
+    qemu_mutex_lock(&net_clients_lock);
     QTAILQ_FOREACH(nc, &net_clients, next) {
         peer = nc->peer;
         type = nc->info->type;
@@ -1056,6 +1092,7 @@  void do_info_network(Monitor *mon, const QDict *qdict)
             print_net_client(mon, peer);
         }
     }
+    qemu_mutex_unlock(&net_clients_lock);
 }
 
 void qmp_set_link(const char *name, bool up, Error **errp)
@@ -1109,6 +1146,7 @@  void net_cleanup(void)
             qemu_del_net_client(nc);
         }
     }
+    qemu_mutex_destroy(&net_clients_lock);
 }
 
 void net_check_clients(void)
@@ -1130,6 +1168,7 @@  void net_check_clients(void)
 
     net_hub_check_clients();
 
+    qemu_mutex_lock(&net_clients_lock);
     QTAILQ_FOREACH(nc, &net_clients, next) {
         if (!nc->peer) {
             fprintf(stderr, "Warning: %s %s has no peer\n",
@@ -1137,6 +1176,7 @@  void net_check_clients(void)
                     "nic" : "netdev", nc->name);
         }
     }
+    qemu_mutex_unlock(&net_clients_lock);
 
     /* Check that all NICs requested via -net nic actually got created.
      * NICs created via -device don't need to be checked here because
@@ -1194,6 +1234,7 @@  int net_init_clients(void)
 #endif
     }
 
+    qemu_mutex_init(&net_clients_lock);
     QTAILQ_INIT(&net_clients);
 
     if (qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL, 1) == -1)
diff --git a/net/slirp.c b/net/slirp.c
index b3f35d5..e541548 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -346,7 +346,7 @@  void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict)
 
     err = slirp_remove_hostfwd(QTAILQ_FIRST(&slirp_stacks)->slirp, is_udp,
                                host_addr, host_port);
-
+    netclient_unref(&s->nc);
     monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
                    err ? "not found" : "removed");
     return;
@@ -437,6 +437,7 @@  void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict)
     }
     if (s) {
         slirp_hostfwd(s, redir_str, 0);
+        netclient_unref(&s->nc);
     }
 
 }