linux-kernel - Re: [PATCH v4 7/7] iov_iter, block: Make bio structs pin pages rather than ref'ing if appropriate

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 09 Jan 2023 21:37:35 +0000
From:   David Howells <dhowells@...hat.com>
To:     Jan Kara <jack@...e.cz>
Cc:     dhowells@...hat.com, Al Viro <viro@...iv.linux.org.uk>,
        Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>,
        Matthew Wilcox <willy@...radead.org>,
        Logan Gunthorpe <logang@...tatee.com>,
        Christoph Hellwig <hch@...radead.org>,
        Jeff Layton <jlayton@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 7/7] iov_iter, block: Make bio structs pin pages rather than ref'ing if appropriate

Jan Kara <jack@...e.cz> wrote:

> So currently we already have BIO_NO_PAGE_REF flag and what you do in this
> patch partially duplicates that. So either I'd drop that flag or instead of
> bi_cleanup_mode variable (which honestly looks a bit wasteful given how we
> microoptimize struct bio) just add another BIO_ flag...

I'm fine with translating the FOLL_* flags to the BIO_* flags.  I could add a
BIO_PAGE_PINNED and translate:

	FOLL_GET => 0
	FOLL_PIN => BIO_PAGE_PINNED
	0	 => BIO_NO_PAGE_REF

It would seem that BIO_NO_PAGE_REF can't be set for BIO_PAGE_PINNED because
BIO_NO_PAGE_REF governs whether bio_release_pages() calls
__bio_release_pages() - which would be necessary.  However, bio_release_page()
can do one or the other on the basis of BIO_PAGE_PINNED being specified.  So
in my patch I would end up with:

	static void bio_release_page(struct bio *bio, struct page *page)
	{
		if (bio->bi_flags & BIO_NO_PAGE_REF)
			;
		else if (bio->bi_flags & BIO_PAGE_PINNED)
			unpin_user_page(page);
		else
			put_page(page);
	}

(This is called from four places, so it has to handle BIO_NO_PAGE_REF).

It might make sense flip the logic of BIO_NO_PAGE_REF so that we have, say:

	FOLL_GET => BIO_PAGE_REFFED
	FOLL_PIN => BIO_PAGE_PINNED
	0	 => 0

Set BIO_PAGE_REFFED by default and clear it in bio_iov_bvec_set().

Note that one reason I was thinking of saving the returned FOLL_* flags is
that I don't know if, at some point, the VM will acquire yet more different
cleanup modes - or even if a page could at some point be both ref'd *and*
pinned.

Also, I could change the interface to return something other than FOLL_* - it
just seems that they're appropriate given the underlying VM interface.

David