On Wed, Mar 12, 2025 at 10:30:06AM +0800, Herbert Xu wrote: > On Tue, Mar 11, 2025 at 10:44:31AM -0700, Eric Biggers wrote: > > > > I guess you think this is fixing a bug in the case where sg->offset > PAGE_SIZE? > > Is that case even supported? It is supposed to be the offset into a page. > > Supported? Obviously not since this bug has been there since the > very start :) > > But is it conceivable to create such a scatterlist, it certainly > seems to be. If we were to create a scatterlist from a single > order-n folio, then it's quite reasonable for the offset to be > greater than PAGE_SIZE. If it needs to work, then it needs to be tested. Currently the self-tests always use sg_set_buf(), and thus scatterlist::offset always gets set to a value less than PAGE_SIZE. And this is yet more evidence that scatterlist based APIs are just a really bad idea. Even 20 years later, there is still no precise definition of what a scatterlist even is... > > Even if so, a simpler fix (1 line) would be to use: > > 'sg->length >= nbytes && sg->offset + nbytes <= PAGE_SIZE' > > That would just mean more use of the fallback code path. Not > a big deal but I was feeling generous. My second suggestion, making it conditional on !HIGHMEM and using nbytes <= sg->length, would also be simple and would reduce the overhead for !HIGHMEM (rather than increasing it as your patch does, just like patch 1 which had the same problem). !HIGHMEM is the common case (by far) these days, of course. - Eric
