linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Alexey Dobriyan <adobriyan@gmail.com>
Cc: linux-kernel@vger.kernel.org, Miguel Ojeda <ojeda@kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH v2] ELF: add and use SUPPRESS_WARN_UNUSED_RESULT
Date: Fri, 25 Jun 2021 16:30:40 -0700	[thread overview]
Message-ID: <20210625163040.a15af04872959da9af161fca@linux-foundation.org> (raw)
In-Reply-To: <YNZG6N0W/7gjG7Gm@localhost.localdomain>

On Sat, 26 Jun 2021 00:13:12 +0300 Alexey Dobriyan <adobriyan@gmail.com> 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?

  reply	other threads:[~2021-06-25 23:31 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-25 19:52 [PATCH] ELF: add and use SUPRESS_WARN_UNUSED_RESULT Alexey Dobriyan
2021-06-25 20:34 ` Miguel Ojeda
2021-06-25 21:10   ` Alexey Dobriyan
2021-06-25 21:11     ` Randy Dunlap
2021-06-25 21:57     ` Miguel Ojeda
2021-06-26  6:44       ` [PATCH] ELF: add and use SUPRESS_WARN_UNUSED_RESULT\ Alexey Dobriyan
2021-06-26 14:29         ` Miguel Ojeda
2021-06-25 21:13 ` [PATCH v2] ELF: add and use SUPPRESS_WARN_UNUSED_RESULT Alexey Dobriyan
2021-06-25 23:30   ` Andrew Morton [this message]
2021-06-26  2:05     ` Linus Torvalds
2021-06-26  2:37       ` Al Viro
2021-06-26  3:13         ` Linus Torvalds
2021-06-26  6:40     ` Alexey Dobriyan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210625163040.a15af04872959da9af161fca@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=adobriyan@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ojeda@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v2] ELF: add and use SUPPRESS_WARN_UNUSED_RESULT' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).