| Message ID | 20211228143958.3409187-4-guoren@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: compat: Add COMPAT mode support for rv64 | expand |
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
Le 28/12/2021 à 15:39, guoren@kernel.org a écrit : > 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> > --- > arch/arm64/include/asm/compat.h | 4 ---- > arch/powerpc/include/asm/compat.h | 4 ---- > arch/s390/include/asm/compat.h | 4 ---- > arch/sparc/include/asm/compat.h | 4 ---- > arch/x86/include/asm/compat.h | 4 ---- > include/uapi/asm-generic/fcntl.h | 2 +- > 6 files changed, 1 insertion(+), 21 deletions(-) > ... > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > index ecd0f5bdfc1d..5bc1e51d73b1 100644 > --- a/include/uapi/asm-generic/fcntl.h > +++ b/include/uapi/asm-generic/fcntl.h > @@ -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 There seems to be a problem with this change: error: /linux/include/uapi/asm-generic/fcntl.h: leak CONFIG_COMPAT to user-space make[3]: *** [/linux/scripts/Makefile.headersinst:63: usr/include/asm-generic/fcntl.h] Error 1 make[3]: *** Waiting for unfinished jobs.... make[2]: *** [/linux/Makefile:1283: headers] Error 2 make[1]: *** [Makefile:219: __sub-make] Error 2 make[2]: Leaving directory '/output' make[1]: Leaving directory '/linux' make: *** [Makefile:157: khdr] Error 2 make: Leaving directory '/linux/tools/testing/selftests' ## Selftest build completed rc = 2 ## Found 2 binaries !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! !! Error build failed rc 2 !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Error: Process completed with exit code 2.
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
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
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index eaa6ca062d89..276328765408 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -73,10 +73,6 @@ struct compat_flock { compat_pid_t l_pid; }; -#define F_GETLK64 12 /* using 'struct flock64' */ -#define F_SETLK64 13 -#define F_SETLKW64 14 - struct compat_flock64 { short l_type; short l_whence; diff --git a/arch/powerpc/include/asm/compat.h b/arch/powerpc/include/asm/compat.h index 7afc96fb6524..83d8f70779cb 100644 --- a/arch/powerpc/include/asm/compat.h +++ b/arch/powerpc/include/asm/compat.h @@ -52,10 +52,6 @@ struct compat_flock { compat_pid_t l_pid; }; -#define F_GETLK64 12 /* using 'struct flock64' */ -#define F_SETLK64 13 -#define F_SETLKW64 14 - struct compat_flock64 { short l_type; short l_whence; diff --git a/arch/s390/include/asm/compat.h b/arch/s390/include/asm/compat.h index cdc7ae72529d..0f14b3188b1b 100644 --- a/arch/s390/include/asm/compat.h +++ b/arch/s390/include/asm/compat.h @@ -110,10 +110,6 @@ struct compat_flock { compat_pid_t l_pid; }; -#define F_GETLK64 12 -#define F_SETLK64 13 -#define F_SETLKW64 14 - struct compat_flock64 { short l_type; short l_whence; diff --git a/arch/sparc/include/asm/compat.h b/arch/sparc/include/asm/compat.h index bd949fcf9d63..108078751bb5 100644 --- a/arch/sparc/include/asm/compat.h +++ b/arch/sparc/include/asm/compat.h @@ -84,10 +84,6 @@ struct compat_flock { short __unused; }; -#define F_GETLK64 12 -#define F_SETLK64 13 -#define F_SETLKW64 14 - struct compat_flock64 { short l_type; short l_whence; diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h index 7516e4199b3c..8d19a212f4f2 100644 --- a/arch/x86/include/asm/compat.h +++ b/arch/x86/include/asm/compat.h @@ -58,10 +58,6 @@ struct compat_flock { compat_pid_t l_pid; }; -#define F_GETLK64 12 /* using 'struct flock64' */ -#define F_SETLK64 13 -#define F_SETLKW64 14 - /* * IA32 uses 4 byte alignment for 64 bit quantities, * so we need to pack this structure. diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h index ecd0f5bdfc1d..5bc1e51d73b1 100644 --- a/include/uapi/asm-generic/fcntl.h +++ b/include/uapi/asm-generic/fcntl.h @@ -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