linux-kernel - Re: [PATCH] x86, efi: Fix a build warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130425065529.GD7806@gmail.com>
Date:	Thu, 25 Apr 2013 08:55:29 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Matt Fleming <matt.fleming@...el.com>
Cc:	Borislav Petkov <bp@...en8.de>, X86 ML <x86@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Borislav Petkov <bp@...e.de>,
	Matthew Garrett <matthew.garrett@...ula.com>
Subject: Re: [PATCH] x86, efi: Fix a build warning


* Matt Fleming <matt.fleming@...el.com> wrote:

> On 24/04/13 11:56, Ingo Molnar wrote:
> > 
> > * Borislav Petkov <bp@...en8.de> wrote:
> > 
> >> From: Borislav Petkov <bp@...e.de>
> >>
> >> Fix this:
> >>
> >> arch/x86/boot/compressed/eboot.c: In function ???setup_efi_vars???:
> >> arch/x86/boot/compressed/eboot.c:269:2: warning: passing argument 1 of ???efi_call_phys??? makes pointer from integer without a cast [enabled by default]
> >> In file included from arch/x86/boot/compressed/eboot.c:12:0:
> >> /w/kernel/linux/arch/x86/include/asm/efi.h:8:33: note: expected ???void *??? but argument is of type ???long unsigned int???
> >>
> >> after cc5a080c5d40 ("efi: Pass boot services variable info to runtime
> >> code").
> >>
> >> Cc: Matthew Garrett <matthew.garrett@...ula.com>
> >> Cc: Matt Fleming <matt.fleming@...el.com>
> >> Signed-off-by: Borislav Petkov <bp@...e.de>
> >> ---
> >>  arch/x86/boot/compressed/eboot.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> >> index 8615f7581820..41de115a55b7 100644
> >> --- a/arch/x86/boot/compressed/eboot.c
> >> +++ b/arch/x86/boot/compressed/eboot.c
> >> @@ -266,7 +266,7 @@ static efi_status_t setup_efi_vars(struct boot_params *params)
> >>  	while (data && data->next)
> >>  		data = (struct setup_data *)(unsigned long)data->next;
> >>  
> >> -	status = efi_call_phys4(sys_table->runtime->query_variable_info,
> >> +	status = efi_call_phys4((void *)sys_table->runtime->query_variable_info,
> >>  				EFI_VARIABLE_NON_VOLATILE |
> >>  				EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>  				EFI_VARIABLE_RUNTIME_ACCESS, &store_size,
> > 
> > Why isn't the query_variale_info field changed to void * instead? That way 
> > no cast would be needed.
> 
> We could either change all fields in efi_system_table or the
> efi_call_phys* prototypes. x86-64 already casts to (void *) when calling
> efi_call0(), etc, though I'm not entirely sure why void * is needed.

It's a basic cleanliness and robustness issue: we generally avoid type 
casts in the kernel, because type casts override compile-time type checks 
and are easy to get wrong. They are also ugly.

So in generaly we try to use the right type for the data structure, which 
matches its usage (and standardize functions/methods around that type) - 
then no cast is needed.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/