Re: [PATCH v2] ELF: add and use SUPPRESS_WARN_UNUSED_RESULT

[Andrew Morton]

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