From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id AC7933858D20 for ; Tue, 30 May 2023 11:20:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AC7933858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1685445600; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Syx6i7GestOd8k6qXeXOQlTrCwpOg6bBe4K33Dw7MRg=; b=IM+gX4PfoqiFCcPiFosK60pBY67M25rDTawWIiu55QrFcjjCR5+6uT62kZCMmOfTFiQC2p 40pjjgkhm9U3lfWKlbL9uvoAskkq/1WV2QF+0CkhzyiISZr/gfqSLhnXhGzBJBbVRLZlQV 9ws8JScD64SaFdqDd2PdnCi8pNYSgnA= Received: from mail-yw1-f198.google.com (mail-yw1-f198.google.com [209.85.128.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-282-8juWi4SqMjqzD35vJpOTGQ-1; Tue, 30 May 2023 07:19:59 -0400 X-MC-Unique: 8juWi4SqMjqzD35vJpOTGQ-1 Received: by mail-yw1-f198.google.com with SMTP id 00721157ae682-5657dbf8cecso78608887b3.3 for ; Tue, 30 May 2023 04:19:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685445599; x=1688037599; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Syx6i7GestOd8k6qXeXOQlTrCwpOg6bBe4K33Dw7MRg=; b=JDTU986UJJhgTrBibzKj/BPczOIIVeWQNYfyk7TUc9Zrz1Db5hnMBeG2PzAI+eWYiL FXKr4e5Xqrzb7TGrxoadbWbnQmSOVA+RvtafG9lJFYEmEG9/6UoYO444+6gtAlBfrDHQ PpmlLmv+gtNUVWuOvgdBvTyMCr0ZmBcqOndD37zu2byuELrkQvsUr8xMuAbXUnv2MIoy Di0aTiT+OJXT61E+C47DXo5rFLDIApddRBeP08lmYJjFwGVHafmAzTLR9wqyksmmSK2A l92YZTXt+oTXC+VHM+oYlrN+K7TekwmDPdrvwO7vfl0bEUHmydQT8aky6gvyczdRy5Lb 7QHQ== X-Gm-Message-State: AC+VfDwbdtj93nVmPU5gyksw8ET/FbJ1uTAJ1mXg0IGtddGiWflA/54H AV1VeJAIBYMQsJOdlK/HczY7iHbBtqrl2B+pjU4lJN4woy7J0x9hrXML1RPe/026Hjylv/yIlJ/ ASwgXqGJdxswhkVDooIhv X-Received: by 2002:a0d:db54:0:b0:565:e48d:32ce with SMTP id d81-20020a0ddb54000000b00565e48d32cemr2045996ywe.11.1685445598743; Tue, 30 May 2023 04:19:58 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6Q1OegWZS9CgGnts9cNoddGOcpjDO34uM2v9lTz8XqtbNC4XItidiUYKfuyrOIXCFre9R2EA== X-Received: by 2002:a0d:db54:0:b0:565:e48d:32ce with SMTP id d81-20020a0ddb54000000b00565e48d32cemr2045980ywe.11.1685445598307; Tue, 30 May 2023 04:19:58 -0700 (PDT) Received: from [192.168.0.241] ([198.48.244.52]) by smtp.gmail.com with ESMTPSA id a127-20020a0df185000000b0055db1f73722sm4330707ywf.107.2023.05.30.04.19.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 May 2023 04:19:57 -0700 (PDT) Message-ID: Date: Tue, 30 May 2023 07:19:56 -0400 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 v2] io: Fix record locking contants on 32 bit arch with 64 bit default time_t (BZ#30477) To: Adhemerval Zanella , libc-alpha@sourceware.org, Bo YU References: <20230525135115.1858505-1-adhemerval.zanella@linaro.org> From: Carlos O'Donell Organization: Red Hat In-Reply-To: <20230525135115.1858505-1-adhemerval.zanella@linaro.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,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: On 5/25/23 09:51, Adhemerval Zanella via Libc-alpha wrote: > For architecture with default 64 bit time_t support, the kernel > does not provide LFS and non-LFS values for F_GETLK, F_GETLK, and > F_GETLK (the default value used for 64 bit architecture are used). Right, and the mistake is that we need to use the 64-bit time_t-ness check to get the right constants in userspace, which your patch fixes. > This is might be considered an ABI break, but the currenct exported > values is bogus anyway. It *is* an ABI break, but if the previous values could never have worked then there doesn't exist a program that can use the interface and this becomes a "bug fix" that corrects an ABI mistake. This is OK with me. > The POSIX lockf is not affected since it is aliased to lockf64, > which already uses the LFS values. > > Checked on i686-linux-gnu and the new tests on a riscv32. LGTM. Passes all pre-commit CI. Reviewed-by: Carlos O'Donell > --- > Changes from v1: > * Remove parens on defined operator. > * Make the test use tst-flock skeleton. > --- > io/Makefile | 1 + > io/tst-fcntl-lock.c | 97 ++++++++++++++++++++++ > io/tst-lockf.c | 58 +++++++------ > sysdeps/unix/sysv/linux/bits/fcntl-linux.h | 2 +- > 4 files changed, 133 insertions(+), 25 deletions(-) > create mode 100644 io/tst-fcntl-lock.c > > diff --git a/io/Makefile b/io/Makefile > index db886ca26f..d573064ecc 100644 > --- a/io/Makefile > +++ b/io/Makefile > @@ -175,6 +175,7 @@ tests := \ > tst-fchmodat \ > tst-fchownat \ > tst-fcntl \ > + tst-fcntl-lock \ OK. Add new test. > tst-fstatat \ > tst-fts \ > tst-fts-lfs \ > diff --git a/io/tst-fcntl-lock.c b/io/tst-fcntl-lock.c > new file mode 100644 > index 0000000000..357c4b7b56 > --- /dev/null > +++ b/io/tst-fcntl-lock.c > @@ -0,0 +1,97 @@ > +/* Test for advisory record locking. OK. > + Copyright (C) 2023 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or > + modify it under the terms of the GNU General Public License > + as published by the Free Software Foundation; either version 2 > + of the License, or (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, see . > +*/ > + > +#include > +#include > +#include > + > +/* This is essentially the POSIX lockf. */ OK. https://pubs.opengroup.org/onlinepubs/9699919799/functions/lockf.html > + > +static int > +fcntl_lockf (int fd, int cmd, off_t len) > +{ > + struct flock fl = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_CUR, > + .l_len = len > + }; > + > + switch (cmd) > + { > + case F_TEST: > + fl.l_type = F_RDLCK; > + if (fcntl (fd, F_GETLK, &fl) < 0) > + return -1; > + if (fl.l_type == F_UNLCK || fl.l_pid == getpid ()) > + return 0; > + errno = EACCES; > + return -1; > + > + case F_ULOCK: > + fl.l_type = F_UNLCK; > + return fcntl (fd, F_SETLK, &fl); > + > + case F_LOCK: > + return fcntl (fd, F_SETLKW, &fl); > + > + case F_TLOCK: > + return fcntl (fd, F_SETLK, &fl); > + } > + > + errno = EINVAL; > + return -1; > +} > + > +static int > +fcntl64_lockf (int fd, int cmd, off64_t len64) > + { > + struct flock64 fl64 = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_CUR, > + .l_len = len64 > + }; > + > + switch (cmd) > + { > + case F_TEST: > + fl64.l_type = F_RDLCK; > + if (fcntl64 (fd, F_GETLK64, &fl64) < 0) > + return -1; > + if (fl64.l_type == F_UNLCK || fl64.l_pid == getpid ()) > + return 0; > + errno = EACCES; > + return -1; > + > + case F_ULOCK: > + fl64.l_type = F_UNLCK; > + return fcntl64 (fd, F_SETLK64, &fl64); > + > + case F_LOCK: > + return fcntl64 (fd, F_SETLKW64, &fl64); > + > + case F_TLOCK: > + return fcntl64 (fd, F_SETLK64, &fl64); > + } > + > + errno = EINVAL; > + return -1; > +} > + > +#define TST_LOCKFD "tst-fcntl-lock." > +#define LOCKF fcntl_lockf > +#define LOCKF64 fcntl64_lockf > +#include "tst-lockf.c" OK. You build a lockf test and then implement lockf using fcntl to verify the functionality works as expected. > diff --git a/io/tst-lockf.c b/io/tst-lockf.c > index eda04b5417..cf8d3001c3 100644 > --- a/io/tst-lockf.c > +++ b/io/tst-lockf.c > @@ -24,13 +24,23 @@ > #include > #include > > +#ifndef TST_LOCKFD > +# define TST_LOCKFD "tst-lockfd." > +#endif > +#ifndef LOCKF > +# define LOCKF lockf > +#endif > +#ifndef LOCKF64 > +# define LOCKF64 lockf64 > +#endif > + > static char *temp_filename; > static int temp_fd; > > static void > do_prepare (int argc, char **argv) > { > - temp_fd = create_temp_file ("tst-lockfd.", &temp_filename); > + temp_fd = create_temp_file (TST_LOCKFD, &temp_filename); > TEST_VERIFY_EXIT (temp_fd != -1); > } > #define PREPARE do_prepare > @@ -40,22 +50,22 @@ do_test_child_lockf (void *closure) > { > /* Check if parent has [0, 1024) locked. */ > TEST_COMPARE (lseek (temp_fd, 0, SEEK_SET), 0); > - TEST_COMPARE (lockf (temp_fd, F_TLOCK, 1024), -1); > + TEST_COMPARE (LOCKF (temp_fd, F_TLOCK, 1024), -1); > TEST_COMPARE (errno, EAGAIN); > - TEST_COMPARE (lockf (temp_fd, F_TEST, 1024), -1); > + TEST_COMPARE (LOCKF (temp_fd, F_TEST, 1024), -1); > TEST_COMPARE (errno, EACCES); > /* Also Check if parent has last 1024 bytes locked. */ > TEST_COMPARE (lseek (temp_fd, INT32_MAX-1024, SEEK_SET), INT32_MAX-1024); > - TEST_COMPARE (lockf (temp_fd, F_TEST, 1024), -1); > + TEST_COMPARE (LOCKF (temp_fd, F_TEST, 1024), -1); > > /* And try to lock [1024, 2048). */ > TEST_COMPARE (lseek (temp_fd, 1024, SEEK_SET), 1024); > - TEST_COMPARE (lockf (temp_fd, F_LOCK, 1024), 0); > + TEST_COMPARE (LOCKF (temp_fd, F_LOCK, 1024), 0); > > /* Check if non-LFS interface cap access to 32-bif off_t. */ > TEST_COMPARE (lseek64 (temp_fd, (off64_t)INT32_MAX, SEEK_SET), > (off64_t)INT32_MAX); > - TEST_COMPARE (lockf64 (temp_fd, F_TEST, 1024), 0); > + TEST_COMPARE (LOCKF64 (temp_fd, F_TEST, 1024), 0); > } > > static void > @@ -63,32 +73,32 @@ do_test_child_lockf64 (void *closure) > { > /* Check if parent has [0, 1024) locked. */ > TEST_COMPARE (lseek64 (temp_fd, 0, SEEK_SET), 0); > - TEST_COMPARE (lockf64 (temp_fd, F_TLOCK, 1024), -1); > + TEST_COMPARE (LOCKF64 (temp_fd, F_TLOCK, 1024), -1); > TEST_COMPARE (errno, EAGAIN); > - TEST_COMPARE (lockf64 (temp_fd, F_TEST, 1024), -1); > + TEST_COMPARE (LOCKF64 (temp_fd, F_TEST, 1024), -1); > TEST_COMPARE (errno, EACCES); > /* Also Check if parent has last 1024 bytes locked. */ > TEST_COMPARE (lseek64 (temp_fd, INT32_MAX-1024, SEEK_SET), INT32_MAX-1024); > - TEST_COMPARE (lockf64 (temp_fd, F_TEST, 1024), -1); > + TEST_COMPARE (LOCKF64 (temp_fd, F_TEST, 1024), -1); > > /* And try to lock [1024, 2048). */ > TEST_COMPARE (lseek64 (temp_fd, 1024, SEEK_SET), 1024); > - TEST_COMPARE (lockf64 (temp_fd, F_LOCK, 1024), 0); > + TEST_COMPARE (LOCKF64 (temp_fd, F_LOCK, 1024), 0); > > /* And also [INT32_MAX, INT32_MAX+1024). */ > { > off64_t off = (off64_t)INT32_MAX; > TEST_COMPARE (lseek64 (temp_fd, off, SEEK_SET), off); > - TEST_COMPARE (lockf64 (temp_fd, F_LOCK, 1024), 0); > + TEST_COMPARE (LOCKF64 (temp_fd, F_LOCK, 1024), 0); > } > > /* Check if [INT32_MAX+1024, INT64_MAX) is locked. */ > { > off64_t off = (off64_t)INT32_MAX+1024; > TEST_COMPARE (lseek64 (temp_fd, off, SEEK_SET), off); > - TEST_COMPARE (lockf64 (temp_fd, F_TLOCK, 1024), -1); > + TEST_COMPARE (LOCKF64 (temp_fd, F_TLOCK, 1024), -1); > TEST_COMPARE (errno, EAGAIN); > - TEST_COMPARE (lockf64 (temp_fd, F_TEST, 1024), -1); > + TEST_COMPARE (LOCKF64 (temp_fd, F_TEST, 1024), -1); > TEST_COMPARE (errno, EACCES); > } > } > @@ -97,38 +107,38 @@ static int > do_test (void) > { > /* Basic tests to check if a lock can be obtained and checked. */ > - TEST_COMPARE (lockf (temp_fd, F_LOCK, 1024), 0); > - TEST_COMPARE (lockf (temp_fd, F_LOCK, INT32_MAX), 0); > - TEST_COMPARE (lockf (temp_fd, F_TLOCK, 1024), 0); > - TEST_COMPARE (lockf (temp_fd, F_TEST, 1024), 0); > + TEST_COMPARE (LOCKF (temp_fd, F_LOCK, 1024), 0); > + TEST_COMPARE (LOCKF (temp_fd, F_LOCK, INT32_MAX), 0); > + TEST_COMPARE (LOCKF (temp_fd, F_TLOCK, 1024), 0); > + TEST_COMPARE (LOCKF (temp_fd, F_TEST, 1024), 0); > TEST_COMPARE (lseek (temp_fd, 1024, SEEK_SET), 1024); > - TEST_COMPARE (lockf (temp_fd, F_ULOCK, 1024), 0); > + TEST_COMPARE (LOCKF (temp_fd, F_ULOCK, 1024), 0); > /* Parent process should have ([0, 1024), [2048, INT32_MAX)) ranges locked. */ > > { > struct support_capture_subprocess result; > result = support_capture_subprocess (do_test_child_lockf, NULL); > - support_capture_subprocess_check (&result, "lockf", 0, sc_allow_none); > + support_capture_subprocess_check (&result, "LOCKF", 0, sc_allow_none); > } > > if (sizeof (off_t) != sizeof (off64_t)) > { > /* Check if previously locked regions with LFS symbol. */ > TEST_COMPARE (lseek (temp_fd, 0, SEEK_SET), 0); > - TEST_COMPARE (lockf64 (temp_fd, F_LOCK, 1024), 0); > - TEST_COMPARE (lockf64 (temp_fd, F_TLOCK, 1024), 0); > - TEST_COMPARE (lockf64 (temp_fd, F_TEST, 1024), 0); > + TEST_COMPARE (LOCKF64 (temp_fd, F_LOCK, 1024), 0); > + TEST_COMPARE (LOCKF64 (temp_fd, F_TLOCK, 1024), 0); > + TEST_COMPARE (LOCKF64 (temp_fd, F_TEST, 1024), 0); > /* Lock region [INT32_MAX+1024, INT64_MAX). */ > off64_t off = (off64_t)INT32_MAX + 1024; > TEST_COMPARE (lseek64 (temp_fd, off, SEEK_SET), off); > - TEST_COMPARE (lockf64 (temp_fd, F_LOCK, 1024), 0); > + TEST_COMPARE (LOCKF64 (temp_fd, F_LOCK, 1024), 0); > /* Parent process should have ([0, 1024), [2048, INT32_MAX), > [INT32_MAX+1024, INT64_MAX)) ranges locked. */ > > { > struct support_capture_subprocess result; > result = support_capture_subprocess (do_test_child_lockf64, NULL); > - support_capture_subprocess_check (&result, "lockf", 0, sc_allow_none); > + support_capture_subprocess_check (&result, "LOCKF", 0, sc_allow_none); > } > } OK. Test fcntl-as-lockf using the lockf test. > > diff --git a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h > index ca6a0d7516..1babbdc84e 100644 > --- a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h > +++ b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h > @@ -101,7 +101,7 @@ > #endif > > #ifndef F_GETLK OK. And this is the fix. Resolves Andreas' comment about parentheses. > -# ifndef __USE_FILE_OFFSET64 > +# if !defined __USE_FILE_OFFSET64 && __TIMESIZE != 64 Florian asked if there were any other cases of this, and you confirmed that there were no other instances that you could find that would indicate the same logical error elsewhere in the sources. Notes here from you: https://sourceware.org/pipermail/libc-alpha/2023-May/148502.html > # define F_GETLK 5 /* Get record locking info. */ > # define F_SETLK 6 /* Set record locking info (non-blocking). */ > # define F_SETLKW 7 /* Set record locking info (blocking). */ -- Cheers, Carlos.