[v1] fs: binfmt_elf_efpic: fix variable set but not used warning

[PATCH] fs: binfmt_elf_efpic: fix variable set but not used warning

Posted by sunliming@linux.dev 11 months, 1 week ago

From: sunliming <sunliming@kylinos.cn>

Fix below kernel warning:
fs/binfmt_elf_fdpic.c:1024:52: warning: variable 'excess1' set but not
used [-Wunused-but-set-variable]

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: sunliming <sunliming@kylinos.cn>
---
 fs/binfmt_elf_fdpic.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index e3cf2801cd64..bed13ee8bfec 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1024,8 +1024,11 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
 	/* deal with each load segment separately */
 	phdr = params->phdrs;
 	for (loop = 0; loop < params->hdr.e_phnum; loop++, phdr++) {
-		unsigned long maddr, disp, excess, excess1;
+		unsigned long maddr, disp, excess;
 		int prot = 0, flags;
+#ifdef CONFIG_MMU
+		unsigned long excess1;
+#endif
 
 		if (phdr->p_type != PT_LOAD)
 			continue;
@@ -1120,9 +1123,9 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
 		 *   extant in the file
 		 */
 		excess = phdr->p_memsz - phdr->p_filesz;
-		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
 
 #ifdef CONFIG_MMU
+		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
 		if (excess > excess1) {
 			unsigned long xaddr = maddr + phdr->p_filesz + excess1;
 			unsigned long xmaddr;
-- 
2.25.1

Re: [PATCH] fs: binfmt_elf_efpic: fix variable set but not used warning

Posted by Jan Kara 11 months, 1 week ago

On Fri 07-03-25 14:11:28, sunliming@linux.dev wrote:
> From: sunliming <sunliming@kylinos.cn>
> 
> Fix below kernel warning:
> fs/binfmt_elf_fdpic.c:1024:52: warning: variable 'excess1' set but not
> used [-Wunused-but-set-variable]
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: sunliming <sunliming@kylinos.cn>

The extra ifdef is not pretty but I guess it's better. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/binfmt_elf_fdpic.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index e3cf2801cd64..bed13ee8bfec 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -1024,8 +1024,11 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
>  	/* deal with each load segment separately */
>  	phdr = params->phdrs;
>  	for (loop = 0; loop < params->hdr.e_phnum; loop++, phdr++) {
> -		unsigned long maddr, disp, excess, excess1;
> +		unsigned long maddr, disp, excess;
>  		int prot = 0, flags;
> +#ifdef CONFIG_MMU
> +		unsigned long excess1;
> +#endif
>  
>  		if (phdr->p_type != PT_LOAD)
>  			continue;
> @@ -1120,9 +1123,9 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
>  		 *   extant in the file
>  		 */
>  		excess = phdr->p_memsz - phdr->p_filesz;
> -		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
>  
>  #ifdef CONFIG_MMU
> +		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
>  		if (excess > excess1) {
>  			unsigned long xaddr = maddr + phdr->p_filesz + excess1;
>  			unsigned long xmaddr;
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: binfmt_elf_efpic: fix variable set but not used warning

Posted by Kees Cook 11 months ago

On Fri, Mar 07, 2025 at 03:47:28PM +0100, Jan Kara wrote:
> On Fri 07-03-25 14:11:28, sunliming@linux.dev wrote:
> > From: sunliming <sunliming@kylinos.cn>
> > 
> > Fix below kernel warning:
> > fs/binfmt_elf_fdpic.c:1024:52: warning: variable 'excess1' set but not
> > used [-Wunused-but-set-variable]
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: sunliming <sunliming@kylinos.cn>
> 
> The extra ifdef is not pretty but I guess it's better. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Since we allow loop-scope variable definitions now, perhaps this is a
case for defining the variable later within the #ifdef, like this?


diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index e3cf2801cd64..b0ef71238328 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1024,7 +1024,7 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
 	/* deal with each load segment separately */
 	phdr = params->phdrs;
 	for (loop = 0; loop < params->hdr.e_phnum; loop++, phdr++) {
-		unsigned long maddr, disp, excess, excess1;
+		unsigned long maddr, disp, excess;
 		int prot = 0, flags;
 
 		if (phdr->p_type != PT_LOAD)
@@ -1120,9 +1120,10 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
 		 *   extant in the file
 		 */
 		excess = phdr->p_memsz - phdr->p_filesz;
-		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
 
 #ifdef CONFIG_MMU
+		const unsigned long excess1 =
+			PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
 		if (excess > excess1) {
 			unsigned long xaddr = maddr + phdr->p_filesz + excess1;
 			unsigned long xmaddr;

> 
> 								Honza
> 
> > ---
> >  fs/binfmt_elf_fdpic.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> > index e3cf2801cd64..bed13ee8bfec 100644
> > --- a/fs/binfmt_elf_fdpic.c
> > +++ b/fs/binfmt_elf_fdpic.c
> > @@ -1024,8 +1024,11 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
> >  	/* deal with each load segment separately */
> >  	phdr = params->phdrs;
> >  	for (loop = 0; loop < params->hdr.e_phnum; loop++, phdr++) {
> > -		unsigned long maddr, disp, excess, excess1;
> > +		unsigned long maddr, disp, excess;
> >  		int prot = 0, flags;
> > +#ifdef CONFIG_MMU
> > +		unsigned long excess1;
> > +#endif
> >  
> >  		if (phdr->p_type != PT_LOAD)
> >  			continue;
> > @@ -1120,9 +1123,9 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
> >  		 *   extant in the file
> >  		 */
> >  		excess = phdr->p_memsz - phdr->p_filesz;
> > -		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
> >  
> >  #ifdef CONFIG_MMU
> > +		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
> >  		if (excess > excess1) {
> >  			unsigned long xaddr = maddr + phdr->p_filesz + excess1;
> >  			unsigned long xmaddr;
> > -- 
> > 2.25.1
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

-- 
Kees Cook

Re: [PATCH] fs: binfmt_elf_efpic: fix variable set but not used warning

Posted by Sunliming 11 months ago

On 2025/3/8 04:30, Kees Cook wrote:
> On Fri, Mar 07, 2025 at 03:47:28PM +0100, Jan Kara wrote:
>> On Fri 07-03-25 14:11:28, sunliming@linux.dev wrote:
>>> From: sunliming <sunliming@kylinos.cn>
>>>
>>> Fix below kernel warning:
>>> fs/binfmt_elf_fdpic.c:1024:52: warning: variable 'excess1' set but not
>>> used [-Wunused-but-set-variable]
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: sunliming <sunliming@kylinos.cn>
>> The extra ifdef is not pretty but I guess it's better. Feel free to add:
>>
>> Reviewed-by: Jan Kara <jack@suse.cz>
> Since we allow loop-scope variable definitions now, perhaps this is a
> case for defining the variable later within the #ifdef, like this?
>
>
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index e3cf2801cd64..b0ef71238328 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -1024,7 +1024,7 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
>   	/* deal with each load segment separately */
>   	phdr = params->phdrs;
>   	for (loop = 0; loop < params->hdr.e_phnum; loop++, phdr++) {
> -		unsigned long maddr, disp, excess, excess1;
> +		unsigned long maddr, disp, excess;
>   		int prot = 0, flags;
>   
>   		if (phdr->p_type != PT_LOAD)
> @@ -1120,9 +1120,10 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
>   		 *   extant in the file
>   		 */
>   		excess = phdr->p_memsz - phdr->p_filesz;
> -		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
>   
>   #ifdef CONFIG_MMU
> +		const unsigned long excess1 =
> +			PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
>   		if (excess > excess1) {
>   			unsigned long xaddr = maddr + phdr->p_filesz + excess1;
>   			unsigned long xmaddr;
I think this is a good idea. I will resend this patch in this way，thanks.
>> 								Honza
>>
>>> ---
>>>   fs/binfmt_elf_fdpic.c | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
>>> index e3cf2801cd64..bed13ee8bfec 100644
>>> --- a/fs/binfmt_elf_fdpic.c
>>> +++ b/fs/binfmt_elf_fdpic.c
>>> @@ -1024,8 +1024,11 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
>>>   	/* deal with each load segment separately */
>>>   	phdr = params->phdrs;
>>>   	for (loop = 0; loop < params->hdr.e_phnum; loop++, phdr++) {
>>> -		unsigned long maddr, disp, excess, excess1;
>>> +		unsigned long maddr, disp, excess;
>>>   		int prot = 0, flags;
>>> +#ifdef CONFIG_MMU
>>> +		unsigned long excess1;
>>> +#endif
>>>   
>>>   		if (phdr->p_type != PT_LOAD)
>>>   			continue;
>>> @@ -1120,9 +1123,9 @@ static int elf_fdpic_map_file_by_direct_mmap(struct elf_fdpic_params *params,
>>>   		 *   extant in the file
>>>   		 */
>>>   		excess = phdr->p_memsz - phdr->p_filesz;
>>> -		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
>>>   
>>>   #ifdef CONFIG_MMU
>>> +		excess1 = PAGE_SIZE - ((maddr + phdr->p_filesz) & ~PAGE_MASK);
>>>   		if (excess > excess1) {
>>>   			unsigned long xaddr = maddr + phdr->p_filesz + excess1;
>>>   			unsigned long xmaddr;
>>> -- 
>>> 2.25.1
>>>
>> -- 
>> Jan Kara <jack@suse.com>
>> SUSE Labs, CR