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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EB5A5C43334 for ; Fri, 17 Jun 2022 19:38:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242790AbiFQTio (ORCPT ); Fri, 17 Jun 2022 15:38:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51096 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242248AbiFQTiX (ORCPT ); Fri, 17 Jun 2022 15:38:23 -0400 Received: from mail-il1-x134.google.com (mail-il1-x134.google.com [IPv6:2607:f8b0:4864:20::134]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6A2032F028 for ; Fri, 17 Jun 2022 12:38:21 -0700 (PDT) Received: by mail-il1-x134.google.com with SMTP id h7so3612068ila.10 for ; Fri, 17 Jun 2022 12:38:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linuxfoundation.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=wmbuV7JBORPXXzvNtfIZq/lTzS3aE7SHxRte41av5uM=; b=E0B1WMNGy43PvdJGAw2GYGQ5aKuCRPlGDVLSnor2QZJNSRe5zTPsqxbwe6DludNLxs R4wYYbLa/fc/Ecl0bf0NnRRH8HbpYCn+8yNpx2sS/CcpcFAfgg29nSpNUvdaE7ZyKLUe X2pIoc6EroL/LYddX/FkGtSPgP77qKkI6tr/o= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=wmbuV7JBORPXXzvNtfIZq/lTzS3aE7SHxRte41av5uM=; b=rAYkW/YLjZX3TiY8Pp5DfozEIDR6TuhSnZPZ6LITMd7pYmV4S8xovwTTXdgz3vneLS GEt07MOphfQSJNNdJcjiWixenbM2Ma58ECuiOW2nWK4vzT51Ira/nF/OiTkvtL1//k5R GEy0P5FdyEEqjijC8w96OpdhBvLbZibbqVSis0e/kYnSLr4IpPb4KoeWlbbCGPTHyqxG BzHqXFJWnd0tg6K4oM61DdFTn6Xrs5X8LBrBRajkKd39MceAY7+ms280sKKLZlLObHx7 IWc4qOCafy2JIt0O1EZw2wa6UQoyeeoY+3XR68IxM71eN1PT29Oq3B5TqbwQh42PLm9A DiRg== X-Gm-Message-State: AJIora/PJ313Z2wbfx25xT7+AFq/deIsyZvT2NhehX+3xzl/neTau2Oi Y3liJ72Wx/sJSzhq9LbAGG/NZA== X-Google-Smtp-Source: AGRyM1tEue9xzls/gET9Q/THpzqGhH2Ss/UxUuncgPwKpQV957JCH1xLpqkKlgCFSExHWhnOMXGSmw== X-Received: by 2002:a05:6e02:1a23:b0:2d3:82bb:4dae with SMTP id g3-20020a056e021a2300b002d382bb4daemr6485677ile.62.1655494700776; Fri, 17 Jun 2022 12:38:20 -0700 (PDT) Received: from [192.168.1.128] ([38.15.45.1]) by smtp.gmail.com with ESMTPSA id m2-20020a6bbc02000000b006656f9eefa3sm2917727iof.18.2022.06.17.12.38.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 17 Jun 2022 12:38:20 -0700 (PDT) Subject: Re: [PATCH] selftests/proc: Fix proc-pid-vm for vsyscall=xonly. To: Dylan Hatch Cc: Shuah Khan , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kselftest@vger.kernel.org, Shuah Khan References: <20220616211016.4037482-1-dylanbhatch@google.com> <941e0991-eb3e-f988-8262-3d51ff8badad@linuxfoundation.org> From: Shuah Khan Message-ID: <47312e8a-87fe-c7dc-d354-74e81482bc1e@linuxfoundation.org> Date: Fri, 17 Jun 2022 13:38:19 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 6/17/22 12:45 PM, Dylan Hatch wrote: > On Thu, Jun 16, 2022 at 4:01 PM Shuah Khan wrote: >> >> On 6/16/22 3:10 PM, Dylan Hatch wrote: >>> This test would erroneously fail the /proc/$PID/maps case if >>> vsyscall=xonly since the existing probe of the vsyscall page only >>> succeeds if the process has read permissions. Fix this by checking for >>> either no vsyscall mapping OR an execute-only vsyscall mapping in the >>> case were probing the vsyscall page segfaults. >>> >> >> Does this fix include skipping the test with a clear message that >> says why test is skipped? >> >>> Signed-off-by: Dylan Hatch >>> --- >>> tools/testing/selftests/proc/proc-pid-vm.c | 20 +++++++++++++++----- >>> 1 file changed, 15 insertions(+), 5 deletions(-) >>> >>> diff --git a/tools/testing/selftests/proc/proc-pid-vm.c b/tools/testing/selftests/proc/proc-pid-vm.c >>> index 28604c9f805c..5ca85520131f 100644 >>> --- a/tools/testing/selftests/proc/proc-pid-vm.c >>> +++ b/tools/testing/selftests/proc/proc-pid-vm.c >>> @@ -213,9 +213,12 @@ static int make_exe(const uint8_t *payload, size_t len) >>> >>> static bool g_vsyscall = false; >>> >>> -static const char str_vsyscall[] = >>> +static const char str_vsyscall_rx[] = >>> "ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]\n"; >>> >>> +static const char str_vsyscall_x[] = >>> +"ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0 [vsyscall]\n"; >>> + >>> #ifdef __x86_64__ >>> static void sigaction_SIGSEGV(int _, siginfo_t *__, void *___) >>> { >>> @@ -261,6 +264,7 @@ int main(void) >>> int exec_fd; >>> >>> vsyscall(); >>> + const char *str_vsyscall = g_vsyscall ? str_vsyscall_rx : str_vsyscall_x; >>> >>> atexit(ate); >>> >>> @@ -314,7 +318,8 @@ int main(void) >>> >>> /* Test /proc/$PID/maps */ >>> { >>> - const size_t len = strlen(buf0) + (g_vsyscall ? strlen(str_vsyscall) : 0); >>> + const size_t len_buf0 = strlen(buf0); >>> + const size_t len_vsys = strlen(str_vsyscall); >>> char buf[256]; >>> ssize_t rv; >>> int fd; >>> @@ -325,11 +330,16 @@ int main(void) >>> return 1; >>> } >>> rv = read(fd, buf, sizeof(buf)); >>> - assert(rv == len); >>> - assert(memcmp(buf, buf0, strlen(buf0)) == 0); >>> if (g_vsyscall) { >>> - assert(memcmp(buf + strlen(buf0), str_vsyscall, strlen(str_vsyscall)) == 0); >>> + assert(rv == len_buf0 + len_vsys); >>> + } else { >>> + /* If vsyscall isn't readable, it's either x-only or not mapped at all */ >>> + assert(rv == len_buf0 + len_vsys || rv == len_buf0); >>> } >>> + assert(memcmp(buf, buf0, len_buf0) == 0); >>> + /* Check for vsyscall mapping if buf is long enough */ >>> + if (rv == len_buf0 + len_vsys) >>> + assert(memcmp(buf + len_buf0, str_vsyscall, len_vsys) == 0); >>> } >>> >>> /* Test /proc/$PID/smaps */ >>> >> >> The change looks good to me. Doesn't look like it skips the test though? > > Instead of skipping the test, it changes the passing condition to > accept both cases of an unmapped vsyscall page and an x-only vsyscall > page. Differentiating between these two cases without relying on > /proc/$PID/maps would involve both checking the kernel command line > for vsyscall=xonly and having a special ifdef block for > CONFIG_VSYSCALL_XONLY, so accepting both as passing conditions seems > like a simpler solution. > It depends on the goal of the test. Is the test looking to see if the probe fails with insufficient permissions, then you are changing the test to not check for that condition. I would say in this case, the right approach would be to leave the test as is and report expected fail and add other cases. The goal being adding more coverage and not necessarily opt for a simple solution. thanks, -- Shuah