linux-kernel - Re: [PATCH v4 1/3] mm/mprotect: Fix soft-dirty check in can_change_pte

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y3gRy8pUiYWFGqcI@x1n>
Date:   Fri, 18 Nov 2022 18:14:19 -0500
From:   Peter Xu <peterx@...hat.com>
To:     Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc:     David Hildenbrand <david@...hat.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v4 1/3] mm/mprotect: Fix soft-dirty check in
 can_change_pte_writable()

On Sat, Nov 19, 2022 at 01:16:26AM +0500, Muhammad Usama Anjum wrote:
> Hi Peter and David,

Hi, Muhammad,

> 
> On 7/25/22 7:20 PM, Peter Xu wrote:
> > The check wanted to make sure when soft-dirty tracking is enabled we won't
> > grant write bit by accident, as a page fault is needed for dirty tracking.
> > The intention is correct but we didn't check it right because VM_SOFTDIRTY
> > set actually means soft-dirty tracking disabled.  Fix it.
> [...]
> > +static inline bool vma_soft_dirty_enabled(struct vm_area_struct *vma)
> > +{
> > +	/*
> > +	 * NOTE: we must check this before VM_SOFTDIRTY on soft-dirty
> > +	 * enablements, because when without soft-dirty being compiled in,
> > +	 * VM_SOFTDIRTY is defined as 0x0, then !(vm_flags & VM_SOFTDIRTY)
> > +	 * will be constantly true.
> > +	 */
> > +	if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
> > +		return false;
> > +
> > +	/*
> > +	 * Soft-dirty is kind of special: its tracking is enabled when the
> > +	 * vma flags not set.
> > +	 */
> > +	return !(vma->vm_flags & VM_SOFTDIRTY);
> > +}
> I'm sorry. I'm unable to understand the inversion here.
> > its tracking is enabled when the vma flags not set.
> VM_SOFTDIRTY is set on the VMA when new VMA is allocated to mark is
> soft-dirty. When we write to clear_refs to clear soft-dirty bit,
> VM_SOFTDIRTY is cleared from the VMA as well. Then why do you say tracking
> is enabled when the vma flags not set?

Because only when 4>clear_refs happens would VM_SOFTDIRTY be cleared, and
only until then the real tracking starts (by removing write bits on ptes).

> I'm missing some obvious thing.  Maybe the meaning of tracking is to see
> if VM_SOFTDIRTY needs to be set. If VM_SOFTDIRTY is already set, tracking
> isn't needed. Can you give an example here?

If VM_SOFTDIRTY is set, pagemap will treat all pages as soft-dirty, please
see pagemap_pmd_range():

		if (vma->vm_flags & VM_SOFTDIRTY)
			flags |= PM_SOFT_DIRTY;

So fundamentally it reports nothing useful when VM_SOFTDIRTY set.  That's
also why we need the clear_refs first before we can have anything useful.

Feel free to reference to the doc page (admin-guide/mm/soft-dirty.rst):

---8<---
The soft-dirty is a bit on a PTE which helps to track which pages a task
writes to. In order to do this tracking one should

  1. Clear soft-dirty bits from the task's PTEs.

     This is done by writing "4" into the ``/proc/PID/clear_refs`` file of the
     task in question.

  2. Wait some time.

  3. Read soft-dirty bits from the PTEs.

     This is done by reading from the ``/proc/PID/pagemap``. The bit 55 of the
     64-bit qword is the soft-dirty one. If set, the respective PTE was
     written to since step 1.
---8<---

The tracking starts at step 1, where is when the flag is cleared.

Thanks,

-- 
Peter Xu