From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2A1F2D2127B for ; Fri, 18 Oct 2024 16:24:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AB8FA6B0083; Fri, 18 Oct 2024 12:24:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A69376B0088; Fri, 18 Oct 2024 12:24:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 90A0E6B0096; Fri, 18 Oct 2024 12:24:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 6EBB76B0083 for ; Fri, 18 Oct 2024 12:24:31 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 9FE7F80349 for ; Fri, 18 Oct 2024 16:24:20 +0000 (UTC) X-FDA: 82687245690.16.7783CD8 Received: from mail-ot1-f43.google.com (mail-ot1-f43.google.com [209.85.210.43]) by imf30.hostedemail.com (Postfix) with ESMTP id E90C880012 for ; Fri, 18 Oct 2024 16:24:07 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=google header.b=YH9qS8xF; dmarc=pass (policy=none) header.from=linuxfoundation.org; spf=pass (imf30.hostedemail.com: domain of skhan@linuxfoundation.org designates 209.85.210.43 as permitted sender) smtp.mailfrom=skhan@linuxfoundation.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729268550; a=rsa-sha256; cv=none; b=TfLxzPHOUzJZjXCsAhqav9XHugmkzoGFSOPjGES6fmiyoORNuozurwvWcDUtEVTbNKXPcG nLVZTx+hGGTEfB3u2AytvpPAIQYRPrqrBYK8fuLiTPf23EBd55hVUb/yQxEQ0G+gLLo/pz R1XJswhgjQYC+SokXgsYNl4jAdb+aY8= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=google header.b=YH9qS8xF; dmarc=pass (policy=none) header.from=linuxfoundation.org; spf=pass (imf30.hostedemail.com: domain of skhan@linuxfoundation.org designates 209.85.210.43 as permitted sender) smtp.mailfrom=skhan@linuxfoundation.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729268550; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=F3v+T7PjpPKYcorK84vLoRIziLno8dgMRinbwlPayfE=; b=QTnAd6y9HOgxN8L9zyIt/W01QgQOVFsiTupp3ULjc0zbIZxkQ5qp+Cz9wb4hDBdazFjKpC m43Lz9xNu1PxIYvwku4sBXMfEbkgtf+BTDavUl0aT6EQEnCJqkrJS6O3lhkgMrmtUSLzJZ 6YAY6EHYECzPL19fZVjoPC6XY6fkCZk= Received: by mail-ot1-f43.google.com with SMTP id 46e09a7af769-71811c7eb8dso781298a34.0 for ; Fri, 18 Oct 2024 09:24:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linuxfoundation.org; s=google; t=1729268668; x=1729873468; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=F3v+T7PjpPKYcorK84vLoRIziLno8dgMRinbwlPayfE=; b=YH9qS8xFGghohxyJ2+bU3TBYI0h7YQxC3W9hKyXx1QDqtwc4J1f9HEzjnU6xVexN4H QXaDsXNiaTACLRy2XR0QUS8RN/vumpsosBJpO00tS58pp16CJ2UcFs53jJo6AGk73pHy 1Pv0AEzEjFQ6iVtm46X3O3iCY0KoLvpTM39dM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729268668; x=1729873468; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=F3v+T7PjpPKYcorK84vLoRIziLno8dgMRinbwlPayfE=; b=PHYF/foJA0bkIHQAoG2TrRqWAS5RMxTg2O41bjhxZfKj2/sZJ2xpkeJul91X0gl+U/ 2Kmvr5HD4mhlUPNpIHxN9FW2Kz7wCwV6xehNVMdbiIzP03L1l6g4VIUY6+tFzQOgb27G okbQZX8sURDdlrWa1dv7tHAQQXsAQ+pJMp8tE6Wi2KuFuxZSt0B3cKI2cPe9JJgIAika T0s2dU8RF+fvGFpY+obNBTJQum4AxPeYGdq9wjAH2xp2gRWU/TZ6dOG8CD2XbkEPQoMV RUgmfUYTrSyBwdDRAEsvzuVvTq8oc41/MJcUwkS+2biBeiShNMaScUXNUdMmJ11YfyfR u5vA== X-Forwarded-Encrypted: i=1; AJvYcCV54rYwdrhWCSDJeIwjDPOPTElAB7H9w3C5RMmCMPz6+nSCm6Eelt8kl5QdxQmicoAPVWO+BjU3kw==@kvack.org X-Gm-Message-State: AOJu0YwNnfUm8GiU8ztVw3BmGe2YFO4Ex0T1+hdnVmV/2jbtoqHIEs4N E7ErL0mrV2bdMIYqmArnrW+qtSCzCAd21B0qF1Mu75DNEU1fcipqTxzFQXg9PMk= X-Google-Smtp-Source: AGHT+IFIMl2GQFcOD4CAePzrCdIz3Ij+s6w3yxltIlvjQyisZktPRrklsesyLhvGbRcpIU0+2bu3ig== X-Received: by 2002:a05:6830:348a:b0:717:fb12:2c2 with SMTP id 46e09a7af769-7181a5c6046mr2826059a34.3.1729268667921; Fri, 18 Oct 2024 09:24:27 -0700 (PDT) Received: from [192.168.1.128] ([38.175.170.29]) by smtp.gmail.com with ESMTPSA id 46e09a7af769-718195da4d0sm376406a34.56.2024.10.18.09.24.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 18 Oct 2024 09:24:27 -0700 (PDT) Message-ID: <40ca64d7-c8c9-47b1-ac17-95524bd76aa6@linuxfoundation.org> Date: Fri, 18 Oct 2024 10:24:25 -0600 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 4/4] selftests/mm: add self tests for guard page feature To: Lorenzo Stoakes Cc: Andrew Morton , Suren Baghdasaryan , "Liam R . Howlett" , Matthew Wilcox , Vlastimil Babka , "Paul E . McKenney" , Jann Horn , David Hildenbrand , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Muchun Song , Richard Henderson , Ivan Kokshaysky , Matt Turner , Thomas Bogendoerfer , "James E . J . Bottomley" , Helge Deller , Chris Zankel , Max Filippov , Arnd Bergmann , linux-alpha@vger.kernel.org, linux-mips@vger.kernel.org, linux-parisc@vger.kernel.org, linux-arch@vger.kernel.org, Shuah Khan , Christian Brauner , linux-kselftest@vger.kernel.org, Sidhartha Kumar , Jeff Xu , Christoph Hellwig , Shuah Khan References: <8b1add3c511effb62d68183cae8a954d8339286c.1729196871.git.lorenzo.stoakes@oracle.com> <1d0bbc60-fda7-4c14-bf02-948bdbf8f029@linuxfoundation.org> <22d386cd-e62f-43f9-905e-2d0881781abe@linuxfoundation.org> <7bbfc635-8d42-4c3d-8808-cb060cd9f658@lucifer.local> Content-Language: en-US From: Shuah Khan In-Reply-To: <7bbfc635-8d42-4c3d-8808-cb060cd9f658@lucifer.local> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: E90C880012 X-Stat-Signature: mdyno95gab1359oy3xkappimod6g1333 X-Rspam-User: X-HE-Tag: 1729268647-432246 X-HE-Meta: U2FsdGVkX1/uwlAPcE6rSAKlW6Sq/tebSGWNhQNlpRRJfyAk71y7L330tgBh6Cry++HbxjxLD+CL5fsB8Fy8+U1Zz9BDHxKZx2Vp7S+/n48l2YSACeexgwxyOJftrTQhDC7XLwdYN8dxGYfYg9kwSusTuwDi+uj39Y4S/pUrOvKD1db56Yk7fQaYpHviUbBs2aE/T/2NM92bAsMBoUgg/Jnvro0K3IjwbFaurCOU1qpR2vh8JNOV6TDu5TXB584ttQIgnnMCKzY2o+cTt5lEfFKzE+c6yY/yDn/FwOXeXAFEbzYBVwUt9i6bO7c3WSlzlbC9MZNsWZkzmNgjlDFMcehiF+gA/i7H9d8eMgTWVeBv2SuyWIOikwIy7VjhbWJnVPteOA4wqkxeClCqFoqR6nCp3z1j+1hO8Zpv2Jav9+/2vAFOH1te9efRYkiiwy8mZ8TMQvBGG185GRIdq8jazLstQ6P3//1Bm/z/WzCH9Hh+NDaItWeXs8BHsbDiijDMWjtX24RW8vLQarKXPW6+6K84EOZhJZNsB6sL46KJI1AZvjF2S3FsQeIathAJgYOe5qSEedLScVasRDJ4DZvWtLIQ0ZmukoY0mY027PRZMoiVCqOuq85igaGtjs5uME8X6h7NjiwCdz0s+cAetks8urHyf2EqqsPpz3+FyMYIwY5v6VLiPNWdOrvU85a6JQDA/PqBbnUB1N5KN1iZOP4rL/pne6grEB3SBS8VKR2NA0Zzi0mfjdE5HA4Sh/E7l9li/1gXnhuRNEHrc4jz5rXWEkf17AnFvuduwLUFG197uA8FqbNklNt9EpdzJ9IHJI6Y2QOlpzB9rfhkL332jgL+DjMLInKfNEY2BQj757O0oC+HZkfBBF05LFJdm+tuMkRKPJFvKvj/AYDeZQKGC+fc6WDW+eXLz457rJIBcF2bj3HB0WllSpHAMjP1/pBzc4zQK9MJIghLEJn5pkqLcRK Y3rHdCaW +iGEZRRgRV5teFobBmRqHNr6a6JV8MPVWbGV4hF0g+y3RTLnBS3WoRAor5wwJlKrAtNI39OrJ0+Nr81WYKPKJ1IGw3Aj5uFLFSzxJZHpJacaHx/p+Xv/PoWBAcZiALVu7aL3q3ldaaxEjOB47jN2lmW6MjUCWAHrugjQKaRZ/FlaMD+XJPNHYmm7xN/ZN2TVhvexVdbuyIxLvzaFiwRPddibERtlIv+JSR7STdUw5hHCvQahhMx9oPg6MWcxLH8eNG6Mp3ucK0pz6wOthqxQda/bYQVzZdnWVEUWWRWXR0i8Um/ArYpC/aoFfQf6XtcByTm5wjTkZUnsN0h6DRm8gYmUzzMifWCbuFmyTQ1zvzxS1xWCt7TOJiTj8SJ4ieDNI3vksHRf/u3EHKR3t/s20eXgUyA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 10/18/24 10:07, Lorenzo Stoakes wrote: > On Fri, Oct 18, 2024 at 09:32:17AM -0600, Shuah Khan wrote: >> On 10/18/24 01:12, Lorenzo Stoakes wrote: >>> On Thu, Oct 17, 2024 at 03:24:49PM -0600, Shuah Khan wrote: >>>> On 10/17/24 14:42, Lorenzo Stoakes wrote: >>>>> Utilise the kselftest harmness to implement tests for the guard page >>>> >>>> Splleing NIT - harmness -> harness >>>> >>>>> implementation. >>>>> >>>>> We start by implement basic tests asserting that guard pages can be >>>> >>>> implmenting? By the way checkpatch will catch spelling stuuf. >>>> Please see comments about warnings below. >>> >>> Thanks. The majority of the checkpatch warnings are invalid so I missed >>> this. Will fix on respin. >>> >>>> >>>>> established (poisoned), cleared (remedied) and that touching poisoned pages >>>>> result in SIGSEGV. We also assert that, in remedying a range, non-poison >>>>> pages remain intact. >>>>> >>>>> We then examine different operations on regions containing poison markers >>>>> behave to ensure correct behaviour: >>>>> >>>>> * Operations over multiple VMAs operate as expected. >>>>> * Invoking MADV_GUARD_POISION / MADV_GUARD_REMEDY via process_madvise() in >>>>> batches works correctly. >>>>> * Ensuring that munmap() correctly tears down poison markers. >>>>> * Using mprotect() to adjust protection bits does not in any way override >>>>> or cause issues with poison markers. >>>>> * Ensuring that splitting and merging VMAs around poison markers causes no >>>>> issue - i.e. that a marker which 'belongs' to one VMA can function just >>>>> as well 'belonging' to another. >>>>> * Ensuring that madvise(..., MADV_DONTNEED) does not remove poison markers. >>>>> * Ensuring that mlock()'ing a range containing poison markers does not >>>>> cause issues. >>>>> * Ensuring that mremap() can move a poisoned range and retain poison >>>>> markers. >>>>> * Ensuring that mremap() can expand a poisoned range and retain poison >>>>> markers (perhaps moving the range). >>>>> * Ensuring that mremap() can shrink a poisoned range and retain poison >>>>> markers. >>>>> * Ensuring that forking a process correctly retains poison markers. >>>>> * Ensuring that forking a VMA with VM_WIPEONFORK set behaves sanely. >>>>> * Ensuring that lazyfree simply clears poison markers. >>>>> * Ensuring that userfaultfd can co-exist with guard pages. >>>>> * Ensuring that madvise(..., MADV_POPULATE_READ) and >>>>> madvise(..., MADV_POPULATE_WRITE) error out when encountering >>>>> poison markers. >>>>> * Ensuring that madvise(..., MADV_COLD) and madvise(..., MADV_PAGEOUT) do >>>>> not remove poison markers. >>>> >>>> Good summary of test. Does the test require root access? >>>> If so does it check and skip appropriately? >>> >>> Thanks and some do, in those cases we skip. >>> >>>> >>>>> >>>>> Signed-off-by: Lorenzo Stoakes >>>>> --- >>>>> tools/testing/selftests/mm/.gitignore | 1 + >>>>> tools/testing/selftests/mm/Makefile | 1 + >>>>> tools/testing/selftests/mm/guard-pages.c | 1168 ++++++++++++++++++++++ >>>>> 3 files changed, 1170 insertions(+) >>>>> create mode 100644 tools/testing/selftests/mm/guard-pages.c >>>>> >>>>> diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore >>>>> index 689bbd520296..8f01f4da1c0d 100644 >>>>> --- a/tools/testing/selftests/mm/.gitignore >>>>> +++ b/tools/testing/selftests/mm/.gitignore >>>>> @@ -54,3 +54,4 @@ droppable >>>>> hugetlb_dio >>>>> pkey_sighandler_tests_32 >>>>> pkey_sighandler_tests_64 >>>>> +guard-pages >>>>> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile >>>>> index 02e1204971b0..15c734d6cfec 100644 >>>>> --- a/tools/testing/selftests/mm/Makefile >>>>> +++ b/tools/testing/selftests/mm/Makefile >>>>> @@ -79,6 +79,7 @@ TEST_GEN_FILES += hugetlb_fault_after_madv >>>>> TEST_GEN_FILES += hugetlb_madv_vs_map >>>>> TEST_GEN_FILES += hugetlb_dio >>>>> TEST_GEN_FILES += droppable >>>>> +TEST_GEN_FILES += guard-pages >>>>> ifneq ($(ARCH),arm64) >>>>> TEST_GEN_FILES += soft-dirty >>>>> diff --git a/tools/testing/selftests/mm/guard-pages.c b/tools/testing/selftests/mm/guard-pages.c >>>>> new file mode 100644 >>>>> index 000000000000..2ab0ff3ba5a0 >>>>> --- /dev/null >>>>> +++ b/tools/testing/selftests/mm/guard-pages.c >>>>> @@ -0,0 +1,1168 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0-or-later >>>>> + >>>>> +#define _GNU_SOURCE >>>>> +#include "../kselftest_harness.h" >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> + >>>>> +/* These may not yet be available in the uAPI so define if not. */ >>>>> + >>>>> +#ifndef MADV_GUARD_POISON >>>>> +#define MADV_GUARD_POISON 102 >>>>> +#endif >>>>> + >>>>> +#ifndef MADV_GUARD_UNPOISON >>>>> +#define MADV_GUARD_UNPOISON 103 >>>>> +#endif >>>>> + >>>>> +volatile bool signal_jump_set; >>>> >>>> Can you add a comment about why volatile is needed. >>> >>> I'm not sure it's really necessary, it's completely standard to do this >>> with signal handling and is one of the exceptions to the 'volatile >>> considered harmful' rule. >>> >>>> By the way did you happen to run checkpatck on this. There are >>>> several instances where single statement blocks with braces {} >>>> >>>> I noticed a few and ran checkpatch on your patch. There are >>>> 45 warnings regarding codeing style. >>>> >>>> Please run checkpatch and clean them up so we can avoid followup >>>> checkpatch cleanup patches. >>> >>> No sorry I won't, checkpatch isn't infallible and series trying to 'clean >>> up' things that aren't issues will be a waste of everybody's time. >>> >> >> Sorry - this violates the coding styles and makes it hard to read. >> >> See process/coding-style.rst: >> >> Do not unnecessarily use braces where a single statement will do. >> >> .. code-block:: c >> >> if (condition) >> action(); >> >> and >> >> .. code-block:: c >> >> if (condition) >> do_this(); >> else >> do_that(); >> >> This does not apply if only one branch of a conditional statement is a single >> statement; in the latter case use braces in both branches: >> >> .. code-block:: c >> >> if (condition) { >> do_this(); >> do_that(); >> } else { >> otherwise(); >> } >> >> Also, use braces when a loop contains more than a single simple statement: >> >> .. code-block:: c >> >> while (condition) { >> if (test) >> do_something(); >> } >> >> thanks, >> -- Shuah > > Shuah, quoting coding standards to an experienced kernel developer > (maintainer now) is maybe not the best way to engage here + it may have > been more productive for you to first engage on why it is I'm deviating > here. > > Firstly, as I said, the code _does not compile_ if I do not use braces in > many cases. This is probably an issue with the macros, but it is out of > scope for this series for me to fix that. I am not trying to throw the book at you. When I see 45 of them I have to ask the reasons why. > > 'Fixing' these cases results in: > > CC guard-pages > guard-pages.c: In function ‘guard_pages_split_merge’: > guard-pages.c:566:17: error: ‘else’ without a previous ‘if’ > 566 | else > | ^~~~ > guard-pages.c: In function ‘guard_pages_dontneed’: > guard-pages.c:666:17: error: ‘else’ without a previous ‘if’ > 666 | else > | ^~~~ > guard-pages.c: In function ‘guard_pages_fork’: > guard-pages.c:957:17: error: ‘else’ without a previous ‘if’ > 957 | else > | ^~~~ > guard-pages.c: In function ‘guard_pages_fork_wipeonfork’: > guard-pages.c:1010:17: error: ‘else’ without a previous ‘if’ > 1010 | else > | ^~~~ > > In other cases I am simply not a fan of single line loops where there is a > lot of compound stuff going on: > > for (i = 0; i < 10; i++) { > ASSERT_FALSE(try_read_write_buf(&ptr1[i * page_size])); > } > > vs. > > for (i = 0; i < 10; i++) > ASSERT_FALSE(try_read_write_buf(&ptr1[i * page_size])); > > When there are very many loops like this. This is kind of a test-specific > thing, you'd maybe put more effort into splitting this up + have less > repetition in non-test code. > > I'm not going to die on the hill of single-line-for-loops though, so if you > insist I'll change those. > > However I simply _cannot_ change the if/else blocks that cause compilation > errors. > > This is what I mean about checkpatch being fallible. It's also fallible in > other cases, like variable declarations via macro (understandably). > > Expecting checkpatch to give zero warnings is simply unattainable, > unfortunately. > > As you seem adamant about strict adherence to checkpatch, and I always try > to be accommodating where I can be, I suggest I fix everything _except > where it breaks the compilation_ does that work for you? Yes Please. If you leave these in here as soon as the patch hits next, we start seeing a flood of patches. It becomes harder to fix these later due to merge conflicts. It becomes a patch overload issue. Yes I would like to see the ones that don't result in compile errors fixed. thanks, -- Shuah > > Thanks.