Re: [PATCH V2 03/17] asm-generic: fcntl: compat: Remove duplicate definitions

From: Arnd Bergmann <arnd@arndb.de>
To: Guo Ren <guoren@kernel.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	Arnd Bergmann <arnd@arndb.de>, Anup Patel <anup.patel@wdc.com>,
	 gregkh <gregkh@linuxfoundation.org>,
	liush <liush@allwinnertech.com>,  Wei Fu <wefu@redhat.com>,
	Drew Fustini <drew@beagleboard.org>,
	 Wang Junqiang <wangjunqiang@iscas.ac.cn>,
	Christoph Hellwig <hch@infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	 linux-riscv <linux-riscv@lists.infradead.org>,
	linux-csky@vger.kernel.org,
	 linux-s390 <linux-s390@vger.kernel.org>,
	sparclinux <sparclinux@vger.kernel.org>,
	 linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	inux-parisc@vger.kernel.org,
	 "open list:BROADCOM NVRAM DRIVER" <linux-mips@vger.kernel.org>,
	 Linux ARM <linux-arm-kernel@lists.infradead.org>,
	 "the arch/x86 maintainers" <x86@kernel.org>,
	Guo Ren <guoren@linux.alibaba.com>,
	Jeff Layton <jlayton@kernel.org>,
	"J. Bruce Fields" <bfields@fieldses.org>,
	 Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH V2 03/17] asm-generic: fcntl: compat: Remove duplicate definitions
Date: Mon, 10 Jan 2022 14:35:19 +0100	[thread overview]
Message-ID: <CAK8P3a2zn9M6X09WsjJ9HYiS9WnO_YPCvJLSBk+HaH+yZHQqfA@mail.gmail.com> (raw)
In-Reply-To: <20211228143958.3409187-4-guoren@kernel.org>

On Tue, Dec 28, 2021 at 3:39 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> Remove duplicate F_GETLK64,F_SETLK64,F_SETLKW64 definitions in
> arch/*/include/asm/compat.h.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>

Unfortunately, this one does not look correct to me:

> @@ -116,7 +116,7 @@
>  #define F_GETSIG       11      /* for sockets. */
>  #endif
>
> -#ifndef CONFIG_64BIT
> +#if !defined(CONFIG_64BIT) || defined(CONFIG_COMPAT)
>  #ifndef F_GETLK64
>  #define F_GETLK64      12      /*  using 'struct flock64' */
>  #define F_SETLK64      13

The problem here is that include/uapi/ headers cannot contain checks for
CONFIG_* symbols because those may have different meanings in user space
compared to kernel.

This is a preexisting problem in the header, but I think the change
makes it worse.

With the current behavior, user space will always see the definitions,
unless it happens to have its own definition for CONFIG_64BIT already.
On 64-bit parisc, this has the effect of defining the macros to the
same values as F_SETOWN/F_SETSIG/F_GETSIG, which is potentially
harmful. On MIPS, it uses values that are different from the 32-bit numbers
but are otherwise unused. Everywhere else, we get the definition from
the 32-bit architecture in user space, which will do nothing in the kernel.

The correct check for a uapi header would be to test for
__BITS_PER_LONG==32. We should probably do that here, but
this won't help you move the definitions, and it is a user-visible change
as the incorrect definition will no longer be visible. [Adding Jeff and Bruce
(the flock mainainers) to Cc for additional feedback on this]

For your series, I would suggest just moving the macro definitions to
include/linux/compat.h along with the 'struct compat_flock64'
definition, and leaving the duplicate one in the uapi header unchanged
until we have decided on a solution.

        Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel