[PATCH v2 0/2] xen-netback: fix memory leaks on XenBus disconnect

[PATCH v2 0/2] xen-netback: fix memory leaks on XenBus disconnect

[Igor Druzhinin]

Just split the initial patch in two as proposed by Wei.

Since the approach for locking netdev statistics is inconsistent (tends not
to have any locking at all) accross the kernel we'd better to rely on our
internal lock for this purpose.

Igor Druzhinin (2):
  xen-netback: fix memory leaks on XenBus disconnect
  xen-netback: protect resource cleaning on XenBus disconnect

 drivers/net/xen-netback/interface.c |  6 ++++--
 drivers/net/xen-netback/xenbus.c    | 13 +++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] xen-netback: fix memory leaks on XenBus disconnect

[Igor Druzhinin]

Eliminate memory leaks introduced several years ago by cleaning the
queue resources which are allocated on XenBus connection event. Namely, queue
structure array and pages used for IO rings.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 drivers/net/xen-netback/xenbus.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 6c57b02..3e99071 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -493,11 +493,20 @@ static int backend_create_xenvif(struct backend_info *be)
 static void backend_disconnect(struct backend_info *be)
 {
 	if (be->vif) {
+		unsigned int queue_index;
+
 		xen_unregister_watchers(be->vif);
 #ifdef CONFIG_DEBUG_FS
 		xenvif_debugfs_delif(be->vif);
 #endif /* CONFIG_DEBUG_FS */
 		xenvif_disconnect_data(be->vif);
+		for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
+			xenvif_deinit_queue(&be->vif->queues[queue_index]);
+
+		vfree(be->vif->queues);
+		be->vif->num_queues = 0;
+		be->vif->queues = NULL;
+
 		xenvif_disconnect_ctrl(be->vif);
 	}
 }
@@ -1026,6 +1035,8 @@ static void connect(struct backend_info *be)
 err:
 	if (be->vif->num_queues > 0)
 		xenvif_disconnect_data(be->vif); /* Clean up existing queues */
+	for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
+		xenvif_deinit_queue(&be->vif->queues[queue_index]);
 	vfree(be->vif->queues);
 	be->vif->queues = NULL;
 	be->vif->num_queues = 0;
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

RE: [PATCH v2 1/2] xen-netback: fix memory leaks on XenBus disconnect

[Paul Durrant]

> -----Original Message-----
> From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> Sent: 17 January 2017 20:50
> To: Wei Liu <wei.liu2@citrix.com>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Igor Druzhinin
> <igor.druzhinin@citrix.com>
> Subject: [PATCH v2 1/2] xen-netback: fix memory leaks on XenBus
> disconnect
> 
> Eliminate memory leaks introduced several years ago by cleaning the
> queue resources which are allocated on XenBus connection event. Namely,
> queue
> structure array and pages used for IO rings.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  drivers/net/xen-netback/xenbus.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
> netback/xenbus.c
> index 6c57b02..3e99071 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -493,11 +493,20 @@ static int backend_create_xenvif(struct
> backend_info *be)
>  static void backend_disconnect(struct backend_info *be)
>  {
>  	if (be->vif) {
> +		unsigned int queue_index;
> +
>  		xen_unregister_watchers(be->vif);
>  #ifdef CONFIG_DEBUG_FS
>  		xenvif_debugfs_delif(be->vif);
>  #endif /* CONFIG_DEBUG_FS */
>  		xenvif_disconnect_data(be->vif);
> +		for (queue_index = 0; queue_index < be->vif-
> >num_queues; ++queue_index)
> +			xenvif_deinit_queue(&be->vif-
> >queues[queue_index]);
> +
> +		vfree(be->vif->queues);
> +		be->vif->num_queues = 0;
> +		be->vif->queues = NULL;
> +
>  		xenvif_disconnect_ctrl(be->vif);
>  	}
>  }
> @@ -1026,6 +1035,8 @@ static void connect(struct backend_info *be)
>  err:
>  	if (be->vif->num_queues > 0)
>  		xenvif_disconnect_data(be->vif); /* Clean up existing
> queues */
> +	for (queue_index = 0; queue_index < be->vif->num_queues;
> ++queue_index)
> +		xenvif_deinit_queue(&be->vif->queues[queue_index]);
>  	vfree(be->vif->queues);
>  	be->vif->queues = NULL;
>  	be->vif->num_queues = 0;
> --
> 1.8.3.1

Re: [PATCH v2 1/2] xen-netback: fix memory leaks on XenBus disconnect

[Wei Liu]

On Tue, Jan 17, 2017 at 08:49:37PM +0000, Igor Druzhinin wrote:
> Eliminate memory leaks introduced several years ago by cleaning the
> queue resources which are allocated on XenBus connection event. Namely, queue
> structure array and pages used for IO rings.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/2] xen-netback: protect resource cleaning on XenBus disconnect

[Igor Druzhinin]

