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 F09DFD3F295 for ; Fri, 18 Oct 2024 20:30:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7F1A18D0001; Fri, 18 Oct 2024 16:30:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 779976B00BA; Fri, 18 Oct 2024 16:30:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5F3FF8D0001; Fri, 18 Oct 2024 16:30:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 376C96B00B9 for ; Fri, 18 Oct 2024 16:30:14 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 10B911C4744 for ; Fri, 18 Oct 2024 20:30:00 +0000 (UTC) X-FDA: 82687864770.26.622075C Received: from mail-io1-f54.google.com (mail-io1-f54.google.com [209.85.166.54]) by imf18.hostedemail.com (Postfix) with ESMTP id 1ABFC1C001F for ; Fri, 18 Oct 2024 20:30:06 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=google header.b=HhcZ2CBn; spf=pass (imf18.hostedemail.com: domain of skhan@linuxfoundation.org designates 209.85.166.54 as permitted sender) smtp.mailfrom=skhan@linuxfoundation.org; dmarc=pass (policy=none) header.from=linuxfoundation.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729283265; 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=4Eb/bjYAyoVfge6Z8/C8u0lCt3wwAWzH/nDkL3fIlcc=; b=qjBOHKGJ7Et1+Uzi+6l07ZHbozdtjm3WlxTRyQlkMgQfVdf9z3p72RHBIdqE0+UF+Dl+wR BLeLDUqWcVmJCv3L1GK+JMN6hwWrx8ClNDcHzcMk4ebkDccd1Jux0mxfLd5a/cSg+Gseto 9QLn0Wv6u7tDdzW0loApzzniuuimkNg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729283265; a=rsa-sha256; cv=none; b=KYNhImUNPGyFT3KZS1dxsSu9cS/WeoSUOkmeDRbg1gCNsPKUi/I1xCQMypdgGuEggrszfJ mH5kkFKeXexSeOkcVQeWHuIuBGkUnBV2O+ct6f7WIvbUoV3Lz1/1gjsxHpJZ8wjQec4eLi 9LxDVevcZIUCeN1ZFFdB9MbtolSzk+8= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=google header.b=HhcZ2CBn; spf=pass (imf18.hostedemail.com: domain of skhan@linuxfoundation.org designates 209.85.166.54 as permitted sender) smtp.mailfrom=skhan@linuxfoundation.org; dmarc=pass (policy=none) header.from=linuxfoundation.org Received: by mail-io1-f54.google.com with SMTP id ca18e2360f4ac-83aad4a05eeso100251639f.1 for ; Fri, 18 Oct 2024 13:30:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linuxfoundation.org; s=google; t=1729283411; x=1729888211; 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=4Eb/bjYAyoVfge6Z8/C8u0lCt3wwAWzH/nDkL3fIlcc=; b=HhcZ2CBnQzCV1VW7daxmUw116s/Ip25Fcu/tjUzU+aM7l3/I5eHnF87OMVtLvo6uQ5 XxvhWILRC1D7i+f9qMX0xOMcg3e8vMaDzWtG4NLXlYdAOsTyTbxpNIJXBDlACAHLhxlt 7w3uKNiwTyqwIy9WR+IBD6wfMQP1r334Yo1wA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729283411; x=1729888211; 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=4Eb/bjYAyoVfge6Z8/C8u0lCt3wwAWzH/nDkL3fIlcc=; b=GlG214oQGlTU192ZgI0s20Yi9/KC+gX4nXM7ALKaaksplfjGrXvuwSOrypRGK9QVhQ S8WTb7LbeBULNfvUjCoOEyH2AEri5RLnd4DoUS5VFcgNcVk2QTPRN1ZoCZsLRoZ1GaZr GrOCUpf7mj68dgqu3fBf27VetmlQynLKbdC7Cu5xFiK5AHQHgDooNXBJTr26bjGnoW3p YEAILaI7CedOhISpPl91irA7ml9CEbWPLaaREc3EksVlZL7szBmDK+skPU3dN2IUt0sl HDWB6jX+7PZxRI+QZvmnP3bYyhoF/OAS1P3lnH96u9JLD2B3477KZBKoLlIH12KkXTPt khuA== X-Forwarded-Encrypted: i=1; AJvYcCUZXg0yX1GmLzC44Q5YR0V8Nmlz2gff03PJXPjL+hZ6RvtCAdVC2XW7ZIcr8cgvyNeBoacW/j7pUQ==@kvack.org X-Gm-Message-State: AOJu0YwK9OfjFeHfXbPPiNCiZyNRvIwbT36nKVk+JEIQzusHns4a0h2N 9MLK9l7wL59czGhRpU71BLjbiirIZNz4K+AHJXWXkMEPewPTUY9KF0T+7b2FesE= X-Google-Smtp-Source: AGHT+IHtME0xh1bwpwUpGOSzAWusPxbbyxBma0gMXzwpjVl6rP5b2Bp9df1W13qPVhmxiPTyD4G2jw== X-Received: by 2002:a05:6602:13d4:b0:83a:a25a:cfaf with SMTP id ca18e2360f4ac-83aba5c097amr415765139f.3.1729283410856; Fri, 18 Oct 2024 13:30:10 -0700 (PDT) Received: from [192.168.1.128] ([38.175.170.29]) by smtp.gmail.com with ESMTPSA id 8926c6da1cb9f-4dc10c6b47asm595342173.162.2024.10.18.13.30.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 18 Oct 2024 13:30:10 -0700 (PDT) Message-ID: <59e6d89e-9b5b-4dd9-9c05-2acd0a51d3af@linuxfoundation.org> Date: Fri, 18 Oct 2024 14:30:09 -0600 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap To: Jeff Xu , Mark Brown Cc: Muhammad Usama Anjum , Lorenzo Stoakes , akpm@linux-foundation.org, linux-kselftest@vger.kernel.org, linux-mm@kvack.org, linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org, pedro.falcato@gmail.com, willy@infradead.org, vbabka@suse.cz, Liam.Howlett@oracle.com, rientjes@google.com, keescook@chromium.org, Shuah Khan References: <4944ce41-9fe1-4e22-8967-f6bd7eafae3f@lucifer.local> <1f8eff74-005b-4fa9-9446-47f4cdbf3e8d@sirena.org.uk> Content-Language: en-US From: Shuah Khan In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 1ABFC1C001F X-Stat-Signature: tnno8p7sq1wkwuagwsrhaxmipx4hekj3 X-HE-Tag: 1729283406-45964 X-HE-Meta: U2FsdGVkX1/fphgDhqi+xrJWz0+pR/KzLFq0VEdL8yUZ8VW8jfSQopkmI8Ps8TI0Co2XI0FkRbI0DUnDacdMVu4AS47rgYzu2VqYhDS728876zrXes9h0eYWZVw1J1ErKiBib+qNRiD2boGI8n+zAF3PrOG3lBSUs/YyEfHwCC0qjgcmMraCVvSMD5B7tGzuN/jG2aw35rjgm743CIuDNNRJ6u9y1OtyrOoWVcZsZ90Q8Nh1wZeypdHUNchzKCeuWgBvKHXigZWd/b1rsYCifiua4dc+b5y6Sm3axTTYNVSkXTZs0adoxUl7U2exMAzq5h+C7mvsGqzgScb7hN+ut2JBI3dabESBtnSdclEWfdz2NZ1gZhGHnI2UIyB54oYMaBvcoHWZ5mM1sNd7sV94OuVR4zaTw5BmnvmSAx6YAFNFNeNLsQp8WPvsHxCbgO/GNJdEVjdCQjBd4yTfctkPelt2wAqzI3/rWyDWZDSCRoi4YLIyKWQNTyqCS1adaOaOpKqAc8kjVhHW/gBUoxA6Y1+/TQXjGY/vXQDfEBVvjyT5ID/KT+OT9+gK5ql6NrIlW+N0QZxVamHxkLLhOXUdXX9T4Vn7OsPYhDJt6NPcdk9t++k/KPyFUj1KXiok+1+/9NDMEGcjKcfeVn4KR7yE5L0i22ISrSDS6vqugTt9JM8RD5aF3MYC/ZoJbg71tItRCi9aCHlIoMV9ziID1TA8Ge0ykueiwi6BmS4xYR2kMw9UhxKPOrk9kTOCVn4L977pTY/AFF3asudIhohMCliEyyWcWGuiZwqeDdc244ICadBgRjjlqeLcZBL8C+0gchxpx6BW7o3DLXbQ/vptbEaz8tpkoiJkGKfvIzOTXNjmVlwjerCH2opeqhn2acZbSQvWvcgs8XSHvku7ViF0DhtHU8+co4WNSy60VOIumdp6qsYVHdEFbtvHZTCkQvGLHLgnXtsbwmTZ/co634Z70aj gfojKxzK zBL7Gzfqz7LNPJnt6WBaVSZzjhn8xLqQhsc3p/28admm9ommRJX6YpanzmxejXAZe6LG2ncrttQor0lc4/nU71g8oeRB0jgN6ONlbNKDToa8qek6B5WKK17XBw9Izt1C20Rm0XZJo/nOF7dYJq//bBjo0AXM+uPIGCl5P3rvJ4wIRvZ+tORkPGmwlQA9xpB0dKzKBQDK6gDZsJS/HEXMFEIlZCc0Hi/CVe9DK0F8cX9iE9GbK2Z4flZVjOgUMRY6PTMaQG7UGwkbgAeC3tvxzX6aPEWKMus1XpL5Jljx/pNeUNBW8k2+fzMxA4uuMu+iSSe/2CvHOSGqO8wcvbsd/yhiAQ/fIO1/E7QG0YkvBodoZ1yBCLYGPydTYEpiWUNRrO2EG3q4ZKqes/7NHTGtB/54zUutiwZXeRtHBO/IoGQOTU+N+3ln5jEiUYZMlrtIJ3liGAIoA4n6cOHCdycq0fdwCfJsAXWS4LJ82Yz8go6iQbO89fHc7S+TtQRhdLI5z0aJaSxioTsye8kQ= 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 13:32, Jeff Xu wrote: > Hi Mark > > On Fri, Oct 18, 2024 at 11:37 AM Mark Brown wrote: >> >> On Fri, Oct 18, 2024 at 11:06:20AM -0700, Jeff Xu wrote: >>> On Fri, Oct 18, 2024 at 6:04 AM Mark Brown wrote: >> >>>> The problem is that the macro name is confusing and not terribly >>>> consistent with how the rest of the selftests work. The standard >>>> kselftest result reporting is >> >>>> ksft_test_result(bool result, char *name_format, ...); >>>> >>>> so the result of the test is a boolean flag which is passed in. This >>>> macro on the other hand sounds like a double negative so you have to >>>> stop and think what the logic is, and it's not seen anywhere else so >>>> nobody is going to be familiar with it. The main thing this is doing is >>>> burying a return statement in there, that's a bit surprising too. >> >>> Thanks for explaining the problem, naming is hard. Do you have a >>> suggestion on a better naming? >> >> Honestly I'd probably deal with this by refactoring such that the macro >> isn't needed and the tests follow a pattern more like: >> >> if (ret != 0) { >> ksft_print_msg("$ACTION failed with %d\n", ret); >> return false; >> } >> > So expanding the macro to actually code ? > But this makes the meal_test quite large with lots of "if", and I > would rather avoid that. > > >> when they encouter a failure, the pattern I sketched in my earlier >> message, or switch to kselftest_harness.h (like I say I don't know if >> the fork()ing is an issue for these tests). If I had to have a macro >> it'd probably be something like mseal_assert(). >> > I can go with mseal_assert, the original macro is used by mseal_test > itself, and only intended as such. > > If changing name to mseal_assert() is acceptable, this seems to be a > minimum change and I'm happy with that. > >>>> I'll also note that these macros are resulting in broken kselftest >>>> output, the name for a test has to be stable for automated systems to be >>>> able to associate test results between runs but these print >> >> .... >> >>>> which includes the line number of the test in the name which is an >>>> obvious problem, automated systems won't be able to tell that any two >>>> failures are related to each other never mind the passing test. We >>>> should report why things failed but it's better to do that with a >>>> ksft_print_msg(), ideally one that's directly readable rather than >>>> requiring someone to go into the source code and look it up. >> >>> I don't know what the issue you described is ? Are you saying that we >>> are missing line numbers ? it is not. here is the sample of output: >> >> No, I'm saying that having the line numbers is a problem. >> >>> Failure in the second test case from last: >> >>> ok 105 test_munmap_free_multiple_ranges >>> not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573 >>> ok 107 test_munmap_free_multiple_ranges_with_split >> >> Test 106 here is called "test_munmap_free_multiple_ranges_with_split: >> line:2573" which automated systems aren't going to be able to associate >> with the passing "test_munmap_free_multiple_ranges_with_split", nor with >> any failures that occur on any other lines in the function. >> > I see. That will happen when those tests are modified and line number > changes. I could see reasoning for this argument, especially when > those tests are flaky and get updated often. > > In practice, I hope any of those kernel self-test failures should get > fixed immediately, or even better, run before dev submitting the patch > that affects the mm area. > > Having line number does help dev to go to error directly, and I'm not > against filling in the "action" field, but you might also agree with > me, finding unique text for each error would require some decent > amount of time, especially for large tests such as mseal_test. > >>> I would image the needs of something similar to FAIL_TEST_IF_FALSE is >>> common in selftest writing: >> >>> 1> lightweight enough so dev can pick up quickly and adapt to existing >>> tests, instead of rewriting everything from scratch. >>> 2> assert like syntax >>> 3> fail the current test case, but continue running the next test case >>> 4> take care of reporting test failures. >> >> Honestly this just sounds and looks like kselftest_harness.h, it's >> ASSERT_ and EXPECT_ macros sound exactly like what you're looking for >> for asserts. The main gotchas with it are that it's not particularly >> elegant for test cases which need to enumerate system features (which I >> don't think is the case for mseal()?) and that it runs each test case in >> a fork()ed child which can be inconvenient for some tests. The use of >> fork() is because that makes the overall test program much more robust >> against breakage in individual tests and allows things like per test >> timeouts. > OK, I didn't know that ASSERT_ and EXPECT_ were part of the test fixture. > > If I switch to test_fixture, e,g, using TEST(test_name) > > how do I pass the "seal" flag to it ? Doesn't TH_LOG() work for you to pass arguments? > e.g. how do I run the same test twice, first seal = true, and second seal=false. > > test_seal_mmap_shrink(false); > test_seal_mmap_shrink(true); > > The example [1], isn't clear about that. > > https://www.kernel.org/doc/html/v4.19/dev-tools/kselftest.html#example > > Thanks > thanks, -- Shuah