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 77E0AC433EF for ; Tue, 19 Apr 2022 16:21:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id ED4948D0081; Tue, 19 Apr 2022 12:21:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E83228D0047; Tue, 19 Apr 2022 12:21:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D25358D0081; Tue, 19 Apr 2022 12:21:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.28]) by kanga.kvack.org (Postfix) with ESMTP id C648A8D0047 for ; Tue, 19 Apr 2022 12:21:53 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay11.hostedemail.com (Postfix) with ESMTP id 850C58236B for ; Tue, 19 Apr 2022 16:21:53 +0000 (UTC) X-FDA: 79374144906.06.E211B00 Received: from mail-oi1-f178.google.com (mail-oi1-f178.google.com [209.85.167.178]) by imf19.hostedemail.com (Postfix) with ESMTP id 7431E1A000B for ; Tue, 19 Apr 2022 16:21:51 +0000 (UTC) Received: by mail-oi1-f178.google.com with SMTP id r8so18664479oib.5 for ; Tue, 19 Apr 2022 09:21:52 -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=p2epm3ggiLebFuCe5K1Ff3vTQYYQk5e79Z+aoVpOO8c=; b=K5l5cdzTlkLERj2yYONu1tJVvYdG+sLFQVzP93XZDitHEplZYpPnEcxQOHejajdl2h iFtoGwzy5AVV08oKBsEQBNjtWWuoAs6Yg53E0t/UPJfwi2DhebcaAbPR7ExCSotRBM7W ZYnLRWZaOmVbDuGxbRLhh2ikFTaiwe8qz3hrc= 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=p2epm3ggiLebFuCe5K1Ff3vTQYYQk5e79Z+aoVpOO8c=; b=RsehEJQBSge1+0fS5HK6Dp7lFntY6TXBLoO2PGcLPkP+5iixtTNw7KDk6ttfEkO1f4 Ndr1aKeK0mwj5o6AxI8cUWBQJOaHCfG4OuPSoPFqXO05MqwuE+/yL4iGMPtNL0xyiDNX 0eSAHh/6fEnoibFzSs2l2kqmSRGGEPrRcmeKsUmRvRuGckFD2ONhvZGLZ3Gy02OPSA9K gdWIwa1oVtMyycBuy8Drclw2cuEDq6ZMFGJeFxSthTkxudQmUfNnFQQByr5p84Ne0pmE g3Zwhb3XVyRwKrhkS7xbPz4tUB5MtVdffHdwfqX3gXFLTxGqDuiK+jocZMK9SVFwjHjo EunA== X-Gm-Message-State: AOAM533jaAn1/w9ahXribCeWrpGtmNgguDRWP9i7moFRLYaHRT8aMHw1 3ahCj/8Uz07QP554d+TbNpTxjQ== X-Google-Smtp-Source: ABdhPJz5UABEfyTQPKsXaQtN5IE89LP+Ydx+2cHyTVuIZKVTLQxS6mdEu/bRG49gSlWxHy1/sCrJAQ== X-Received: by 2002:a05:6808:2085:b0:322:b167:77f9 with SMTP id s5-20020a056808208500b00322b16777f9mr2986961oiw.152.1650385311996; Tue, 19 Apr 2022 09:21:51 -0700 (PDT) Received: from [192.168.1.128] ([71.205.29.0]) by smtp.gmail.com with ESMTPSA id q12-20020a4ad54c000000b003245ac0a745sm5617178oos.22.2022.04.19.09.21.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 19 Apr 2022 09:21:51 -0700 (PDT) Subject: Re: [PATCH 2/4] selftest/vm: verify remap destination address in mremap_test To: Sidhartha Kumar , shuah@kernel.org, akpm@linux-foundation.org Cc: linux-mm@kvack.org, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, Shuah Khan References: <20220414171529.62058-1-sidhartha.kumar@oracle.com> <20220414171529.62058-3-sidhartha.kumar@oracle.com> <7b1c0a82-f7c3-4f60-ccb3-893caf4221f9@oracle.com> From: Shuah Khan Message-ID: <3a062d04-b35c-868a-cd94-5d57fef68813@linuxfoundation.org> Date: Tue, 19 Apr 2022 10:21:50 -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: <7b1c0a82-f7c3-4f60-ccb3-893caf4221f9@oracle.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 7431E1A000B X-Stat-Signature: peome38833d6bqp7ce7mpouizwchn3iw Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=google header.b=K5l5cdzT; dmarc=pass (policy=none) header.from=linuxfoundation.org; spf=pass (imf19.hostedemail.com: domain of skhan@linuxfoundation.org designates 209.85.167.178 as permitted sender) smtp.mailfrom=skhan@linuxfoundation.org X-HE-Tag: 1650385311-583259 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: On 4/14/22 4:24 PM, Sidhartha Kumar wrote: > > > On 4/14/22 2:47 PM, Shuah Khan wrote: >> On 4/14/22 11:15 AM, Sidhartha Kumar wrote: >>> Because mremap does not have a NOREPLACE flag, >>> it can destroy existing mappings. This can >>> cause a segfault if regions such as text are >>> destroyed. >> >> Please explain the reason for segfault. >> > With the MREMAP_FIXED flag used by the test, > the text region, which fell in the range of the remap > region, got unmapped. This caused a segfault when > trying to fetch the next instruction after the mremap() > call. >> Add a blank line here. Makes it easier to read. >> >> Verify the requested mremap destination >>> address does not overlap any existing mappings >>> by using mmap's FIXED_NOREPLACE flag and checking >> >> Spell this out fully - MAP_FIXED_NOREPLACE >>> for the EEXIST error code. Keep incrementing the >>> destination address until a valid mapping is found >>> or max address is reached. >>> >> >> Essentially mremap() doesn't check for overlaps and removes >> or overwrites existing mappings? The way you are fixing it >> is by verifying by calling mremap() with MAP_FIXED_NOREPLACE >> flag and check for EEXIST. >> > Yes, with the MREMAP_FIXED flag that the test uses, any previous > mapping in the address range of the remap region gets unmapped. > Yes, fixing this issue by calling mmap() with MAP_FIXED_NOREPLACE > flag and checking for EEXIST. > >> What happens when max address is reached? >> > That is covered by the check if (addr > ULLONG_MAX - region size) > in the remap_region_valid() function. >> Same comment on # of chars per line in commit log. Also >> >>> Signed-off-by: Sidhartha Kumar >>> --- >>>   tools/testing/selftests/vm/mremap_test.c | 36 ++++++++++++++++++++++++ >>>   1 file changed, 36 insertions(+) >>> >>> diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c >>> index 58600fee4b81..98e9cff34aa7 100644 >>> --- a/tools/testing/selftests/vm/mremap_test.c >>> +++ b/tools/testing/selftests/vm/mremap_test.c >>> @@ -10,6 +10,7 @@ >>>   #include >>>   #include >>>   #include >>> +#include >>>     #include "../kselftest.h" >>>   @@ -65,6 +66,34 @@ enum { >>>       .expect_failure = should_fail                \ >>>   } >>>   +/* >>> + * Returns 0 if the requested remap region overlaps with an >>> + * existing mapping (e.g text, stack) else returns 1. >>> + */ >>> +static int remap_region_valid(void *addr, unsigned long long size) >> >> This returns bool 0 (false) 1 (true) >> >> Please name the routine - is_remap_region_valid() and change it to >> return bool. >> >>> +{ >>> +    void *remap_addr = NULL; >>> +    int ret = 1; >>> + >>> +    if ((unsigned long long) addr > ULLONG_MAX - size) { >>> +        ksft_print_msg("Can't find a valid region to remap to\n"); >> >> Change it to "Couldn't" - also this message doesn't look right. We hav't >> looked for valid region yet and it just exceeds the limits? >> > Because this function is called in a loop in remap_region() and addr is being > incremented continuously, we could enter this function with addr high enough that > another increment would cause overflow. >> What this means is we could see lots of these messages that just say "Can't find" without any other information such as the "addr"? We probably don't want this message printed in a loop and print from caller after the while loop ends without finding valid mapping. >>> +        exit(KSFT_SKIP);> +    } >>> + >>> +    /* Use MAP_FIXED_NOREPLACE flag to ensure region is not mapped */ >>> +    remap_addr = mmap(addr, size, PROT_READ | PROT_WRITE, >>> +            MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED, >>> +            -1, 0); >> >> Alignment should match open parenthesis here and in other places. Makes it >> easier to read the code. >> >>> +    if (remap_addr == MAP_FAILED) { >>> +        if (errno == EEXIST) >>> +            ret = 0; >>> +    } else { >>> +        munmap(remap_addr, size); >>> +    } >>> + >>> +    return ret; >>> +} >>> + >>>   /* Returns mmap_min_addr sysctl */ >>>   static unsigned long long get_mmap_min_addr(void) >>>   { >>> @@ -180,6 +209,13 @@ static long long remap_region(struct config c, unsigned int threshold_mb, >>>       if (!((unsigned long long) addr & c.dest_alignment)) >>>           addr = (void *) ((unsigned long long) addr | c.dest_alignment); >>>   +    /* Don't destroy existing mappings unless expected to overlap */ >>> +    while (!remap_region_valid(addr, c.region_size)) { >>> +        if (c.overlapping) >>> +            break; >>> +        addr += c.src_alignment; >>> +    } >>> + Here instead of in the loop. >>>       clock_gettime(CLOCK_MONOTONIC, &t_start); >>>       dest_addr = mremap(src_addr, c.region_size, c.region_size, >>>               MREMAP_MAYMOVE|MREMAP_FIXED, (char *) addr); >>> >> thanks, -- Shuah