[PATCH] Remove #ifdef CONFIG_64BIT from all asm-generic/fcntl.h

[PATCH] Remove #ifdef CONFIG_64BIT from all asm-generic/fcntl.h

[Palmer Dabbelt]

From: Palmer Dabbelt <palmer.dabbelt@eecs.berkeley.edu>

When working on the RISC-V port I noticed that F_SETLK64 was being
defined on our 64-bit platform, despite our port being so new that
we've only ever had the 64-bit file ops.  Since there's not compat
layer for these, this causes fcntl to bail out.

It turns out that one of the ways in with F_SETLK64 was being defined
(there's some more in glibc, but that's a whole different story... :))
is the result of CONFIG_64BIT showing up in this user-visible header.
<asm-generic/bitsperlong.h> confirms this isn't sane, so I replaced it
with a __BITS_PER_LONG check.

I went ahead and grep'd for any more of these (with
headers_install_all), and this was the only one I found.

Signed-off-by: Palmer Dabbelt <palmer.dabbelt@eecs.berkeley.edu>
Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
---
 include/uapi/asm-generic/fcntl.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index e063effe0cc1..14a5c8237d84 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -1,6 +1,7 @@
 #ifndef _ASM_GENERIC_FCNTL_H
 #define _ASM_GENERIC_FCNTL_H
 
+#include <asm/bitsperlong.h>
 #include <linux/types.h>
 
 /*
@@ -115,7 +116,7 @@
 #define F_GETSIG	11	/* for sockets. */
 #endif
 
-#ifndef CONFIG_64BIT
+#if (__BITS_PER_LONG == 32)
 #ifndef F_GETLK64
 #define F_GETLK64	12	/*  using 'struct flock64' */
 #define F_SETLK64	13
-- 
2.4.6

Re: [PATCH] Remove #ifdef CONFIG_64BIT from all asm-generic/fcntl.h

[Arnd Bergmann]

On Tuesday 01 September 2015 17:10:10 Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmer.dabbelt@eecs.berkeley.edu>
> 
> When working on the RISC-V port I noticed that F_SETLK64 was being
> defined on our 64-bit platform, despite our port being so new that
> we've only ever had the 64-bit file ops.  Since there's not compat
> layer for these, this causes fcntl to bail out.
> 
> It turns out that one of the ways in with F_SETLK64 was being defined
> (there's some more in glibc, but that's a whole different story... :))
> is the result of CONFIG_64BIT showing up in this user-visible header.
> <asm-generic/bitsperlong.h> confirms this isn't sane, so I replaced it
> with a __BITS_PER_LONG check.
> 
> I went ahead and grep'd for any more of these (with
> headers_install_all), and this was the only one I found.
> 
> Signed-off-by: Palmer Dabbelt <palmer.dabbelt@eecs.berkeley.edu>
> Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
> Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>

Looks good to me. Are you planning to submit the RISC-V port upstream
any time soon? If so, just keep the patch in your tree and add my

Acked-by: Arnd Bergmann <arnd@arndb.de>

However, I did see a lot of similar bugs now that you point me to it:

$  grep -r \\\<CONFIG obj-tmp/usr/include/
obj-tmp/usr/include/asm-generic/fcntl.h:#ifndef CONFIG_64BIT
obj-tmp/usr/include/asm-generic/mman-common.h:#ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED
obj-tmp/usr/include/asm-generic/unistd.h:#ifdef CONFIG_MMU
obj-tmp/usr/include/asm-generic/unistd.h:#endif /* CONFIG_MMU */
obj-tmp/usr/include/linux/atmdev.h:#ifdef CONFIG_COMPAT
obj-tmp/usr/include/linux/elfcore.h:#ifdef CONFIG_BINFMT_ELF_FDPIC
obj-tmp/usr/include/linux/eventpoll.h:#ifdef CONFIG_PM_SLEEP
obj-tmp/usr/include/linux/fb.h:#ifdef CONFIG_FB_BACKLIGHT
obj-tmp/usr/include/linux/flat.h:#ifdef CONFIG_BINFMT_SHARED_FLAT
obj-tmp/usr/include/linux/hw_breakpoint.h:#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
obj-tmp/usr/include/linux/pktcdvd.h:#if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
obj-tmp/usr/include/linux/raw.h:#define MAX_RAW_MINORS CONFIG_MAX_RAW_DEVS
obj-tmp/usr/include/asm/ptrace.h:#ifdef CONFIG_CPU_ENDIAN_BE8

These all have the same problem, and we should fix them, as well as
(probably) adding an automated check to scripts/headers_install.sh.

	Arnd

