From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) by sourceware.org (Postfix) with ESMTPS id 461E93858D33 for ; Thu, 6 Jul 2023 13:54:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 461E93858D33 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-3fbc77e769cso7878815e9.0 for ; Thu, 06 Jul 2023 06:54:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688651658; x=1691243658; h=content-transfer-encoding:in-reply-to:references:cc:to:from :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=xMvTmexCCXp7NwfqOSsTO3y36eDg1NrzCJ+WXSq9NKg=; b=J9XXJOTrJ90aXDxMMsRhNkLkq+sXJ/X6B6MGCBVHupQ/dij+la/KUxxLg9yMA0Gzuh sAJ4Fb0I9XFVj2YyX9Cjq1jtFts9a3XU489KvQGW2NTj7M4aOa5h1F42M+OUIH+Uijn+ REW2bK+a2vZ0Gp81ZaQI5eLc5ROUA6j4YxhUA/KErA0KIp11nd//d9E/pjnOJYsdPoxj /pGyusnwVSFZz7j/c/aVvQ03NHcKftenM6fT02/ZgOyMJIta2Qy0Iyep8IliqSYjsHxU qA3Xng1+20uRw0AZE1SposA/hsO4D1eHPM5IU/E+EoTCAiN7PMl7MrnCGyn29mv8Fivy 90yg== X-Gm-Message-State: ABy/qLaFwqVhefY955lLNa2ff0qPY4cS81AKGuFHhzRw/40m19LfNB5i 6OkUSi0B/LdfIvRjbCTYuUxHAV4H85w= X-Google-Smtp-Source: APBJJlGxKA2rkcvIFbC628uPXgpGpSv4oHhOuIl1FXZZNHoNknSTQMPfTbfoaFiadizDvHtpNh9ACQ== X-Received: by 2002:a7b:c451:0:b0:3fb:b4fc:be62 with SMTP id l17-20020a7bc451000000b003fbb4fcbe62mr1439503wmi.17.1688651657832; Thu, 06 Jul 2023 06:54:17 -0700 (PDT) Received: from ?IPV6:2001:8a0:f91d:bc00:4994:a784:f998:13bf? ([2001:8a0:f91d:bc00:4994:a784:f998:13bf]) by smtp.gmail.com with ESMTPSA id y4-20020a1c4b04000000b003fa999cefc0sm2173424wma.36.2023.07.06.06.54.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 Jul 2023 06:54:17 -0700 (PDT) Message-ID: Date: Thu, 6 Jul 2023 14:54:06 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525) Content-Language: en-US From: Pedro Alves To: Andrew Burgess , gdb-patches@sourceware.org Cc: Matt Turner References: <20230705134141.1441753-1-pedro@palves.net> <87a5wakzo4.fsf@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi again, On 2023-07-06 14:43, Pedro Alves wrote: > Hi Andrew, > > On 2023-07-05 18:59, Andrew Burgess wrote: >> Pedro Alves writes: >>> + if ((off_t) offset >= 0) >>> + { >>> + ret = (readbuf != nullptr >>> + ? pread64 (fd, readbuf, len, offset) >>> + : pwrite64 (fd, writebuf, len, offset)); >>> + } >> >> I haven't tested this, but the change looks good to me with one nit... >> >> I think the '{' and '}' here (and in gdbserver below) aren't GDB style, >> as there's only a single statement? > > I don't really mind much either way, as we're not super consistent, but note that > we do have a GDB-specific rule about this. Here: > > https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards > > It says: > > "Any two or more lines in code should be wrapped in braces, even if they are comments, as they look like separate statements:" > > As that says "Any two or more lines in code", I think it applies in this situation too. > I just realized that the lseek path, which I just reindented, has an if branch with more than one line which isn't using braces... The patch should at least be consistent in all the lines it touches... :-) If we follow the documented convention, then here's what we get. WDYT? Should we tweak the convention instead? >From ab7a42f42e711f2a3565da4f53b0d2e6bea0039d Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 7 Jun 2023 10:38:14 +0100 Subject: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525) Since commit 05c06f318fd9 ("Linux: Access memory even if threads are running"), GDB prefers pread64/pwrite64 to access inferior memory instead of ptrace. That change broke reading shared libraries on SPARC64 Linux, as reported by PR gdb/30525 ("gdb cannot read shared libraries on SPARC64"). On SPARC64 Linux, surprisingly (to me), userspace shared libraries are mapped at high 64-bit addresses: (gdb) info sharedlibrary Cannot access memory at address 0xfff80001002011e0 Cannot access memory at address 0xfff80001002011d8 Cannot access memory at address 0xfff80001002011d8 From To Syms Read Shared Object Library 0xfff80001000010a0 0xfff8000100021f80 Yes (*) /lib64/ld-linux.so.2 (*): Shared library is missing debugging information. Those addresses are 64-bit addresses with the high bits set. When interpreted as signed, they're negative. The Linux kernel rejects pread64/pwrite64 if the offset argument of type off_t (a signed type) is negative, which happens if the memory address we're accessing has its high bit set. See linux/fs/read_write.c sys_pread64 and sys_pwrite64 in Linux. Thankfully, lseek does not fail in that situation. So the fix is to use the 'lseek + read|write' path if the offset would be negative. Fix this in both native GDB and GDBserver. Tested on a SPARC64 GNU/Linux and x86-64 GNU/Linux. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30525 Change-Id: I79c724f918037ea67b7396fadb521bc9d1b10dc5 --- gdb/linux-nat.c | 32 ++++++++++++++++++++++---------- gdbserver/linux-low.cc | 35 ++++++++++++++++++++++------------- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index 383ef58fa23..86f3ebd3573 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -3909,18 +3909,30 @@ linux_proc_xfer_memory_partial_fd (int fd, int pid, gdb_assert (fd != -1); - /* Use pread64/pwrite64 if available, since they save a syscall and can - handle 64-bit offsets even on 32-bit platforms (for instance, SPARC - debugging a SPARC64 application). */ + /* Use pread64/pwrite64 if available, since they save a syscall and + can handle 64-bit offsets even on 32-bit platforms (for instance, + SPARC debugging a SPARC64 application). But only use them if the + offset isn't so high that when cast to off_t it'd be negative, as + seen on SPARC64. pread64/pwrite64 outright reject such offsets. + lseek does not. */ #ifdef HAVE_PREAD64 - ret = (readbuf ? pread64 (fd, readbuf, len, offset) - : pwrite64 (fd, writebuf, len, offset)); -#else - ret = lseek (fd, offset, SEEK_SET); - if (ret != -1) - ret = (readbuf ? read (fd, readbuf, len) - : write (fd, writebuf, len)); + if ((off_t) offset >= 0) + { + ret = (readbuf != nullptr + ? pread64 (fd, readbuf, len, offset) + : pwrite64 (fd, writebuf, len, offset)); + } + else #endif + { + ret = lseek (fd, offset, SEEK_SET); + if (ret != -1) + { + ret = (readbuf != nullptr + ? read (fd, readbuf, len) + : write (fd, writebuf, len)); + } + } if (ret == -1) { diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc index 8ab16698632..f580e4b7977 100644 --- a/gdbserver/linux-low.cc +++ b/gdbserver/linux-low.cc @@ -5377,21 +5377,30 @@ proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf, { int bytes; - /* If pread64 is available, use it. It's faster if the kernel - supports it (only one syscall), and it's 64-bit safe even on - 32-bit platforms (for instance, SPARC debugging a SPARC64 - application). */ + /* Use pread64/pwrite64 if available, since they save a syscall + and can handle 64-bit offsets even on 32-bit platforms (for + instance, SPARC debugging a SPARC64 application). But only + use them if the offset isn't so high that when cast to off_t + it'd be negative, as seen on SPARC64. pread64/pwrite64 + outright reject such offsets. lseek does not. */ #ifdef HAVE_PREAD64 - bytes = (readbuf != nullptr - ? pread64 (fd, readbuf, len, memaddr) - : pwrite64 (fd, writebuf, len, memaddr)); -#else - bytes = -1; - if (lseek (fd, memaddr, SEEK_SET) != -1) - bytes = (readbuf != nullptr - ? read (fd, readbuf, len) - : write (fd, writebuf, len)); + if ((off_t) memaddr >= 0) + { + bytes = (readbuf != nullptr + ? pread64 (fd, readbuf, len, memaddr) + : pwrite64 (fd, writebuf, len, memaddr)); + } + else #endif + { + bytes = -1; + if (lseek (fd, memaddr, SEEK_SET) != -1) + { + bytes = (readbuf != nullptr + ? read (fd, readbuf, len) + : write (fd, writebuf, len)); + } + } if (bytes < 0) return errno; base-commit: a5042543d819df8a76ebec8c4d715f244efbab0a -- 2.34.1