vif->lock is used to protect statistics gathering agents from using the
queue structure during cleaning.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
 drivers/net/xen-netback/interface.c | 6 ++++--
 drivers/net/xen-netback/xenbus.c    | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 41c69b3..c48252a 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -230,18 +230,18 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev)
 {
 	struct xenvif *vif = netdev_priv(dev);
 	struct xenvif_queue *queue = NULL;
-	unsigned int num_queues = vif->num_queues;
 	unsigned long rx_bytes = 0;
 	unsigned long rx_packets = 0;
 	unsigned long tx_bytes = 0;
 	unsigned long tx_packets = 0;
 	unsigned int index;
 
+	spin_lock(&vif->lock);
 	if (vif->queues == NULL)
 		goto out;
 
 	/* Aggregate tx and rx stats from each queue */
-	for (index = 0; index < num_queues; ++index) {
+	for (index = 0; index < vif->num_queues; ++index) {
 		queue = &vif->queues[index];
 		rx_bytes += queue->stats.rx_bytes;
 		rx_packets += queue->stats.rx_packets;
@@ -250,6 +250,8 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev)
 	}
 
 out:
+	spin_unlock(&vif->lock);
+
 	vif->dev->stats.rx_bytes = rx_bytes;
 	vif->dev->stats.rx_packets = rx_packets;
 	vif->dev->stats.tx_bytes = tx_bytes;
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 3e99071..d82cd71 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -503,9 +503,11 @@ static void backend_disconnect(struct backend_info *be)
 		for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
 			xenvif_deinit_queue(&be->vif->queues[queue_index]);
 
+		spin_lock(&be->vif->lock);
 		vfree(be->vif->queues);
 		be->vif->num_queues = 0;
 		be->vif->queues = NULL;
+		spin_unlock(&be->vif->lock);
 
 		xenvif_disconnect_ctrl(be->vif);
 	}
-- 
1.8.3.1

Re: [PATCH v2 2/2] xen-netback: protect resource cleaning on XenBus disconnect

[Wei Liu]

On Tue, Jan 17, 2017 at 08:49:38PM +0000, Igor Druzhinin wrote:
> vif->lock is used to protect statistics gathering agents from using the
> queue structure during cleaning.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] xen-netback: protect resource cleaning on XenBus disconnect

[Paul Durrant]

> -----Original Message-----
> From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> Sent: 17 January 2017 20:50
> To: Wei Liu <wei.liu2@citrix.com>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Igor Druzhinin
> <igor.druzhinin@citrix.com>
> Subject: [PATCH v2 2/2] xen-netback: protect resource cleaning on XenBus
> disconnect
> 
> vif->lock is used to protect statistics gathering agents from using the
> queue structure during cleaning.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  drivers/net/xen-netback/interface.c | 6 ++++--
>  drivers/net/xen-netback/xenbus.c    | 2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-
> netback/interface.c
> index 41c69b3..c48252a 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -230,18 +230,18 @@ static struct net_device_stats
> *xenvif_get_stats(struct net_device *dev)
>  {
>  	struct xenvif *vif = netdev_priv(dev);
>  	struct xenvif_queue *queue = NULL;
> -	unsigned int num_queues = vif->num_queues;
>  	unsigned long rx_bytes = 0;
>  	unsigned long rx_packets = 0;
>  	unsigned long tx_bytes = 0;
>  	unsigned long tx_packets = 0;
>  	unsigned int index;
> 
> +	spin_lock(&vif->lock);
>  	if (vif->queues == NULL)
>  		goto out;
> 
>  	/* Aggregate tx and rx stats from each queue */
> -	for (index = 0; index < num_queues; ++index) {
> +	for (index = 0; index < vif->num_queues; ++index) {
>  		queue = &vif->queues[index];
>  		rx_bytes += queue->stats.rx_bytes;
>  		rx_packets += queue->stats.rx_packets;
> @@ -250,6 +250,8 @@ static struct net_device_stats
> *xenvif_get_stats(struct net_device *dev)
>  	}
> 
>  out:
> +	spin_unlock(&vif->lock);
> +
>  	vif->dev->stats.rx_bytes = rx_bytes;
>  	vif->dev->stats.rx_packets = rx_packets;
>  	vif->dev->stats.tx_bytes = tx_bytes;
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
> netback/xenbus.c
> index 3e99071..d82cd71 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -503,9 +503,11 @@ static void backend_disconnect(struct backend_info
> *be)
>  		for (queue_index = 0; queue_index < be->vif-
> >num_queues; ++queue_index)
>  			xenvif_deinit_queue(&be->vif-
> >queues[queue_index]);
> 
> +		spin_lock(&be->vif->lock);
>  		vfree(be->vif->queues);
>  		be->vif->num_queues = 0;
>  		be->vif->queues = NULL;
> +		spin_unlock(&be->vif->lock);
> 
>  		xenvif_disconnect_ctrl(be->vif);
>  	}
> --
> 1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 0/2] xen-netback: fix memory leaks on XenBus disconnect

[David Miller]

From: Igor Druzhinin <igor.druzhinin@citrix.com>
Date: Tue, 17 Jan 2017 20:49:36 +0000

> Just split the initial patch in two as proposed by Wei.
> 
> Since the approach for locking netdev statistics is inconsistent (tends not
> to have any locking at all) accross the kernel we'd better to rely on our
> internal lock for this purpose.

Series applied, thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel