linux-kernel - RE: [EXTERNAL] Re: [RFC PATCH 3/4] Hyper-V/Balloon: Call add_memory() with dm_device.ha

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 12 Dec 2019 08:24:18 +0000
From:   Tianyu Lan <Tianyu.Lan@...rosoft.com>
To:     vkuznets <vkuznets@...hat.com>,
        "lantianyu1986@...il.com" <lantianyu1986@...il.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "sashal@...nel.org" <sashal@...nel.org>,
        Michael Kelley <mikelley@...rosoft.com>
CC:     "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "eric.devolder@...cle.com" <eric.devolder@...cle.com>
Subject: RE: [EXTERNAL] Re: [RFC PATCH 3/4] Hyper-V/Balloon: Call add_memory()
 with dm_device.ha_lock.

Hi Vitaly:
	Thanks for your review.
> From: Vitaly Kuznetsov <vkuznets@...hat.com> 
> > From: Tianyu Lan <Tianyu.Lan@...rosoft.com>
> > @@ -771,8 +767,13 @@ static void hv_online_page(struct page *pg,
> unsigned int order)
> >  	struct hv_hotadd_state *has;
> >  	unsigned long flags;
> >  	unsigned long pfn = page_to_pfn(pg);
> > +	int unlocked;
> > +
> > +	if (dm_device.lock_thread != current) {
> 
> With lock_thread checking you're trying to protect against taking the spinlock
> twice (when this is called from add_memory()) but why not just check that
> spin_is_locked() AND we sit on the same CPU as the VMBus channel
> attached to the balloon device?

Yes, that's another approach.
> 
> > +		spin_lock_irqsave(&dm_device.ha_lock, flags);
> > +		unlocked = 1;
> > +	}
> 
> We set unlocked to '1' when we're actually locked, aren't we?

The "unlocked" means ha_lock isn't hold before calling hv_online_page().

> 
> >
> > -	spin_lock_irqsave(&dm_device.ha_lock, flags);
> >  	list_for_each_entry(has, &dm_device.ha_region_list, list) {
> >  		/* The page belongs to a different HAS. */
> >  		if ((pfn < has->start_pfn) ||
> > @@ -782,7 +783,9 @@ static void hv_online_page(struct page *pg,
> unsigned int order)
> >  		hv_bring_pgs_online(has, pfn, 1UL << order);
> >  		break;
> >  	}
> > -	spin_unlock_irqrestore(&dm_device.ha_lock, flags);
> > +
> > +	if (unlocked)
> > +		spin_unlock_irqrestore(&dm_device.ha_lock, flags);
> >  }
> >
> >  static int pfn_covered(unsigned long start_pfn, unsigned long
> > pfn_cnt) @@ -860,6 +863,7 @@ static unsigned long
> handle_pg_range(unsigned long pg_start,
> >  		pg_start);
> >
> >  	spin_lock_irqsave(&dm_device.ha_lock, flags);
> > +	dm_device.lock_thread = current;
> >  	list_for_each_entry(has, &dm_device.ha_region_list, list) {
> >  		/*
> >  		 * If the pfn range we are dealing with is not in the current
> @@
> > -912,9 +916,7 @@ static unsigned long handle_pg_range(unsigned long
> pg_start,
> >  			} else {
> >  				pfn_cnt = size;
> >  			}
> > -			spin_unlock_irqrestore(&dm_device.ha_lock, flags);
> >  			hv_mem_hot_add(has->ha_end_pfn, size, pfn_cnt,
> has);
> > -			spin_lock_irqsave(&dm_device.ha_lock, flags);
> 
> Apart from the deadlock you mention in the commit message, add_memory
> does lock_device_hotplug()/unlock_device_hotplug() which is a mutex. If
> I'm not mistaken you now take the mutext under a spinlock
> (&dm_device.ha_lock). Not good.

Yes, you are right. I missed this. I will try other way. Nice catch. Thanks.

> 
> 
> >  		}
> >  		/*
> >  		 * If we managed to online any pages that were given to us,
> @@
> > -923,6 +925,7 @@ static unsigned long handle_pg_range(unsigned long
> pg_start,
> >  		res = has->covered_end_pfn - old_covered_state;
> >  		break;
> >  	}
> > +	dm_device.lock_thread = NULL;
> >  	spin_unlock_irqrestore(&dm_device.ha_lock, flags);
> >
> >  	return res;
> 
> --
> Vitaly