Re: [PATCH 1/6] mm: pagewalk: make error checks more obvious

Re: [PATCH 1/6] mm: pagewalk: make error checks more obvious

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

Am Dienstag, 23. August 2022, 11:16:42 CEST schrieb Dan Carpenter:
> Hi Rolf,
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:   
> https://github.com/intel-lab-lkp/linux/commits/Rolf-Eike-Beer/Minor-improve
> ments-for-pagewalk-code/20220822-214107 base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
> config: x86_64-randconfig-m001
> (https://download.01.org/0day-ci/archive/20220823/202208231649.7QKavhA3-lkp
> @intel.com/config) compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> mm/pagewalk.c:248 walk_p4d_range() error: uninitialized symbol 'err'.
> mm/pagewalk.c:287 walk_pgd_range() error: uninitialized symbol 'err'.
> mm/pagewalk.c:324 walk_hugetlb_range() error: uninitialized symbol 'err'.
> 
> vim +/err +248 mm/pagewalk.c

[...]

> Same.  Although this seems more like a real bug.

They all are. I wonder how they got lost and why this never showed up during 
my tests, but still.

Andrew, do you want me to resend the series with the fixes squashed in or add a 
v7?

Eike
-- 
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 313 bytes --]

Re: [PATCH 1/6] mm: pagewalk: make error checks more obvious

[Andrew Morton]

On Mon, 22 Aug 2022 15:00:05 +0200 Rolf Eike Beer <eb@emlix.com> wrote:

> The err variable only needs to be checked when it was assigned directly
> before, it is not carried on to any later checks. Move the checks into the
> same "if" conditions where they are assigned. Also just return the error at
> the relevant places. While at it move these err variables to a more local
> scope at some places.
> 
> ...
>
> @@ -593,16 +608,15 @@ int walk_page_mapping(struct address_space *mapping, pgoff_t first_index,
>  		walk.mm = vma->vm_mm;
>  
>  		err = walk_page_test(vma->vm_start, vma->vm_end, &walk);
> -		if (err > 0) {
> -			err = 0;
> -			break;
> -		} else if (err < 0)
> -			break;
> +		if (err > 0)
> +			return 0;
> +		else if (err < 0)
> +			return err;
>  
>  		err = __walk_page_range(start_addr, end_addr, &walk);
>  		if (err)
> -			break;
> +			return err;
>  	}
>  
> -	return err;
> +	return 0;
>  }

I'm not really a fan of multiple return points - it tends to lead to
locking/resource leaks as the code evolves.  I don't really think it's
worth redoing the patch for this reason though; the rest looks good.