linux-kernel - Re: [PATCH 1/2] iov_iter: iterate_folioq: fix handling of offset >= folio size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aJpipiVk0zneTxXl@codewreck.org>
Date: Tue, 12 Aug 2025 06:37:42 +0900
From: asmadeus@...ewreck.org
To: David Howells <dhowells@...hat.com>
Cc: "Matthew Wilcox (Oracle)" <willy@...radead.org>,
	Christian Brauner <brauner@...nel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Maximilian Bosch <maximilian@...sch.me>,
	Ryan Lahfa <ryan@...fa.xyz>, Christian Theune <ct@...ingcircus.io>,
	Arnout Engelen <arnout@...t.net>, linux-kernel@...r.kernel.org,
	linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH 1/2] iov_iter: iterate_folioq: fix handling of offset >=
 folio size

David Howells wrote on Mon, Aug 11, 2025 at 03:37:43PM +0100:
> Dominique Martinet via B4 Relay wrote:
> > It's apparently possible to get an iov forwarded all the way up to the
> 
> By "forwarded" I presume you mean "advanced"?

Thanks, swapped words in v2

> > This should have been because we're only in the 2nd slot and there's
> > another one after this, but iterate_folioq should not try to map a
> > folio that skips the whole size, and more importantly part here does
> > not end up zero (because 'PAGE_SIZE - skip % PAGE_SIZE' ends up
> > PAGE_SIZE and not zero..), so skip forward to the "advance to next
> > folio" code.
> 
> Note that things get complicated because folioqs form a segmented list that
> can be under construction as it advances.  So if there's no next folioq
> segment at the time you advance to the end of the current one, it will end up
> parked at the end of the last folio or with slot==nr_slots because there's
> nowhere for it to advance to.

Hmm, I've already sent a v2 with other things fixed but now you made me
look at the "we're at the end of the iov_iter" case I think this won't
work well either?
folioq_folio() always returns something, and the advance code only
advances if folioq->next is set and doesn't bail out if it's unset.

There should be a `if (slot == folioq_nr_slots(folioq)) break` check
somewhere as well? Or is the iov_iter guaranteed to always 1/ have some
data and 2/ either be big enough or have remaining data in a step?

I can believe the former but wouldn't trust the later...

> Note that extract_folioq_to_sg() already does this as does
> iov_iter_extract_folioq_pages().

Yes we're not quite consistent here, some functions like the plain
iov_iter_advance will get you on an invalid slot to check for
folioq->next on next invocations while others point at the end of the
last folio in the queue (like iov_iter_extract_folioq_pages(), and
iov_folioq_get_pages() before patch 2);
I think either pattern is valid; I've changed iov_folioq_get_pages()
because it was a bit weird to have an iov_iter with offset > count and
iov_iter_advance wouldn't do this, but I agree either should work, we
just probably want to be more consistent.

Thanks,
-- 
Dominique Martinet | Asmadeus