| Message ID | 1507829163-59506-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] netdev: Fix memory leak on error path. | expand |
On Thu, Oct 12, 2017 at 10:26:03AM -0700, William Tu wrote: > Found by inspection. > > Signed-off-by: William Tu <u9012063@gmail.com> > --- > lib/netdev.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/lib/netdev.c b/lib/netdev.c > index b4e570bafd08..bedc21dec48e 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -2220,6 +2220,8 @@ netdev_ports_insert(struct netdev *netdev, const struct dpif_class *dpif_class, > ovs_mutex_lock(&netdev_hmap_mutex); > if (netdev_ports_lookup(dpif_port->port_no, dpif_class)) { > ovs_mutex_unlock(&netdev_hmap_mutex); > + free(data); > + free(ifidx); > return EEXIST; > } Thanks for the report and the fix. I think that we can avoid doing the allocations entirely for this case. What do you think of the following? I guess that it moves allocations into the lock, but I don't know of a performance bottleneck on this lock. --8<--------------------------cut here-------------------------->8-- From: Ben Pfaff <blp@ovn.org> Date: Thu, 12 Oct 2017 10:33:06 -0700 Subject: [PATCH] netdev: Fix memory leak on error path in netdev_ports_insert(). Reported-by: William Tu <u9012063@gmail.com> Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/netdev.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/netdev.c b/lib/netdev.c index b4e570bafd08..8d611bc826c2 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -2206,27 +2206,24 @@ netdev_ports_insert(struct netdev *netdev, const struct dpif_class *dpif_class, struct dpif_port *dpif_port) { size_t hash = NETDEV_PORTS_HASH_INT(dpif_port->port_no, dpif_class); - struct port_to_netdev_data *data; - struct ifindex_to_port_data *ifidx; int ifindex = netdev_get_ifindex(netdev); if (ifindex < 0) { return ENODEV; } - data = xzalloc(sizeof *data); - ifidx = xzalloc(sizeof *ifidx); - ovs_mutex_lock(&netdev_hmap_mutex); if (netdev_ports_lookup(dpif_port->port_no, dpif_class)) { ovs_mutex_unlock(&netdev_hmap_mutex); return EEXIST; } + struct port_to_netdev_data *data = xzalloc(sizeof *data); data->netdev = netdev_ref(netdev); data->dpif_class = dpif_class; dpif_port_clone(&data->dpif_port, dpif_port); + struct ifindex_to_port_data *ifidx = xzalloc(sizeof *ifidx); ifidx->ifindex = ifindex; ifidx->port = dpif_port->port_no;
> Thanks for the report and the fix. > > I think that we can avoid doing the allocations entirely for this case. > What do you think of the following? I guess that it moves allocations > into the lock, but I don't know of a performance bottleneck on this > lock. > > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <blp@ovn.org> > Date: Thu, 12 Oct 2017 10:33:06 -0700 > Subject: [PATCH] netdev: Fix memory leak on error path in > netdev_ports_insert(). > > Reported-by: William Tu <u9012063@gmail.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > lib/netdev.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/lib/netdev.c b/lib/netdev.c > index b4e570bafd08..8d611bc826c2 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -2206,27 +2206,24 @@ netdev_ports_insert(struct netdev *netdev, const struct dpif_class *dpif_class, > struct dpif_port *dpif_port) > { > size_t hash = NETDEV_PORTS_HASH_INT(dpif_port->port_no, dpif_class); > - struct port_to_netdev_data *data; > - struct ifindex_to_port_data *ifidx; > int ifindex = netdev_get_ifindex(netdev); > > if (ifindex < 0) { > return ENODEV; > } > > - data = xzalloc(sizeof *data); > - ifidx = xzalloc(sizeof *ifidx); > - > ovs_mutex_lock(&netdev_hmap_mutex); > if (netdev_ports_lookup(dpif_port->port_no, dpif_class)) { > ovs_mutex_unlock(&netdev_hmap_mutex); > return EEXIST; > } > > + struct port_to_netdev_data *data = xzalloc(sizeof *data); > data->netdev = netdev_ref(netdev); > data->dpif_class = dpif_class; > dpif_port_clone(&data->dpif_port, dpif_port); > > + struct ifindex_to_port_data *ifidx = xzalloc(sizeof *ifidx); > ifidx->ifindex = ifindex; > ifidx->port = dpif_port->port_no; > > -- > 2.10.2 > Thanks for the feedback. Yes, I think it looks better, and since port insert is not in performance critical path, allocation inside the mutex should not have much impact. Let's me test it and resubmit v2. William
diff --git a/lib/netdev.c b/lib/netdev.c index b4e570bafd08..bedc21dec48e 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -2220,6 +2220,8 @@ netdev_ports_insert(struct netdev *netdev, const struct dpif_class *dpif_class, ovs_mutex_lock(&netdev_hmap_mutex); if (netdev_ports_lookup(dpif_port->port_no, dpif_class)) { ovs_mutex_unlock(&netdev_hmap_mutex); + free(data); + free(ifidx); return EEXIST; }
Found by inspection. Signed-off-by: William Tu <u9012063@gmail.com> --- lib/netdev.c | 2 ++ 1 file changed, 2 insertions(+)