Re: [PATCH v4] kernel/resource: Fix locking in request_free_mem_region

From: David Hildenbrand <david@redhat.com>
To: Dan Williams <dan.j.williams@intel.com>,
	Alistair Popple <apopple@nvidia.com>
Cc: "Andrew Morton" <akpm@linux-foundation.org>,
	"Linux MM" <linux-mm@kvack.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Daniel Vetter" <daniel.vetter@ffwll.ch>,
	"Greg KH" <gregkh@linuxfoundation.org>,
	"John Hubbard" <jhubbard@nvidia.com>,
	"Jérôme Glisse" <jglisse@redhat.com>,
	"Balbir Singh" <bsingharora@gmail.com>,
	"Muchun Song" <smuchun@gmail.com>,
	"kernel test robot" <oliver.sang@intel.com>
Subject: Re: [PATCH v4] kernel/resource: Fix locking in request_free_mem_region
Date: Fri, 16 Apr 2021 10:18:02 +0200	[thread overview]
Message-ID: <147cc510-9c00-1ffe-bd02-60042fccf2c7@redhat.com> (raw)
In-Reply-To: <CAPcyv4grDqR09QWv68sJY=AXMDom5MSj40nOHaE+nKP9d9qc+g@mail.gmail.com>

On 16.04.21 06:19, Dan Williams wrote:
> On Thu, Apr 15, 2021 at 7:58 PM Alistair Popple <apopple@nvidia.com> wrote:
>>
>> request_free_mem_region() is used to find an empty range of physical
>> addresses for hotplugging ZONE_DEVICE memory. It does this by iterating
>> over the range of possible addresses using region_intersects() to see if
>> the range is free.
>>
>> region_intersects() obtains a read lock before walking the resource tree
>> to protect against concurrent changes. However it drops the lock prior
>> to returning. This means by the time request_mem_region() is called in
>> request_free_mem_region() another thread may have already reserved the
>> requested region resulting in unexpected failures and a message in the
>> kernel log from hitting this condition:
>>
>>          /*
>>           * mm/hmm.c reserves physical addresses which then
>>           * become unavailable to other users.  Conflicts are
>>           * not expected.  Warn to aid debugging if encountered.
>>           */
>>          if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) {
>>                  pr_warn("Unaddressable device %s %pR conflicts with %pR",
>>                          conflict->name, conflict, res);
>>
>> To fix this create versions of region_intersects() and
>> request_mem_region() that allow the caller to take the appropriate lock
>> such that it may be held over the required calls.
>>
>> Instead of creating another version of devm_request_mem_region() that
>> doesn't take the lock open-code it to allow the caller to pre-allocate
>> the required memory prior to taking the lock.
>>
>> On some architectures and kernel configurations revoke_iomem() also
>> calls resource code so cannot be called with the resource lock held.
>> Therefore call it only after dropping the lock.
> 
> The patch is difficult to read because too many things are being
> changed at once, and the changelog seems to confirm that. Can you try
> breaking this down into a set of incremental changes? Not only will
> this ease review it will distribute any regressions over multiple
> bisection targets.
> 
> Something like:
> 
> * Refactor region_intersects() to allow external locking
> * Refactor __request_region() to allow external locking
> * Push revoke_iomem() down into...
> * Fix resource_lock usage in [devm_]request_free_mem_region()

+1


-- 
Thanks,

David / dhildenb