Re: vmd(8): fix memory leak in virtio network TX path
On Mon, Sep 23, 2019 at 08:44:01AM +0200, Theo Buehler wrote:
> On Sun, Sep 22, 2019 at 02:42:28AM -0700, Mike Larkin wrote:
> > We allocate a 'pkt' for each network packet in the queue, but only were
> > freeing the last one. This has always been a bug, but it looks like recent
> > changes elsewhere in the network stack may have made the problem more
> > apparent
> > since more packets seem to be deposited in the queue for each TX operation
> > now.
> >
> > This was reported back in July by Paul de Weerd, who told me this could be
> > reproduced by doing iperf tests between a VM and the host. He and I have
> > tested that this diff seems to resolve the problem.
> >
> > There is a similar leak with the built-in vmd(8) DHCP service's packets.
> > Both are fixed below. Each pointer is reset to NULL so that the final
> > free at the end of the function doesn't double free the last buffer(s).
>
> That doesn't look right as dhcppkt will be used after the while loop:
>
> 1542 if (dhcpsz > 0) {
> 1543 if (vionet_enq_rx(dev, dhcppkt, dhcpsz, ))
> 1544 ret = 1;
> 1545 }
>
> I've tried this diff with a local interface and indeed it broke dhcp.
>
> Note that dhcppkt is only allocated once as afterwards dhcpsz > 0 will
> prevent further calls to dhcp_request(), so I'm not sure we should touch
> dhcppkt at all.
>
> An alternative to the diff below would be to free and null out pkt and
> dhcpkt at the beginning of the while loop and setting pktsz and dhcpsz
> to 0. Both approaches work for me, but I don't know which is
> better/correct.
>
Good catch, I'll revert that part.
Thanks.
-ml
> Index: virtio.c
> ===
> RCS file: /var/cvs/src/usr.sbin/vmd/virtio.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 virtio.c
> --- virtio.c 22 Jan 2019 10:12:36 - 1.77
> +++ virtio.c 23 Sep 2019 06:28:43 -
> @@ -1533,6 +1533,9 @@ vionet_notify_tx(struct vionet_dev *dev)
> num_enq++;
>
> idx = dev->vq[TXQ].last_avail & VIONET_QUEUE_MASK;
> +
> + free(pkt);
> + pkt = NULL;
> }
>
> if (write_mem(q_gpa, vr, vr_sz)) {
>
Re: vmd(8): fix memory leak in virtio network TX path
On Sun, Sep 22, 2019 at 02:42:28AM -0700, Mike Larkin wrote:
> We allocate a 'pkt' for each network packet in the queue, but only were
> freeing the last one. This has always been a bug, but it looks like recent
> changes elsewhere in the network stack may have made the problem more apparent
> since more packets seem to be deposited in the queue for each TX operation
> now.
>
> This was reported back in July by Paul de Weerd, who told me this could be
> reproduced by doing iperf tests between a VM and the host. He and I have
> tested that this diff seems to resolve the problem.
>
> There is a similar leak with the built-in vmd(8) DHCP service's packets.
> Both are fixed below. Each pointer is reset to NULL so that the final
> free at the end of the function doesn't double free the last buffer(s).
That doesn't look right as dhcppkt will be used after the while loop:
1542 if (dhcpsz > 0) {
1543 if (vionet_enq_rx(dev, dhcppkt, dhcpsz, ))
1544 ret = 1;
1545 }
I've tried this diff with a local interface and indeed it broke dhcp.
Note that dhcppkt is only allocated once as afterwards dhcpsz > 0 will
prevent further calls to dhcp_request(), so I'm not sure we should touch
dhcppkt at all.
An alternative to the diff below would be to free and null out pkt and
dhcpkt at the beginning of the while loop and setting pktsz and dhcpsz
to 0. Both approaches work for me, but I don't know which is
better/correct.
Index: virtio.c
===
RCS file: /var/cvs/src/usr.sbin/vmd/virtio.c,v
retrieving revision 1.77
diff -u -p -r1.77 virtio.c
--- virtio.c22 Jan 2019 10:12:36 - 1.77
+++ virtio.c23 Sep 2019 06:28:43 -
@@ -1533,6 +1533,9 @@ vionet_notify_tx(struct vionet_dev *dev)
num_enq++;
idx = dev->vq[TXQ].last_avail & VIONET_QUEUE_MASK;
+
+ free(pkt);
+ pkt = NULL;
}
if (write_mem(q_gpa, vr, vr_sz)) {
Re: vmd(8): fix memory leak in virtio network TX path
ok benno@
Mike Larkin(mlar...@azathoth.net) on 2019.09.22 02:42:28 -0700:
> We allocate a 'pkt' for each network packet in the queue, but only were
> freeing the last one. This has always been a bug, but it looks like recent
> changes elsewhere in the network stack may have made the problem more apparent
> since more packets seem to be deposited in the queue for each TX operation
> now.
>
> This was reported back in July by Paul de Weerd, who told me this could be
> reproduced by doing iperf tests between a VM and the host. He and I have
> tested that this diff seems to resolve the problem.
>
> There is a similar leak with the built-in vmd(8) DHCP service's packets.
> Both are fixed below. Each pointer is reset to NULL so that the final
> free at the end of the function doesn't double free the last buffer(s).
>
> ok?
>
> -ml
>
> Index: virtio.c
> ===
> RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v
> retrieving revision 1.77
> diff -u -p -a -u -r1.77 virtio.c
> --- virtio.c 22 Jan 2019 10:12:36 - 1.77
> +++ virtio.c 22 Sep 2019 07:45:08 -
> @@ -1533,6 +1533,12 @@ vionet_notify_tx(struct vionet_dev *dev)
> num_enq++;
>
> idx = dev->vq[TXQ].last_avail & VIONET_QUEUE_MASK;
> +
> + free(pkt);
> + pkt = NULL;
> +
> + free(dhcppkt);
> + dhcppkt = NULL;
> }
>
> if (write_mem(q_gpa, vr, vr_sz)) {
>
vmd(8): fix memory leak in virtio network TX path
We allocate a 'pkt' for each network packet in the queue, but only were
freeing the last one. This has always been a bug, but it looks like recent
changes elsewhere in the network stack may have made the problem more apparent
since more packets seem to be deposited in the queue for each TX operation
now.
This was reported back in July by Paul de Weerd, who told me this could be
reproduced by doing iperf tests between a VM and the host. He and I have
tested that this diff seems to resolve the problem.
There is a similar leak with the built-in vmd(8) DHCP service's packets.
Both are fixed below. Each pointer is reset to NULL so that the final
free at the end of the function doesn't double free the last buffer(s).
ok?
-ml
Index: virtio.c
===
RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v
retrieving revision 1.77
diff -u -p -a -u -r1.77 virtio.c
--- virtio.c22 Jan 2019 10:12:36 - 1.77
+++ virtio.c22 Sep 2019 07:45:08 -
@@ -1533,6 +1533,12 @@ vionet_notify_tx(struct vionet_dev *dev)
num_enq++;
idx = dev->vq[TXQ].last_avail & VIONET_QUEUE_MASK;
+
+ free(pkt);
+ pkt = NULL;
+
+ free(dhcppkt);
+ dhcppkt = NULL;
}
if (write_mem(q_gpa, vr, vr_sz)) {
