linux-kernel - Re: [PATCH v2] RDMA/umem: minor bug fix and cleanup in error handling paths

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: <bef8680b-acc5-9f13-f49e-8f36f1939387@nvidia.com>
Date:   Mon, 4 Mar 2019 15:11:05 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Ira Weiny <ira.weiny@...el.com>,
        Artemy Kovalyov <artemyko@...lanox.com>
CC:     "john.hubbard@...il.com" <john.hubbard@...il.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Doug Ledford <dledford@...hat.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
Subject: Re: [PATCH v2] RDMA/umem: minor bug fix and cleanup in error handling
 paths

On 3/3/19 8:55 AM, Ira Weiny wrote:
> On Sun, Mar 03, 2019 at 11:52:41AM +0200, Artemy Kovalyov wrote:
>>
>>
>> On 02/03/2019 21:44, Ira Weiny wrote:
>>>
>>> On Sat, Mar 02, 2019 at 12:24:35PM -0800, john.hubbard@...il.com wrote:
>>>> From: John Hubbard <jhubbard@...dia.com>
>>>>
>>>> ...
>>>> 3. Dead code removal: the check for (user_virt & ~page_mask)
>>>> is checking for a condition that can never happen,
>>>> because earlier:
>>>>
>>>>      user_virt = user_virt & page_mask;
>>>>
>>>> ...so, remove that entire phrase.
>>>>
>>>>   		bcnt -= min_t(size_t, npages << PAGE_SHIFT, bcnt);
>>>>   		mutex_lock(&umem_odp->umem_mutex);
>>>>   		for (j = 0; j < npages; j++, user_virt += PAGE_SIZE) {
>>>> -			if (user_virt & ~page_mask) {
>>>> -				p += PAGE_SIZE;
>>>> -				if (page_to_phys(local_page_list[j]) != p) {
>>>> -					ret = -EFAULT;
>>>> -					break;
>>>> -				}
>>>> -				put_page(local_page_list[j]);
>>>> -				continue;
>>>> -			}
>>>> -
>>>
>>> I think this is trying to account for compound pages. (ie page_mask could
>>> represent more than PAGE_SIZE which is what user_virt is being incrimented by.)
>>> But putting the page in that case seems to be the wrong thing to do?
>>>
>>> Yes this was added by Artemy[1] now cc'ed.
>>
>> Right, this is for huge pages, please keep it.
>> put_page() needed to decrement refcount of the head page.
> 
> You mean decrement the refcount of the _non_-head pages?
> 
> Ira
> 

Actually, I'm sure Artemy means head page, because put_page() always
operates on the head page. 

And this reminds me that I have a problem to solve nearby: get_user_pages
on huge pages increments the page->_refcount *for each tail page* as well.
That's a minor problem for my put_user_page() 
patchset, because my approach so far assumed that I could just change us
over to:

get_user_page(): increments page->_refcount by a large amount (1024)

put_user_page(): decrements page->_refcount by a large amount (1024)

...and just stop doing the odd (to me) technique of incrementing once for
each tail page. I cannot see any reason why that's actually required, as
opposed to just "raise the page->_refcount enough to avoid losing the head
page too soon".

However, it may be tricky to do this in one pass. Probably at first, I'll have
to do this horrible thing approach:

get_user_page(): increments page->_refcount by a large amount (1024)

put_user_page(): decrements page->_refcount by a large amount (1024) MULTIPLIED
                 by the number of tail pages. argghhh that's ugly.

thanks,
-- 
John Hubbard
NVIDIA