diff mbox series

[4/5] uapi: always define F_GETLK64/F_SETLK64/F_SETLKW64 in fcntl.h

Message ID 20220111083515.502308-5-hch@lst.de (mailing list archive)
State Superseded
Headers show
Series [1/5] uapi: remove the unused HAVE_ARCH_STRUCT_FLOCK64 define | expand

Commit Message

Christoph Hellwig Jan. 11, 2022, 8:35 a.m. UTC
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(-)

Comments

Arnd Bergmann Jan. 11, 2022, 3:33 p.m. UTC | #1
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
Guo Ren Jan. 12, 2022, 2:08 a.m. UTC | #2
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
Christoph Hellwig Jan. 12, 2022, 7:56 a.m. UTC | #3
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.
Arnd Bergmann Jan. 12, 2022, 8:28 a.m. UTC | #4
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
Jeff Layton Jan. 12, 2022, 11:15 a.m. UTC | #5
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>
Arnd Bergmann Jan. 12, 2022, 12:08 p.m. UTC | #6
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
Christoph Hellwig Jan. 12, 2022, 4:09 p.m. UTC | #7
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 mbox series

Patch

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