Last write to the "error" variable in load_elf_binary() is dead write.
Add and use SUPRESS_WARN_UNUSED_RESULT macro to express intent better.
Credit goes to Ed Catmur:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425#c34
Macro doesn't work for WUR functions returning structures and unions,
but it will work once gcc copies clang.
Signed-off-by: Alexey Dobriyan <[email protected]>
---
fs/binfmt_elf.c | 3 ++-
include/linux/compiler_attributes.h | 4 ++++
2 files changed, 6 insertions(+), 1 deletion(-)
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1290,7 +1290,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
and some applications "depend" upon this behavior.
Since we do not have the power to recompile these, we
emulate the SVr4 behavior. Sigh. */
- error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
+ SUPRESS_WARN_UNUSED_RESULT
+ vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
MAP_FIXED | MAP_PRIVATE, 0);
}
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -284,6 +284,10 @@
* clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
*/
#define __must_check __attribute__((__warn_unused_result__))
+/*
+ * "(void)" is enough for clang but not for gcc.
+ */
+#define SUPRESS_WARN_UNUSED_RESULT (void)!
/*
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-weak-function-attribute
On Fri, Jun 25, 2021 at 9:52 PM Alexey Dobriyan <[email protected]> wrote:
>
> +/*
> + * "(void)" is enough for clang but not for gcc.
> + */
> +#define SUPRESS_WARN_UNUSED_RESULT (void)!
While it is related to the attribute, this macro is not an attribute,
so please add it somewhere else.
By the way, the name has a typo.
Cheers,
Miguel
On Fri, Jun 25, 2021 at 10:34:29PM +0200, Miguel Ojeda wrote:
> On Fri, Jun 25, 2021 at 9:52 PM Alexey Dobriyan <[email protected]> wrote:
> >
> > +/*
> > + * "(void)" is enough for clang but not for gcc.
> > + */
> > +#define SUPRESS_WARN_UNUSED_RESULT (void)!
>
> While it is related to the attribute, this macro is not an attribute,
> so please add it somewhere else.
This is natural place. If you're supressing WUR, then the WUR macro
itself is defined implying that the header has been included.
> By the way, the name has a typo.
Ha!
On 6/25/21 2:10 PM, Alexey Dobriyan wrote:
> On Fri, Jun 25, 2021 at 10:34:29PM +0200, Miguel Ojeda wrote:
>> On Fri, Jun 25, 2021 at 9:52 PM Alexey Dobriyan <[email protected]> wrote:
>>>
>>> +/*
>>> + * "(void)" is enough for clang but not for gcc.
>>> + */
>>> +#define SUPRESS_WARN_UNUSED_RESULT (void)!
>>
>> While it is related to the attribute, this macro is not an attribute,
>> so please add it somewhere else.
>
> This is natural place. If you're supressing WUR, then the WUR macro
> itself is defined implying that the header has been included.
>
>> By the way, the name has a typo.
>
> Ha!
>
as in Aha!?
s/SUPRESS/SUPPRESS/
Last write to the "error" variable in load_elf_binary() is dead write.
Add and use SUPPRESS_WARN_UNUSED_RESULT macro to express intent better.
Credit goes to Ed Catmur:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425#c34
Macro doesn't work for WUR functions returning structures and unions,
but it will work when gcc copies clang.
Signed-off-by: Alexey Dobriyan <[email protected]>
---
fs/binfmt_elf.c | 3 ++-
include/linux/compiler_attributes.h | 4 ++++
2 files changed, 6 insertions(+), 1 deletion(-)
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1290,7 +1290,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
and some applications "depend" upon this behavior.
Since we do not have the power to recompile these, we
emulate the SVr4 behavior. Sigh. */
- error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
+ SUPPRESS_WARN_UNUSED_RESULT
+ vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
MAP_FIXED | MAP_PRIVATE, 0);
}
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -284,6 +284,10 @@
* clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
*/
#define __must_check __attribute__((__warn_unused_result__))
+/*
+ * "(void)" is enough for clang but not for gcc.
+ */
+#define SUPPRESS_WARN_UNUSED_RESULT (void)!
/*
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-weak-function-attribute
On Fri, Jun 25, 2021 at 11:10 PM Alexey Dobriyan <[email protected]> wrote:
>
> This is natural place. If you're supressing WUR, then the WUR macro
> itself is defined implying that the header has been included.
I am not sure I understand -- I guess you are saying that your macro
is only ever needed if `__must_check` is defined, which is what I
meant by "related".
But this header was intentionally created to untangle others by
keeping only attributes here.
Cheers,
Miguel
On Sat, 26 Jun 2021 00:13:12 +0300 Alexey Dobriyan <[email protected]> wrote:
> Last write to the "error" variable in load_elf_binary() is dead write.
>
> Add and use SUPPRESS_WARN_UNUSED_RESULT macro to express intent better.
>
> Credit goes to Ed Catmur:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425#c34
>
> Macro doesn't work for WUR functions returning structures and unions,
> but it will work when gcc copies clang.
>
> ...
>
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1290,7 +1290,8 @@ static int load_elf_binary(struct linux_binprm *bprm)
> and some applications "depend" upon this behavior.
> Since we do not have the power to recompile these, we
> emulate the SVr4 behavior. Sigh. */
> - error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
> + SUPPRESS_WARN_UNUSED_RESULT
> + vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
> MAP_FIXED | MAP_PRIVATE, 0);
> }
>
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -284,6 +284,10 @@
> * clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result
> */
> #define __must_check __attribute__((__warn_unused_result__))
> +/*
> + * "(void)" is enough for clang but not for gcc.
> + */
> +#define SUPPRESS_WARN_UNUSED_RESULT (void)!
That macro is rather ugly. Hopefully we won't really need it - how
many such sites are there in a full kernel build anyway?
I can't imagine who added this to load_elf_binary():
if (current->personality & MMAP_PAGE_ZERO) {
/* Why this, you ask??? Well SVr4 maps page 0 as read-only,
and some applications "depend" upon this behavior.
Since we do not have the power to recompile these, we
emulate the SVr4 behavior. Sigh. */
error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
MAP_FIXED | MAP_PRIVATE, 0);
}
I think it was there before most of us were born. The comment has a
torvaldsy/viroey feel to it.
Do we really care about userspace which relies upon an SVR4 quirk? I
guess it's too hard to prove the no case, so it stays.
But given that the loader is being asked to map this page, shouldn't we
handle this error (fail the exec) if the mapping attempt failed? That
seems better behavior than permitting some creaky old application to
blunder into a mysterious crash?
On Fri, Jun 25, 2021 at 4:30 PM Andrew Morton <[email protected]> wrote:
>
> I can't imagine who added this to load_elf_binary():
>
> if (current->personality & MMAP_PAGE_ZERO) {
> /* Why this, you ask??? Well SVr4 maps page 0 as read-only,
> and some applications "depend" upon this behavior.
> Since we do not have the power to recompile these, we
> emulate the SVr4 behavior. Sigh. */
> error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
> MAP_FIXED | MAP_PRIVATE, 0);
> }
>
> I think it was there before most of us were born. The comment has a
> torvaldsy/viroey feel to it.
Heh.
It goes back to at least 1.1.14 (1994, I think) and originates in the
ibcs code (Intel Binary Compatibility Specification 2), back in the
dark ages when we thought that mattered.
Native Linux binaries were still a.out at that point. When ELF then
became a native thing, we just moved (or copied) the old iBCS2 code
over, and that "map zeroes at NULL" came along.
And I think it's actually Eric Youngdale who did that code. See
https://www.linuxjournal.com/article/2809
and
https://www.linuxjournal.com/article/1059
https://www.linuxjournal.com/article/1060
> Do we really care about userspace which relies upon an SVR4 quirk? I
> guess it's too hard to prove the no case, so it stays.
I think we can safely remove it. Doing a mmap() at address zero will
not actually work anyway in any half-way modern Linux environment.
And I think the "map zeroes at NULL" wasn't even universal for SVr4.
_Some_ binaries may have expected it, but I suspect it was the
exception rather than the rule.
So I'd happily take a patch that just removes it. If nothing else, it
would be trivial to put back if somebody screams, but I seriously
doubt that is going to happen.
Linus
On Fri, Jun 25, 2021 at 07:05:58PM -0700, Linus Torvalds wrote:
> > Do we really care about userspace which relies upon an SVR4 quirk? I
> > guess it's too hard to prove the no case, so it stays.
>
> I think we can safely remove it. Doing a mmap() at address zero will
> not actually work anyway in any half-way modern Linux environment.
>
> And I think the "map zeroes at NULL" wasn't even universal for SVr4.
> _Some_ binaries may have expected it, but I suspect it was the
> exception rather than the rule.
>
> So I'd happily take a patch that just removes it. If nothing else, it
> would be trivial to put back if somebody screams, but I seriously
> doubt that is going to happen.
Wasn't there some emulator (dosemu? wine?) that relied upon that?
Said that, I could be easily wrong - half-asleep right now...
On Fri, Jun 25, 2021 at 7:37 PM Al Viro <[email protected]> wrote:
>
> Wasn't there some emulator (dosemu? wine?) that relied upon that?
> Said that, I could be easily wrong - half-asleep right now...
Different issue.
dosemu used to use vm86 mode to do hardware acceleration of 16-bit
emulation. And the way vm86 mode works, it all has to be mapped
beginning at address 0.
So yes, dosemu used to do a mmap at address 0 too, but it didn't use
this ELF mmap MMAP_PAGE_ZERO personality thing to do it, it just did
its own (it also wants a lot more than one page).
Nobody uses vm86 mode any more, because hardware acceleration of
16-bit code just isn't relevant any more. Any 16-bit code you have
doesn't need special hw modes to run sufficiently fast.
Plus x86-64 doesn't support vm86 mode at all (well, technically you
can do it in a VM that runs a 32-bit OS if you really really want to,
but see above as to why you don't really need it).
There are other things that have mmap'ed things at zero. I think all
PA-RISC HP-UX binaries used to do the same thing iBCS2 did, and the
compiler would actually hoist loads to before the NULL pointer test,
so it was pretty much "architectural". Afaik, that's the same reason
why iBCS2 did that zero mapping too, but PA-RISC just required it a
lot more.
It's a horrible thing to do, and only makes debugging harder (because
you won't actually get a SIGSEGV on a NULL pointer load, you'll just
silently get a zero value and your buggy program will continue
running).
Linus
On Fri, Jun 25, 2021 at 04:30:40PM -0700, Andrew Morton wrote:
> On Sat, 26 Jun 2021 00:13:12 +0300 Alexey Dobriyan <[email protected]> wrote:
>
> > Last write to the "error" variable in load_elf_binary() is dead write.
> >
> > Add and use SUPPRESS_WARN_UNUSED_RESULT macro to express intent better.
> >
> > Credit goes to Ed Catmur:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425#c34
> >
> > Macro doesn't work for WUR functions returning structures and unions,
> > but it will work when gcc copies clang.
> >
> > ...
> >
> > #define __must_check __attribute__((__warn_unused_result__))
> > +/*
> > + * "(void)" is enough for clang but not for gcc.
> > + */
> > +#define SUPPRESS_WARN_UNUSED_RESULT (void)!
>
> That macro is rather ugly. Hopefully we won't really need it - how
> many such sites are there in a full kernel build anyway?
I don't know really. And they're hard to find because "(void)" doesn't
work and not everything is marked as WUR.
copy_from_user/copy_to_user are WUR but get_user/put_user aren't.
Logically they should bed. And if put_user() is WUR then I know another
place inside fork:
if (clone_flags & CLONE_PARENT_SETTID)
put_user(nr, args->parent_tid);
On Fri, Jun 25, 2021 at 11:57:21PM +0200, Miguel Ojeda wrote:
> On Fri, Jun 25, 2021 at 11:10 PM Alexey Dobriyan <[email protected]> wrote:
> >
> > This is natural place. If you're supressing WUR, then the WUR macro
> > itself is defined implying that the header has been included.
>
> I am not sure I understand -- I guess you are saying that your macro
> is only ever needed if `__must_check` is defined, which is what I
> meant by "related".
>
> But this header was intentionally created to untangle others by
> keeping only attributes here.
Sure. But then developer needs to include another header to use SUPRESS_WUR.
As posted, attribute and its suppressor go hand to hand.
On Sat, Jun 26, 2021 at 8:44 AM Alexey Dobriyan <[email protected]> wrote:
>
> Sure. But then developer needs to include another header to use SUPRESS_WUR.
> As posted, attribute and its suppressor go hand to hand.
These headers are automatically included in most places.
Cheers,
Miguel