linux-kernel - Re: [PATCH v4] kernel/resource: Fix locking in request_free_mem

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]

Message-ID: <147cc510-9c00-1ffe-bd02-60042fccf2c7@redhat.com>
Date:   Fri, 16 Apr 2021 10:18:02 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Dan Williams <dan.j.williams@...el.com>,
        Alistair Popple <apopple@...dia.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Greg KH <gregkh@...uxfoundation.org>,
        John Hubbard <jhubbard@...dia.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Balbir Singh <bsingharora@...il.com>,
        Muchun Song <smuchun@...il.com>,
        kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH v4] kernel/resource: Fix locking in
 request_free_mem_region

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