linux-kernel - Re: [PATCH] xen-netback: fix memory leaks on XenBus disconnect

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2447fe0c-cfae-a77b-2468-db22bb709163@citrix.com>
Date:   Fri, 13 Jan 2017 11:06:33 +0000
From:   Igor Druzhinin <igor.druzhinin@...rix.com>
To:     Wei Liu <wei.liu2@...rix.com>
CC:     <xen-devel@...ts.xenproject.org>, <paul.durrant@...rix.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] xen-netback: fix memory leaks on XenBus disconnect

On 13/01/17 10:38, Wei Liu wrote:
> On Thu, Jan 12, 2017 at 05:51:56PM +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.
>> vif->lock is used to protect statistics gathering agents from using the
>> queue structure during cleaning.
>>
> 
> There is code in netback_remove which eventually calls xenvif_free to
> free up the resources, maybe you should modify xenvif_free instead? That
> seems more symmetric to me. What do you think?
> 
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@...rix.com>
>> ---
>>  drivers/net/xen-netback/interface.c |  6 ++++--
>>  drivers/net/xen-netback/xenbus.c    | 13 +++++++++++++
>>  2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
>> index e30ffd2..5795213 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -221,18 +221,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;
>> @@ -241,6 +241,8 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev)
>>  	}
>>  
>>  out:
>> +	spin_unlock(&vif->lock);
>> +
> 
> Good catch, this is definitely needed. And it would probably be in a
> separate patch.

I suggest we also need to have this spinlock acquired in another part
of cleaning code (xenvif_free). The reason to introduce it is a locking
practice in openvswitch when they don't grab any network subsystem locks
(rtnl_lock) in order to gather the statistics. I'm not sure that it's
correct behavior. Anyone can advise?

> Wei.
>