From: Guo Ren <[email protected]>
There is no CONFIG_64BIT in userspace, we shouldn't limit it with
__BITS_PER_LONG == 32 to break the compatibility. Just export F_*64
definitions to userspace permanently.
gcc-11 -DHAVE_CONFIG_H -I./linux/x86_64 -I../../../src/linux/x86_64
-I./linux/generic -I../../../src/linux/generic -I. -I../../../src
-DIN_STRACE=1 -isystem /opt/kernel/include -Wall -Wextra
-Wno-missing-field-initializers -Wno-unused-parameter -Wdate-time
-Wformat-security -Wimplicit-fallthrough=5 -Winit-self -Wlogical-op
-Wmissing-prototypes -Wnested-externs -Wold-style-definition
-Wtrampolines -Wundef -Wwrite-strings -Werror -g -O2 -c -o
libstrace_a-fetch_bpf_fprog.o `test -f 'fetch_bpf_fprog.c' || echo
'../../../src/'`fetch_bpf_fprog.c
In file included from ../../../src/defs.h:404,
from ../../../src/fcntl.c:12:
../../../src/xlat/fcntlcmds.h:54:7: error: ‘F_GETLK64’ undeclared here
(not in a function); did you mean ‘F_GETLK’?
54 | XLAT(F_GETLK64),
| ^~~~~~~~~
../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
64 | # define XLAT(val) { (unsigned)(val), #val
}
| ^~~
../../../src/xlat/fcntlcmds.h:57:7: error: ‘F_SETLK64’ undeclared here
(not in a function); did you mean ‘F_SETLK’?
57 | XLAT(F_SETLK64),
| ^~~~~~~~~
../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
64 | # define XLAT(val) { (unsigned)(val), #val
}
| ^~~
../../../src/xlat/fcntlcmds.h:60:7: error: ‘F_SETLKW64’ undeclared here
(not in a function); did you mean ‘F_SETLKW’?
60 | XLAT(F_SETLKW64),
| ^~~~~~~~~~
../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
64 | # define XLAT(val) { (unsigned)(val), #val
}
| ^~~
make[4]: *** [Makefile:5017: libstrace_a-fcntl.o] Error 1
comment by Eugene:
Actually, it's quite the opposite: "ifndef" usage made it vailable at all
times to the userspace, and this change has actually broken building strace
with the latest kernel headers[1][2]. There could be some debate whether
having these F_*64 definitions exposed to the user space 64-bit
applications, but it seems that were no harm (as they were exposed already
for quite some time), and they are useful at least for strace for compat
application tracing purposes.
Fixes: 306f7cc1e9061 "uapi: always define F_GETLK64/F_SETLK64/F_SETLKW64 in fcntl.h"
Signed-off-by: Guo Ren <[email protected]>
Reported-by: Eugene Syromiatnikov <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Heiko Stuebner <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
---
include/uapi/asm-generic/fcntl.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index f13d37b60775..cd6bd65ec25d 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
-#if __BITS_PER_LONG == 32 || defined(__KERNEL__)
#ifndef F_GETLK64
#define F_GETLK64 12 /* using 'struct flock64' */
#define F_SETLK64 13
#define F_SETLKW64 14
#endif
-#endif /* __BITS_PER_LONG == 32 || defined(__KERNEL__) */
#ifndef F_SETOWN_EX
#define F_SETOWN_EX 15
--
2.36.1
Am Montag, 13. Juni 2022, 03:30:51 CEST schrieb [email protected]:
> From: Guo Ren <[email protected]>
>
> There is no CONFIG_64BIT in userspace, we shouldn't limit it with
> __BITS_PER_LONG == 32 to break the compatibility. Just export F_*64
> definitions to userspace permanently.
>
> gcc-11 -DHAVE_CONFIG_H -I./linux/x86_64 -I../../../src/linux/x86_64
> -I./linux/generic -I../../../src/linux/generic -I. -I../../../src
> -DIN_STRACE=1 -isystem /opt/kernel/include -Wall -Wextra
> -Wno-missing-field-initializers -Wno-unused-parameter -Wdate-time
> -Wformat-security -Wimplicit-fallthrough=5 -Winit-self -Wlogical-op
> -Wmissing-prototypes -Wnested-externs -Wold-style-definition
> -Wtrampolines -Wundef -Wwrite-strings -Werror -g -O2 -c -o
> libstrace_a-fetch_bpf_fprog.o `test -f 'fetch_bpf_fprog.c' || echo
> '../../../src/'`fetch_bpf_fprog.c
> In file included from ../../../src/defs.h:404,
> from ../../../src/fcntl.c:12:
> ../../../src/xlat/fcntlcmds.h:54:7: error: ‘F_GETLK64’ undeclared here
> (not in a function); did you mean ‘F_GETLK’?
> 54 | XLAT(F_GETLK64),
> | ^~~~~~~~~
> ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
> 64 | # define XLAT(val) { (unsigned)(val), #val
> }
> | ^~~
> ../../../src/xlat/fcntlcmds.h:57:7: error: ‘F_SETLK64’ undeclared here
> (not in a function); did you mean ‘F_SETLK’?
> 57 | XLAT(F_SETLK64),
> | ^~~~~~~~~
> ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
> 64 | # define XLAT(val) { (unsigned)(val), #val
> }
> | ^~~
> ../../../src/xlat/fcntlcmds.h:60:7: error: ‘F_SETLKW64’ undeclared here
> (not in a function); did you mean ‘F_SETLKW’?
> 60 | XLAT(F_SETLKW64),
> | ^~~~~~~~~~
> ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
> 64 | # define XLAT(val) { (unsigned)(val), #val
> }
> | ^~~
> make[4]: *** [Makefile:5017: libstrace_a-fcntl.o] Error 1
>
> comment by Eugene:
> Actually, it's quite the opposite: "ifndef" usage made it vailable at all
> times to the userspace, and this change has actually broken building strace
> with the latest kernel headers[1][2]. There could be some debate whether
> having these F_*64 definitions exposed to the user space 64-bit
> applications, but it seems that were no harm (as they were exposed already
> for quite some time), and they are useful at least for strace for compat
> application tracing purposes.
>
> Fixes: 306f7cc1e9061 "uapi: always define F_GETLK64/F_SETLK64/F_SETLKW64 in fcntl.h"
> Signed-off-by: Guo Ren <[email protected]>
> Reported-by: Eugene Syromiatnikov <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Heiko Stuebner <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> ---
> include/uapi/asm-generic/fcntl.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
> index f13d37b60775..cd6bd65ec25d 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
>
> -#if __BITS_PER_LONG == 32 || defined(__KERNEL__)
> #ifndef F_GETLK64
> #define F_GETLK64 12 /* using 'struct flock64' */
> #define F_SETLK64 13
> #define F_SETLKW64 14
> #endif
> -#endif /* __BITS_PER_LONG == 32 || defined(__KERNEL__) */
Looks like prviously there were a number of ways these constants
were ifdef'd - or not. A number of platforms already had no ifdef of
any sort around them before, so this looks like the sane way to do it.
Though in the original patch the special-mips-variant also gained the
#if __BITS_PER_LONG == 32 ...
conditional in arch/mips/include/uapi/asm/fcntl.h .
So, is it also affected by this issue?
Heiko
The change looks fine, but the commit log could use some work, please
move the notes from the back to the front and make them a standalone
commit log and just drop the rest.
On Mon, 13 Jun 2022 07:29:47 PDT (-0700), Christoph Hellwig wrote:
> The change looks fine, but the commit log could use some work, please
> move the notes from the back to the front and make them a standalone
> commit log and just drop the rest.
I'm digging through old stuff and don't see a v2, not sure if I just
missed it. Either way I don't think it's really a RISC-V thing so
Acked-by: Palmer Dabbelt <[email protected]>
in case that helps.
Thanks!
On Mon, Jun 13, 2022 at 10:29 PM Christoph Hellwig <[email protected]> wrote:
>
> The change looks fine, but the commit log could use some work, please
> move the notes from the back to the front and make them a standalone
> commit log and just drop the rest.
Okay.
--
Best Regards
Guo Ren
Thx for the reply, I even forgot that patch. I would update V2 with
your acked, and Christoph advice.
On Thu, Aug 4, 2022 at 8:40 AM Palmer Dabbelt <[email protected]> wrote:
>
> On Mon, 13 Jun 2022 07:29:47 PDT (-0700), Christoph Hellwig wrote:
> > The change looks fine, but the commit log could use some work, please
> > move the notes from the back to the front and make them a standalone
> > commit log and just drop the rest.
>
> I'm digging through old stuff and don't see a v2, not sure if I just
> missed it. Either way I don't think it's really a RISC-V thing so
>
> Acked-by: Palmer Dabbelt <[email protected]>
>
> in case that helps.
>
> Thanks!
--
Best Regards
Guo Ren
On Mon, Jun 13, 2022 at 9:58 PM Heiko Stübner <[email protected]> wrote:
>
> Am Montag, 13. Juni 2022, 03:30:51 CEST schrieb [email protected]:
> > From: Guo Ren <[email protected]>
> >
> > There is no CONFIG_64BIT in userspace, we shouldn't limit it with
> > __BITS_PER_LONG == 32 to break the compatibility. Just export F_*64
> > definitions to userspace permanently.
> >
> > gcc-11 -DHAVE_CONFIG_H -I./linux/x86_64 -I../../../src/linux/x86_64
> > -I./linux/generic -I../../../src/linux/generic -I. -I../../../src
> > -DIN_STRACE=1 -isystem /opt/kernel/include -Wall -Wextra
> > -Wno-missing-field-initializers -Wno-unused-parameter -Wdate-time
> > -Wformat-security -Wimplicit-fallthrough=5 -Winit-self -Wlogical-op
> > -Wmissing-prototypes -Wnested-externs -Wold-style-definition
> > -Wtrampolines -Wundef -Wwrite-strings -Werror -g -O2 -c -o
> > libstrace_a-fetch_bpf_fprog.o `test -f 'fetch_bpf_fprog.c' || echo
> > '../../../src/'`fetch_bpf_fprog.c
> > In file included from ../../../src/defs.h:404,
> > from ../../../src/fcntl.c:12:
> > ../../../src/xlat/fcntlcmds.h:54:7: error: ‘F_GETLK64’ undeclared here
> > (not in a function); did you mean ‘F_GETLK’?
> > 54 | XLAT(F_GETLK64),
> > | ^~~~~~~~~
> > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
> > 64 | # define XLAT(val) { (unsigned)(val), #val
> > }
> > | ^~~
> > ../../../src/xlat/fcntlcmds.h:57:7: error: ‘F_SETLK64’ undeclared here
> > (not in a function); did you mean ‘F_SETLK’?
> > 57 | XLAT(F_SETLK64),
> > | ^~~~~~~~~
> > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
> > 64 | # define XLAT(val) { (unsigned)(val), #val
> > }
> > | ^~~
> > ../../../src/xlat/fcntlcmds.h:60:7: error: ‘F_SETLKW64’ undeclared here
> > (not in a function); did you mean ‘F_SETLKW’?
> > 60 | XLAT(F_SETLKW64),
> > | ^~~~~~~~~~
> > ../../../src/xlat.h:64:54: note: in definition of macro ‘XLAT’
> > 64 | # define XLAT(val) { (unsigned)(val), #val
> > }
> > | ^~~
> > make[4]: *** [Makefile:5017: libstrace_a-fcntl.o] Error 1
> >
> > comment by Eugene:
> > Actually, it's quite the opposite: "ifndef" usage made it vailable at all
> > times to the userspace, and this change has actually broken building strace
> > with the latest kernel headers[1][2]. There could be some debate whether
> > having these F_*64 definitions exposed to the user space 64-bit
> > applications, but it seems that were no harm (as they were exposed already
> > for quite some time), and they are useful at least for strace for compat
> > application tracing purposes.
> >
> > Fixes: 306f7cc1e9061 "uapi: always define F_GETLK64/F_SETLK64/F_SETLKW64 in fcntl.h"
> > Signed-off-by: Guo Ren <[email protected]>
> > Reported-by: Eugene Syromiatnikov <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Heiko Stuebner <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > ---
> > include/uapi/asm-generic/fcntl.h | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
> > index f13d37b60775..cd6bd65ec25d 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
> >
> > -#if __BITS_PER_LONG == 32 || defined(__KERNEL__)
> > #ifndef F_GETLK64
> > #define F_GETLK64 12 /* using 'struct flock64' */
> > #define F_SETLK64 13
> > #define F_SETLKW64 14
> > #endif
> > -#endif /* __BITS_PER_LONG == 32 || defined(__KERNEL__) */
>
> Looks like prviously there were a number of ways these constants
> were ifdef'd - or not. A number of platforms already had no ifdef of
> any sort around them before, so this looks like the sane way to do it.
>
> Though in the original patch the special-mips-variant also gained the
> #if __BITS_PER_LONG == 32 ...
> conditional in arch/mips/include/uapi/asm/fcntl.h .
> So, is it also affected by this issue?
Not sure about mips.
>
>
> Heiko
>
>
--
Best Regards
Guo Ren