linux-kernel - Re: [PATCH] smaps: Fix the abnormal memory statistics obtained through /proc/pid/smaps

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: <5a2c9ae4-50f5-3301-3b50-f57026e1f8e8@redhat.com>
Date:   Thu, 27 Jul 2023 19:27:02 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     liubo <liubo254@...wei.com>, akpm@...ux-foundation.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        hughd@...gle.com, willy@...radead.org
Subject: Re: [PATCH] smaps: Fix the abnormal memory statistics obtained
 through /proc/pid/smaps

>>
>> This was wrong from the very start. If we're not in GUP, we shouldn't call
>> GUP functions.
> 
> My understanding is !GET && !PIN is also called gup.. otherwise we don't
> need GET and it can just be always implied.

That's not the point. The point is that _arbitrary_ code shouldn't call 
into GUP internal helper functions, where they bypass, for example, any 
sanity checks.

> 
> The other proof is try_grab_page() doesn't fail hard on !GET && !PIN.  So I
> don't know whether that's "wrong" to be used..
> 

To me, that is arbitrary code using a GUP internal helper and, 
therefore, wrong.

> Back to the topic: I'd say either of the patches look good to solve the
> problem.  If p2pdma pages are mapped as PFNMAP/MIXEDMAP (?), I guess
> vm_normal_page_pmd() proposed here will also work on it, so nothing I see
> wrong on 2nd one yet.
> 
> It looks nicer indeed to not have FOLL_FORCE here, but it also makes me
> just wonder whether we should document NUMA behavior for FOLL_* somewhere,
> because we have an implication right now on !FOLL_FORCE over NUMA, which is
> not obvious to me..

Yes, we probably should. For get_use_pages() and friends that behavior 
was always like that and it makes sense: usually it represent 
application behavior.

> 
> And to look more over that aspect, see follow_page(): previously we can
> follow a page for protnone (as it never applies FOLL_NUMA) but now it won't
> (it never applies FOLL_FORCE, either, so it seems "accidentally" implies
> FOLL_NUMA now).  Not sure whether it's intended, though..

That was certainly an oversight, thanks for spotting that. That patch 
was not supposed to change semantics:

diff --git a/mm/gup.c b/mm/gup.c
index 76d222ccc3ff..ac926e19ff72 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -851,6 +851,13 @@ struct page *follow_page(struct vm_area_struct 
*vma, unsigned long address,
         if (WARN_ON_ONCE(foll_flags & FOLL_PIN))
                 return NULL;

+       /*
+        * In contrast to get_user_pages() and friends, we don't want to
+        * fail if the PTE is PROT_NONE: see gup_can_follow_protnone().
+        */
+       if (!(foll_flags & FOLL_WRITE))
+               foll_flags |= FOLL_FORCE;
+
         page = follow_page_mask(vma, address, foll_flags, &ctx);
         if (ctx.pgmap)
                 put_dev_pagemap(ctx.pgmap);


-- 
Cheers,

David / dhildenb