* Re: [PATCH V2 03/17] asm-generic: fcntl: compat: Remove duplicate definitions
2022-01-10 13:35 ` [PATCH V2 03/17] asm-generic: fcntl: compat: Remove duplicate definitions Arnd Bergmann
@ 2022-01-10 16:30 ` Christoph Hellwig
2022-01-11 2:43 ` Guo Ren
1 sibling, 0 replies; 3+ messages in thread
From: Christoph Hellwig @ 2022-01-10 16:30 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Guo Ren, Palmer Dabbelt, Anup Patel, gregkh, liush, Wei Fu,
Drew Fustini, Wang Junqiang, Christoph Hellwig,
Linux Kernel Mailing List, linux-riscv, linux-csky, linux-s390,
sparclinux, linuxppc-dev, inux-parisc,
open list:BROADCOM NVRAM DRIVER, Linux ARM,
the arch/x86 maintainers, Guo Ren, Jeff Layton, J. Bruce Fields,
Linux FS-devel Mailing List
On Mon, Jan 10, 2022 at 02:35:19PM +0100, Arnd Bergmann wrote:
> > +#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.
FYI, this is what I did in my old branch, which also sidesteps the
duplicate value problem on parisc. The rebase is untested so far,
but I can spend some cycles on finishing it:
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/fcntl-asm-generic-cleanup
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH V2 03/17] asm-generic: fcntl: compat: Remove duplicate definitions
2022-01-10 13:35 ` [PATCH V2 03/17] asm-generic: fcntl: compat: Remove duplicate definitions Arnd Bergmann
2022-01-10 16:30 ` Christoph Hellwig
@ 2022-01-11 2:43 ` Guo Ren
1 sibling, 0 replies; 3+ messages in thread
From: Guo Ren @ 2022-01-11 2:43 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Palmer Dabbelt, Anup Patel, gregkh, liush, Wei Fu, Drew Fustini,
Wang Junqiang, Christoph Hellwig, Linux Kernel Mailing List,
linux-riscv, linux-csky, linux-s390, sparclinux, linuxppc-dev,
inux-parisc, open list:BROADCOM NVRAM DRIVER, Linux ARM,
the arch/x86 maintainers, Guo Ren, Jeff Layton, J. Bruce Fields,
Linux FS-devel Mailing List
On Mon, Jan 10, 2022 at 9:35 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> 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.
Okay.
>
> Arnd
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 3+ messages in thread