Re: [PATCH] Remove #ifdef CONFIG_64BIT from all asm-generic/fcntl.h

[Palmer Dabbelt]

On Mon, 07 Sep 2015 06:16:37 PDT (-0700), arnd@arndb.de wrote:
> On Tuesday 01 September 2015 17:10:10 Palmer Dabbelt wrote:
>> From: Palmer Dabbelt <palmer.dabbelt@eecs.berkeley.edu>
>>
>> When working on the RISC-V port I noticed that F_SETLK64 was being
>> defined on our 64-bit platform, despite our port being so new that
>> we've only ever had the 64-bit file ops.  Since there's not compat
>> layer for these, this causes fcntl to bail out.
>>
>> It turns out that one of the ways in with F_SETLK64 was being defined
>> (there's some more in glibc, but that's a whole different story... :))
>> is the result of CONFIG_64BIT showing up in this user-visible header.
>> <asm-generic/bitsperlong.h> confirms this isn't sane, so I replaced it
>> with a __BITS_PER_LONG check.
>>
>> I went ahead and grep'd for any more of these (with
>> headers_install_all), and this was the only one I found.
>>
>> Signed-off-by: Palmer Dabbelt <palmer.dabbelt@eecs.berkeley.edu>
>> Reviewed-by: Andrew Waterman <waterman@eecs.berkeley.edu>
>> Reviewed-by: Albert Ou <aou@eecs.berkeley.edu>
>
> Looks good to me. Are you planning to submit the RISC-V port upstream
> any time soon? If so, just keep the patch in your tree and add my
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

The RISC-V stuff is still a few months off, that's why I submitted this
upstream stand-alone.  The supervisor specification isn't 100% set in
stone yet, and we're waiting on that before upstreaming anything
significant.

> However, I did see a lot of similar bugs now that you point me to it:
>
> $  grep -r \\\<CONFIG obj-tmp/usr/include/
> obj-tmp/usr/include/asm-generic/fcntl.h:#ifndef CONFIG_64BIT
> obj-tmp/usr/include/asm-generic/mman-common.h:#ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED
> obj-tmp/usr/include/asm-generic/unistd.h:#ifdef CONFIG_MMU
> obj-tmp/usr/include/asm-generic/unistd.h:#endif /* CONFIG_MMU */
> obj-tmp/usr/include/linux/atmdev.h:#ifdef CONFIG_COMPAT
> obj-tmp/usr/include/linux/elfcore.h:#ifdef CONFIG_BINFMT_ELF_FDPIC
> obj-tmp/usr/include/linux/eventpoll.h:#ifdef CONFIG_PM_SLEEP
> obj-tmp/usr/include/linux/fb.h:#ifdef CONFIG_FB_BACKLIGHT
> obj-tmp/usr/include/linux/flat.h:#ifdef CONFIG_BINFMT_SHARED_FLAT
> obj-tmp/usr/include/linux/hw_breakpoint.h:#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
> obj-tmp/usr/include/linux/pktcdvd.h:#if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
> obj-tmp/usr/include/linux/raw.h:#define MAX_RAW_MINORS CONFIG_MAX_RAW_DEVS
> obj-tmp/usr/include/asm/ptrace.h:#ifdef CONFIG_CPU_ENDIAN_BE8
>
> These all have the same problem, and we should fix them, as well as
> (probably) adding an automated check to scripts/headers_install.sh.

Well, I was going to go fix them all and ran a very similar grep, but
I think I got a lot of false-positives.  If I understand correctly,
it's allowed to have CONFIG_* when guarded by __KERNEL__ in
user-visible headers?

Now that I've written that, I realize it'd be pretty easy to just use
cpp to drop everything inside __KERNEL__ and then look for CONFIG_*.
If you want, I can try to do that, fix what triggers the check, and
re-submit everything together?

Re: [PATCH] Remove #ifdef CONFIG_64BIT from all asm-generic/fcntl.h

[Palmer Dabbelt]

