Re: [v2 PATCH 1/2] crypto: scatterwalk - Use nth_page instead of doing it by hand

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Mar 12, 2025 at 12:29:57PM +0800, Herbert Xu wrote:
> Curiously, the Crypto API scatterwalk incremented pages by hand
> rather than using nth_page.  Possibly because scatterwalk predates
> nth_page (the following commit is from the history tree):

Well, also because it's never been clearly defined what a scatterlist even is.

> 	commit 3957f2b34960d85b63e814262a8be7d5ad91444d
> 	Author: James Morris <jmorris@intercode.com.au>
> 	Date:   Sun Feb 2 07:35:32 2003 -0800
> 
> 	    [CRYPTO]: in/out scatterlist support for ciphers.
> 
> Fix this by using nth_page.

This needs an explanation of what this is actually fixing.

I think it is: on a system with SPARSEMEM && !SPARSEMEM_VMEMMAP, if the caller
provided a scatterlist element that spanned physical memory sections that are
physically contiguous (but have separate vmemmaps), then the struct page(s)
calculated for any non-first sections would be invalid.  But then that invalid
struct page just gets passed to kmap_local_page(), so it would only matter if
also HIGHMEM || WANT_PAGE_VIRTUAL which are the cases where the struct page
actually gets used, I think?

All the edge cases like this need to be fixed of course, though fortunately it
sounds like this one was super rare.

> diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
> index b7e617ae4442..0ba5be363abf 100644
> --- a/include/crypto/scatterwalk.h
> +++ b/include/crypto/scatterwalk.h
> @@ -100,11 +100,15 @@ static inline void scatterwalk_get_sglist(struct scatter_walk *walk,
>  static inline void scatterwalk_map(struct scatter_walk *walk)
>  {
>  	struct page *base_page = sg_page(walk->sg);
> +	unsigned int offset = walk->offset;
> +	void *addr;
>  
>  	if (IS_ENABLED(CONFIG_HIGHMEM)) {
> -		walk->__addr = kmap_local_page(base_page +
> -					       (walk->offset >> PAGE_SHIFT)) +
> -			       offset_in_page(walk->offset);
> +		struct page *page;
> +
> +		page = nth_page(base_page, offset >> PAGE_SHIFT);
> +		offset = offset_in_page(offset);
> +		addr = kmap_local_page(page) + offset;

It would be cleaner as:

		struct page *page;

		page = nth_page(base_page, offset >> PAGE_SHIFT);
		walk->__addr = kmap_local_page(page) + offset_in_page(offset);

>  	} else {
>  		/*
>  		 * When !HIGHMEM we allow the walker to return segments that
> @@ -112,13 +116,14 @@ static inline void scatterwalk_map(struct scatter_walk *walk)
>  		 * clear that in this case we're working in the linear buffer of
>  		 * the whole sg entry in the kernel's direct map rather than
>  		 * within the mapped buffer of a single page, compute the
> -		 * address as an offset from the page_address() of the first
> -		 * page of the sg entry.  Either way the result is the address
> -		 * in the direct map, but this makes it clearer what is really
> -		 * going on.
> +		 * address as an offset from the lowmem_page_address() of
> +		 * the first page of the sg entry.  Either way the result
> +		 * is the address in the direct map, but this makes it clearer
> +		 * what is really going on.
>  		 */
> -		walk->__addr = page_address(base_page) + walk->offset;
> +		addr = lowmem_page_address(base_page) + offset;
>  	}
> +	walk->__addr = addr;

This change is unrelated and seems incorrect.  lowmem_page_address() is a mm
implementation detail.  page_address() is the right one to use.

- Eric