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

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Mar 16, 2025 at 12:28:24PM +0800, Herbert Xu wrote:
> On Sat, Mar 15, 2025 at 08:31:41PM -0700, Eric Biggers wrote:
> >
> > > @@ -189,14 +195,18 @@ static inline void scatterwalk_done_dst(struct scatter_walk *walk,
> > >  	 * reliably optimized out or not.
> > >  	 */
> > >  	if (ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE) {
> > > -		struct page *base_page, *start_page, *end_page, *page;
> > > +		struct page *base_page;
> > > +		unsigned int offset;
> > > +		int start, end, i;
> > >  
> > >  		base_page = sg_page(walk->sg);
> > > -		start_page = base_page + (walk->offset >> PAGE_SHIFT);
> > > -		end_page = base_page + ((walk->offset + nbytes +
> > > -					 PAGE_SIZE - 1) >> PAGE_SHIFT);
> > > -		for (page = start_page; page < end_page; page++)
> > > -			flush_dcache_page(page);
> > > +		offset = walk->offset;
> > > +		start = offset >> PAGE_SHIFT;
> > > +		end = start + (nbytes >> PAGE_SHIFT);
> > > +		end += (offset_in_page(offset) + offset_in_page(nbytes) +
> > > +			PAGE_SIZE - 1) >> PAGE_SHIFT;
> > 
> > The change to how the end page index is calculated is unrelated to the nth_page
> > fix, and it makes the code slower and harder to understand.  My original code
> > just rounded the new offset up to a page boundary to get the end page index.
> 
> The original code is open to overflows in the addition.  The new
> version is not.

If you think you are fixing a separate issue, you need to say so.

But I also don't think it's worth worrying about lengths so close to UINT_MAX.
No one is using them, they have no testing, and there are likely to be other
overflows like this one too.

- Eric