Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1b574e20-26f3-fb3e-69d2-21f35c9e4730@redhat.com>
Date:   Mon, 25 Jul 2022 16:11:19 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v3 1/3] mm/mprotect: Fix soft-dirty check in
 can_change_pte_writable()
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>, Nadav Amit <nadav.amit@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, Linux MM <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>
References: <20220721183338.27871-1-peterx@redhat.com>
 <20220721183338.27871-2-peterx@redhat.com>
 <e35e42ce-e942-141d-09e7-a3a7868f4abb@redhat.com>
 <C2910936-FDCF-4ECF-B014-D985284B225A@gmail.com>
 <Yt6htg03lqj7FFU2@xz-m1.local>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <Yt6htg03lqj7FFU2@xz-m1.local>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 25.07.22 15:59, Peter Xu wrote:
> On Fri, Jul 22, 2022 at 10:21:00AM -0700, Nadav Amit wrote:
>> On Jul 22, 2022, at 12:08 AM, David Hildenbrand <david@redhat.com> wrote:
>>
>>>> +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);
>>>> +}
>>>
>>> That will come in handy in other patches I'm cooking.
>>
>> clear_refs_write() also comes to mind as well (for consistency; I see no
>> correctness or performance issue).
> 
> I explicitly didn't touch that because current code is better..
> 
>         mas_for_each(&mas, vma, ULONG_MAX) {
>                 if (!(vma->vm_flags & VM_SOFTDIRTY))
>                         continue;
>                 vma->vm_flags &= ~VM_SOFTDIRTY;
>                 vma_set_page_prot(vma);
>         }
> 
> It means when !CONFIG_MEM_SOFT_DIRTY the "if" will always be true and all
> vma will be jumped.
> 
> If replaced with vma_soft_dirty_enabled() it'll be instead constantly false
> returned.  We'll redo vma_set_page_prot() even if unnecessary.
> 
> Here if we want to add the "CONFIG_MEM_SOFT_DIRTY" into equation it can be:
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index f8cd58846a28..ab6f2913b5a5 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1290,6 +1290,8 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
>                 }
>  
>                 if (type == CLEAR_REFS_SOFT_DIRTY) {
> +                       if (!IS_ENABLED(CONFIG_MEM_SOFT_DIRTY))
> +                               goto out_unlock;
>                         mas_for_each(&mas, vma, ULONG_MAX) {
>                                 if (!(vma->vm_flags & VM_SOFTDIRTY))
>                                         continue;
> 
> Or even at the entrance to not take the mm sem.  But it's not anything
> important IMHO, so if no one asking for that I'll just leave it be.


Yeah, I don't think we particularly care about that.


-- 
Thanks,

David / dhildenb