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

[David Hildenbrand]

On 16.04.21 06:19, Dan Williams wrote:
> On Thu, Apr 15, 2021 at 7:58 PM Alistair Popple <apopple@xxxxxxxxxx> 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