On Thu, 10 Sep 2015 04:18:05 PDT (-0700), dhowells@redhat.com wrote:
> David Howells <dhowells@redhat.com> wrote:
>
>> Rather than iterating through all the rest of your patches and saying the same
>> thing, if there's something in a UAPI header that needs wrapping in __KERNEL__
>> to exclude it from userspace's use, then it should be transferred to the
>> non-UAPI variant of that header (which should #include the UAPI variant).
>
> I should mention that there is the odd case where this is difficult to
> achieve.  See include/uapi/linux/acct.h for an example...

OK, sorry about that.  I'm submitting a v3 that should fix these
problems.

Re: [PATCH] Remove #ifdef CONFIG_64BIT from all asm-generic/fcntl.h

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Rather than iterating through all the rest of your patches and saying the same
> thing, if there's something in a UAPI header that needs wrapping in __KERNEL__
> to exclude it from userspace's use, then it should be transferred to the
> non-UAPI variant of that header (which should #include the UAPI variant).

I should mention that there is the odd case where this is difficult to
achieve.  See include/uapi/linux/acct.h for an example...

David

Re: [PATCH] Remove #ifdef CONFIG_64BIT from all asm-generic/fcntl.h

[David Howells]

Rather than iterating through all the rest of your patches and saying the same
thing, if there's something in a UAPI header that needs wrapping in __KERNEL__
to exclude it from userspace's use, then it should be transferred to the
non-UAPI variant of that header (which should #include the UAPI variant).

David

Re: [PATCH] Remove #ifdef CONFIG_64BIT from all asm-generic/fcntl.h

[Palmer Dabbelt]

I cut the RISC-V stuff, but I intend to reply to it later.  As you
said, it's just a different topic.

>>> However, I did see a lot of similar bugs now that you point me to it:
>>>
>>> $  grep -r \\\<CONFIG obj-tmp/usr/include/
>>> obj-tmp/usr/include/asm-generic/fcntl.h:#ifndef CONFIG_64BIT
>>> obj-tmp/usr/include/asm-generic/mman-common.h:#ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED
>>> obj-tmp/usr/include/asm-generic/unistd.h:#ifdef CONFIG_MMU
>>> obj-tmp/usr/include/asm-generic/unistd.h:#endif /* CONFIG_MMU */
>>> obj-tmp/usr/include/linux/atmdev.h:#ifdef CONFIG_COMPAT
>>> obj-tmp/usr/include/linux/elfcore.h:#ifdef CONFIG_BINFMT_ELF_FDPIC
>>> obj-tmp/usr/include/linux/eventpoll.h:#ifdef CONFIG_PM_SLEEP
>>> obj-tmp/usr/include/linux/fb.h:#ifdef CONFIG_FB_BACKLIGHT
>>> obj-tmp/usr/include/linux/flat.h:#ifdef CONFIG_BINFMT_SHARED_FLAT
>>> obj-tmp/usr/include/linux/hw_breakpoint.h:#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
>>> obj-tmp/usr/include/linux/pktcdvd.h:#if defined(CONFIG_CDROM_PKTCDVD_WCACHE)
>>> obj-tmp/usr/include/linux/raw.h:#define MAX_RAW_MINORS CONFIG_MAX_RAW_DEVS
>>> obj-tmp/usr/include/asm/ptrace.h:#ifdef CONFIG_CPU_ENDIAN_BE8
>>>
>>> These all have the same problem, and we should fix them, as well as
>>> (probably) adding an automated check to scripts/headers_install.sh.
>>
>> Well, I was going to go fix them all and ran a very similar grep, but
>> I think I got a lot of false-positives.  If I understand correctly,
>> it's allowed to have CONFIG_* when guarded by __KERNEL__ in
>> user-visible headers?
>
> That is right.

It turns out there was actually a header checking script
(scripts/headers_check.pl), and it already had a check for this.  The
check was just disabled because there was "too much noise".  Rather
than putting it in headers_install I've just fixed that script.  I'm
definately lacking in perl powers, so I have no idea if what I've done
is sane.  Specifically: there's a global variable and a line over 80
characters, but since there's a bunch of other violations I figure
it's fine.

>> Now that I've written that, I realize it'd be pretty easy to just use
>> cpp to drop everything inside __KERNEL__ and then look for CONFIG_*.
>
> The lines quoted above are from the output of 'make headers_install',
> which already drops everything inside of __KERNEL__. A lot of them
> probably just need to add that #ifdef, or move the portion of the
> header file to the normal (non-uabi) file.
>
>> If you want, I can try to do that, fix what triggers the check, and
>> re-submit everything together?
>
> That would be great, yes.

OK.  I think this has turned into more of a RFC than a PATCH,
though...  I've just #ifdef'd things for now to reduce the diff size,
though I think it might be cleaner to move some of them to the
non-user headers (ep_take_care_of_epollwakeup(), USE_WCACHING,
MAX_RAW_MINORS).

I'm pretty far out of my depth here, so these should all be carefully
considered, but there's a few that scare me more ("struct
elf_prstatus", "enum by_type_idx", AT_VECTOR_SIZE_ARCH).  I think
there's only one actual bug here (MAP_UNINITIALIZED), the rest just
quiet the checking script.  Each patch has my rationale for what I
did.

Since this touches a whole lot of stuff, I've added a whole bunch of
CCs.