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: <b75653b8-5264-ca03-bf5c-671e07e7fdd8@collabora.com>
Date:   Sat, 19 Nov 2022 01:16:26 +0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Cc:     Muhammad Usama Anjum <usama.anjum@collabora.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v4 1/3] mm/mprotect: Fix soft-dirty check in
 can_change_pte_writable()
Content-Language: en-US
To:     Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>
References: <20220725142048.30450-1-peterx@redhat.com>
 <20220725142048.30450-2-peterx@redhat.com>
From:   Muhammad Usama Anjum <usama.anjum@collabora.com>
In-Reply-To: <20220725142048.30450-2-peterx@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Peter and David,

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? 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?

-- 
BR,
Muhammad Usama Anjum