Message-ID: <671d9bbb-0f19-2710-00ef-47734085dddc@redhat.com>
Date:   Tue, 31 Jan 2023 09:47:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH v1] sparc/mm: don't unconditionally set HW writable bit
 when setting PTE dirty on 64bit
Content-Language: en-US
To:     linux-kernel@vger.kernel.org
Cc:     linux-mm@kvack.org, sparclinux@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        "David S. Miller" <davem@davemloft.net>,
        Peter Xu <peterx@redhat.com>, Hev <r@hev.cc>,
        Anatoly Pugachev <matorola@gmail.com>,
        Raghavendra K T <raghavendra.kt@amd.com>,
        Thorsten Leemhuis <regressions@leemhuis.info>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Juergen Gross <jgross@suse.com>
References: <20221212130213.136267-1-david@redhat.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <20221212130213.136267-1-david@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12.12.22 14:02, David Hildenbrand wrote:
> On sparc64, there is no HW modified bit, therefore, SW tracks via a SW
> bit if the PTE is dirty via pte_mkdirty(). However, pte_mkdirty()
> currently also unconditionally sets the HW writable bit, which is wrong.
> 
> pte_mkdirty() is not supposed to make a PTE actually writable, unless the
> SW writable bit (pte_write()) indicates that the PTE is not
> write-protected. Fortunately, sparc64 also defines a SW writable bit.
> 
> For example, this already turned into a problem in the context of
> THP splitting as documented in commit 624a2c94f5b7 ("Partly revert "mm/thp:
> carry over dirty bit when thp splits on pmd") and might be an issue during
> page migration in mm/migrate.c:remove_migration_pte() as well where we:
> 	if (folio_test_dirty(folio) && is_migration_entry_dirty(entry))
> 		pte = pte_mkdirty(pte);
> 
> But more general, anything like:
> 	maybe_mkwrite(pte_mkdirty(pte), vma)
> code is broken on sparc64, because it will unconditionally set the HW
> writable bit even if the SW writable bit is not set.
> 
> Simple reproducer that will result in a writable PTE after ptrace
> access, to highlight the problem and as an easy way to verify if it has
> been fixed:
> 
> --------------------------------------------------------------------------
>   #include <fcntl.h>
>   #include <signal.h>
>   #include <unistd.h>
>   #include <string.h>
>   #include <errno.h>
>   #include <stdlib.h>
>   #include <sys/mman.h>
> 
>   static void signal_handler(int sig)
>   {
>           if (sig == SIGSEGV)
>                   printf("[PASS] SIGSEGV generated\n");
>           else
>                   printf("[FAIL] wrong signal generated\n");
>           exit(0);
>   }
> 
>   int main(void)
>   {
>           size_t pagesize = getpagesize();
>           char data = 1;
>           off_t offs;
>           int mem_fd;
>           char *map;
>           int ret;
> 
>           mem_fd = open("/proc/self/mem", O_RDWR);
>           if (mem_fd < 0) {
>                   fprintf(stderr, "open(/proc/self/mem) failed: %d\n", errno);
>                   return 1;
>           }
> 
>           map = mmap(NULL, pagesize, PROT_READ, MAP_PRIVATE|MAP_ANON, -1 ,0);
>           if (map == MAP_FAILED) {
>                   fprintf(stderr, "mmap() failed: %d\n", errno);
>                   return 1;
>           }
> 
>           printf("original: %x\n", *map);
> 
>           /* debug access */
>           offs = lseek(mem_fd, (uintptr_t) map, SEEK_SET);
>           ret = write(mem_fd, &data, 1);
>           if (ret != 1) {
>                   fprintf(stderr, "pwrite(/proc/self/mem) failed with %d: %d\n", ret, errno);
>                   return 1;
>           }
>           if (*map != data) {
>                   fprintf(stderr, "pwrite(/proc/self/mem) not visible\n");
>                   return 1;
>           }
> 
>           printf("ptrace: %x\n", *map);
> 
>           /* Install signal handler. */
>           if (signal(SIGSEGV, signal_handler) == SIG_ERR) {
>                   fprintf(stderr, "signal() failed\n");
>                   return 1;
>           }
> 
>           /* Ordinary access. */
>           *map = 2;
> 
>           printf("access: %x\n", *map);
> 
>           printf("[FAIL] SIGSEGV not generated\n");
> 
>           return 0;
>   }
> --------------------------------------------------------------------------
> 
> Without this commit (sun4u in QEMU):
> 	# ./reproducer
> 	original: 0
> 	ptrace: 1
> 	access: 2
> 	[FAIL] SIGSEGV not generated
> 
> Let's fix this by setting the HW writable bit only if both, the SW dirty
> bit and the SW writable bit are set. This matches, for example, how
> s390x handles pte_mkwrite() and pte_mkdirty() -- except, that they have
> to clear the _PAGE_PROTECT bit.
> 
> We have to move pte_dirty() and pte_dirty() up. The code patching
> mechanism and handling constants > 22bit is a bit special on sparc64.
> 
> With this commit (sun4u in QEMU):
> 	# ./reproducer
> 	original: 0
> 	ptrace: 1
> 	[PASS] SIGSEGV generated
> 
> This handling seems to have been in place forever.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Hev <r@hev.cc>
> Cc: Anatoly Pugachev <matorola@gmail.com>
> Cc: Raghavendra K T <raghavendra.kt@amd.com>
> Cc: Thorsten Leemhuis <regressions@leemhuis.info>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Juergen Gross <jgross@suse.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

Ping

-- 
Thanks,

David / dhildenb