| Message ID | 20220111083515.502308-5-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] uapi: remove the unused HAVE_ARCH_STRUCT_FLOCK64 define | expand |
On Tue, Jan 11, 2022 at 9:35 AM Christoph Hellwig <hch@lst.de> wrote: > > The fcntl F_GETLK64/F_SETLK64/F_SETLKW64 are only implemented for the > 32-bit syscall APIs, but we also need them for compat handling on 64-bit > builds. Redefining them is error prone (as shown by the example that > parisc gets it wrong currently), so we should use the same defines for > both case. In theory we could try to hide them from userspace, but > given that only MIPS actually gets that right, while the asm-generic > version used by most architectures relies on a Kconfig symbol that can't > be relied on to be set properly by userspace is a clear indicator to not > bother. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > index 98f4ff165b776..43d7c44031be0 100644 > --- a/include/uapi/asm-generic/fcntl.h > +++ b/include/uapi/asm-generic/fcntl.h > @@ -116,13 +116,11 @@ > #define F_GETSIG 11 /* for sockets. */ > #endif > > -#ifndef CONFIG_64BIT > #ifndef F_GETLK64 > #define F_GETLK64 12 /* using 'struct flock64' */ > #define F_SETLK64 13 > #define F_SETLKW64 14 > #endif > -#endif > > #ifndef F_SETOWN_EX > #define F_SETOWN_EX 15 This is a very subtle change to the exported UAPI header contents: On 64-bit architectures, the three unusable numbers are now always shown, rather than depending on a user-controlled symbol. This is probably what we want here for compatibility reasons, but I think it should be explained in the changelog text, and I'd like Jeff or Bruce to comment on it as well: the alternative here would be to make the uapi definition depend on __BITS_PER_LONG==32, which is technically the right thing to do but more a of a change. Arnd
On Tue, Jan 11, 2022 at 11:33 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Jan 11, 2022 at 9:35 AM Christoph Hellwig <hch@lst.de> wrote: > > > > The fcntl F_GETLK64/F_SETLK64/F_SETLKW64 are only implemented for the > > 32-bit syscall APIs, but we also need them for compat handling on 64-bit > > builds. Redefining them is error prone (as shown by the example that > > parisc gets it wrong currently), so we should use the same defines for > > both case. In theory we could try to hide them from userspace, but > > given that only MIPS actually gets that right, while the asm-generic > > version used by most architectures relies on a Kconfig symbol that can't > > be relied on to be set properly by userspace is a clear indicator to not > > bother. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > > index 98f4ff165b776..43d7c44031be0 100644 > > --- a/include/uapi/asm-generic/fcntl.h > > +++ b/include/uapi/asm-generic/fcntl.h > > @@ -116,13 +116,11 @@ > > #define F_GETSIG 11 /* for sockets. */ > > #endif > > > > -#ifndef CONFIG_64BIT > > #ifndef F_GETLK64 > > #define F_GETLK64 12 /* using 'struct flock64' */ > > #define F_SETLK64 13 > > #define F_SETLKW64 14 > > #endif > > -#endif > > > > #ifndef F_SETOWN_EX > > #define F_SETOWN_EX 15 > > This is a very subtle change to the exported UAPI header contents: > On 64-bit architectures, the three unusable numbers are now always > shown, rather than depending on a user-controlled symbol. > > This is probably what we want here for compatibility reasons, but I think > it should be explained in the changelog text, and I'd like Jeff or Bruce > to comment on it as well: the alternative here would be to make the > uapi definition depend on __BITS_PER_LONG==32, which is __BITS_PER_LONG==32 || __KERNEL__ just for kernel use in compat. > technically the right thing to do but more a of a change. > > Arnd
On Tue, Jan 11, 2022 at 04:33:30PM +0100, Arnd Bergmann wrote: > This is a very subtle change to the exported UAPI header contents: > On 64-bit architectures, the three unusable numbers are now always > shown, rather than depending on a user-controlled symbol. Well, the change is bigger and less subtle. Before this change the constants were never visible to userspace at all (except on mips), because the #ifdef CONFIG_64BIT it never set for userspace builds. > This is probably what we want here for compatibility reasons, but I think > it should be explained in the changelog text, and I'd like Jeff or Bruce > to comment on it as well: the alternative here would be to make the > uapi definition depend on __BITS_PER_LONG==32, which is > technically the right thing to do but more a of a change. I can change this to #if __BITS_PER_LONG==32 || defined(__KERNEL__), but it will still be change in what userspace sees.
On Wed, Jan 12, 2022 at 8:56 AM Christoph Hellwig <hch@lst.de> wrote: > > On Tue, Jan 11, 2022 at 04:33:30PM +0100, Arnd Bergmann wrote: > > This is a very subtle change to the exported UAPI header contents: > > On 64-bit architectures, the three unusable numbers are now always > > shown, rather than depending on a user-controlled symbol. > > Well, the change is bigger and less subtle. Before this change the > constants were never visible to userspace at all (except on mips), > because the #ifdef CONFIG_64BIT it never set for userspace builds. I suppose you mean /always/ visible here, with that ifndef. > > This is probably what we want here for compatibility reasons, but I think > > it should be explained in the changelog text, and I'd like Jeff or Bruce > > to comment on it as well: the alternative here would be to make the > > uapi definition depend on __BITS_PER_LONG==32, which is > > technically the right thing to do but more a of a change. > > I can change this to #if __BITS_PER_LONG==32 || defined(__KERNEL__), > but it will still be change in what userspace sees. Exactly, that is the tradeoff, which is why I'd like the flock maintainers to say which way they prefer. We can either do it more correctly (hiding the constants from user space when they are not usable), or with less change (removing the incorrect #ifdef). Either way sounds reasonable to me, I mainly care that this is explained in the changelog and that the maintainers are aware of the two options. Arnd
On Wed, 2022-01-12 at 09:28 +0100, Arnd Bergmann wrote: > On Wed, Jan 12, 2022 at 8:56 AM Christoph Hellwig <hch@lst.de> wrote: > > > > On Tue, Jan 11, 2022 at 04:33:30PM +0100, Arnd Bergmann wrote: > > > This is a very subtle change to the exported UAPI header contents: > > > On 64-bit architectures, the three unusable numbers are now always > > > shown, rather than depending on a user-controlled symbol. > > > > Well, the change is bigger and less subtle. Before this change the > > constants were never visible to userspace at all (except on mips), > > because the #ifdef CONFIG_64BIT it never set for userspace builds. > > I suppose you mean /always/ visible here, with that ifndef. > > > > This is probably what we want here for compatibility reasons, but I think > > > it should be explained in the changelog text, and I'd like Jeff or Bruce > > > to comment on it as well: the alternative here would be to make the > > > uapi definition depend on __BITS_PER_LONG==32, which is > > > technically the right thing to do but more a of a change. > > > > I can change this to #if __BITS_PER_LONG==32 || defined(__KERNEL__), > > but it will still be change in what userspace sees. > > Exactly, that is the tradeoff, which is why I'd like the flock maintainers > to say which way they prefer. We can either do it more correctly (hiding > the constants from user space when they are not usable), or with less > change (removing the incorrect #ifdef). Either way sounds reasonable > to me, I mainly care that this is explained in the changelog and that the > maintainers are aware of the two options. > I don't have a strong opinion here. If we were taking symbols away that were previously visible to userland it would be one thing, but since we're just adding symbols that may not have been there before, this seems less likely to break anything. I probably lean toward Christoph's original solution instead of keeping the conditional definitions. It's hard to imagine there are many programs that care whether these other symbols are defined or not. You can add this to the original patch: Acked-by: Jeff Layton <jlayton@kernel.org>
On Wed, Jan 12, 2022 at 12:15 PM Jeff Layton <jlayton@kernel.org> wrote: > On Wed, 2022-01-12 at 09:28 +0100, Arnd Bergmann wrote: > > On Wed, Jan 12, 2022 at 8:56 AM Christoph Hellwig <hch@lst.de> wrote: > > > > Exactly, that is the tradeoff, which is why I'd like the flock maintainers > > to say which way they prefer. We can either do it more correctly (hiding > > the constants from user space when they are not usable), or with less > > change (removing the incorrect #ifdef). Either way sounds reasonable > > to me, I mainly care that this is explained in the changelog and that the > > maintainers are aware of the two options. > > > > I don't have a strong opinion here. If we were taking symbols away that > were previously visible to userland it would be one thing, but since > we're just adding symbols that may not have been there before, this > seems less likely to break anything. Changing #ifndef CONFIG_64BIT to #if __BITS_PER_LONG==32 || defined(__KERNEL__), would take symbols away, since the CONFIG_64BIT macro is never set in user space. > I probably lean toward Christoph's original solution instead of keeping > the conditional definitions. It's hard to imagine there are many > programs that care whether these other symbols are defined or not. > > You can add this to the original patch: > > Acked-by: Jeff Layton <jlayton@kernel.org> Sounds good, thanks Arnd
On Wed, Jan 12, 2022 at 01:08:24PM +0100, Arnd Bergmann wrote: > > I don't have a strong opinion here. If we were taking symbols away that > > were previously visible to userland it would be one thing, but since > > we're just adding symbols that may not have been there before, this > > seems less likely to break anything. > > Changing > > #ifndef CONFIG_64BIT > > to > > #if __BITS_PER_LONG==32 || defined(__KERNEL__), > > would take symbols away, since the CONFIG_64BIT macro is never > set in user space. Yes. > > I probably lean toward Christoph's original solution instead of keeping > > the conditional definitions. It's hard to imagine there are many > > programs that care whether these other symbols are defined or not. > > > > You can add this to the original patch: > > > > Acked-by: Jeff Layton <jlayton@kernel.org> > > Sounds good, thanks So should we go ahead with the series as-is? Or respin it? Or add the above change ontop?
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index eaa6ca062d89b..2763287654081 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/mips/include/asm/compat.h b/arch/mips/include/asm/compat.h index bbb3bc5a42fd8..6a350c1f70d7e 100644 --- a/arch/mips/include/asm/compat.h +++ b/arch/mips/include/asm/compat.h @@ -65,10 +65,6 @@ struct compat_flock { s32 pad[4]; }; -#define F_GETLK64 33 -#define F_SETLK64 34 -#define F_SETLKW64 35 - struct compat_flock64 { short l_type; short l_whence; diff --git a/arch/mips/include/uapi/asm/fcntl.h b/arch/mips/include/uapi/asm/fcntl.h index 9e44ac810db94..1769fc50d35f0 100644 --- a/arch/mips/include/uapi/asm/fcntl.h +++ b/arch/mips/include/uapi/asm/fcntl.h @@ -44,11 +44,9 @@ #define F_SETOWN 24 /* for sockets. */ #define F_GETOWN 23 /* for sockets. */ -#ifndef __mips64 #define F_GETLK64 33 /* using 'struct flock64' */ #define F_SETLK64 34 #define F_SETLKW64 35 -#endif #if _MIPS_SIM != _MIPS_SIM_ABI64 #define __ARCH_FLOCK_EXTRA_SYSID long l_sysid; diff --git a/arch/powerpc/include/asm/compat.h b/arch/powerpc/include/asm/compat.h index 7afc96fb6524b..83d8f70779cbc 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 cdc7ae72529d8..0f14b3188b1bb 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 bd949fcf9d63b..108078751bb5a 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 7516e4199b3c6..8d19a212f4f26 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 98f4ff165b776..43d7c44031be0 100644 --- a/include/uapi/asm-generic/fcntl.h +++ b/include/uapi/asm-generic/fcntl.h @@ -116,13 +116,11 @@ #define F_GETSIG 11 /* for sockets. */ #endif -#ifndef CONFIG_64BIT #ifndef F_GETLK64 #define F_GETLK64 12 /* using 'struct flock64' */ #define F_SETLK64 13 #define F_SETLKW64 14 #endif -#endif #ifndef F_SETOWN_EX #define F_SETOWN_EX 15 diff --git a/tools/include/uapi/asm-generic/fcntl.h b/tools/include/uapi/asm-generic/fcntl.h index bf961a71802e0..6e16722026f39 100644 --- a/tools/include/uapi/asm-generic/fcntl.h +++ b/tools/include/uapi/asm-generic/fcntl.h @@ -115,13 +115,11 @@ #define F_GETSIG 11 /* for sockets. */ #endif -#ifndef CONFIG_64BIT #ifndef F_GETLK64 #define F_GETLK64 12 /* using 'struct flock64' */ #define F_SETLK64 13 #define F_SETLKW64 14 #endif -#endif #ifndef F_SETOWN_EX #define F_SETOWN_EX 15
The fcntl F_GETLK64/F_SETLK64/F_SETLKW64 are only implemented for the 32-bit syscall APIs, but we also need them for compat handling on 64-bit builds. Redefining them is error prone (as shown by the example that parisc gets it wrong currently), so we should use the same defines for both case. In theory we could try to hide them from userspace, but given that only MIPS actually gets that right, while the asm-generic version used by most architectures relies on a Kconfig symbol that can't be relied on to be set properly by userspace is a clear indicator to not bother. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/arm64/include/asm/compat.h | 4 ---- arch/mips/include/asm/compat.h | 4 ---- arch/mips/include/uapi/asm/fcntl.h | 2 -- 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 -- tools/include/uapi/asm-generic/fcntl.h | 2 -- 9 files changed, 30 deletions(-)