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 --]

[PATCH 0/6] Minor improvements for pagewalk code

[Rolf Eike Beer]

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

For some project I had to use the pagewalk API for certain things and during 
this have read through the code multiple times. Our usage has changed several 
times depending on our current state of research as well.

During all of this I have made some tweaks to the code to be able to follow it 
better when hunting my own problems, and not call into some things that I 
actually don't need. The patches are more or less independent of each other. 
Especially the last one may heavily depend on personal taste, so if you don't 
like it, just ignore it.

I would welcome if you could just pick those that you think are fitting and 
provide feedback on either of the remaining ones. At the end none of them 
should make any functional difference.

Regards,

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.