linux-ext4 - Re: [PATCH 05/11] fsverity: kick off hash readahead at data I/O submission time

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]

Message-ID: <20260126043042.GB30803@lst.de>
Date: Mon, 26 Jan 2026 05:30:42 +0100
From: Christoph Hellwig <hch@....de>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Christoph Hellwig <hch@....de>, Al Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	David Sterba <dsterba@...e.com>, Theodore Ts'o <tytso@....edu>,
	Jaegeuk Kim <jaegeuk@...nel.org>, Chao Yu <chao@...nel.org>,
	Andrey Albershteyn <aalbersh@...hat.com>,
	"Matthew Wilcox (Oracle)" <willy@...radead.org>,
	linux-fsdevel@...r.kernel.org, linux-btrfs@...r.kernel.org,
	linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
	fsverity@...ts.linux.dev
Subject: Re: [PATCH 05/11] fsverity: kick off hash readahead at data I/O
 submission time

On Sat, Jan 24, 2026 at 12:53:29PM -0800, Eric Biggers wrote:
> > +void generic_readahead_merkle_tree(struct inode *inode, pgoff_t index,
> > +		unsigned long nr_pages)
> >  {
> >  	struct folio *folio;
> >  
> >  	folio = __filemap_get_folio(inode->i_mapping, index, FGP_ACCESSED, 0);
> > -	if (IS_ERR(folio) || !folio_test_uptodate(folio)) {
> > +	if (PTR_ERR(folio) == -ENOENT || !folio_test_uptodate(folio)) {
> 
> This dereferences an ERR_PTR() when __filemap_get_folio() returns an
> error other than -ENOENT.

Yes.  I've fixed that and split the change in error handling into a
separate well described prep patch while at it.
> I think the correct thing to do here would be the following:
> 
>         if (inode->i_sb->s_vop->readahead_merkle_tree)
> 		inode->i_sb->s_vop->readahead_merkle_tree(inode, index,
> 							  last_index - index + 1);
> 
> Then __fsverity_readahead() can be folded into fsverity_readahead().

I've done that, and also added a little comment.

> 
> > +void __fsverity_readahead(struct inode *inode, const struct fsverity_info *vi,
> > +		loff_t data_start_pos, unsigned long nr_pages)
> > +{
> > +	const struct merkle_tree_params *params = &vi->tree_params;
> > +	u64 start_hidx = data_start_pos >> params->log_blocksize;
> > +	u64 end_hidx = (data_start_pos + ((nr_pages - 1) << PAGE_SHIFT)) >>
> > +			params->log_blocksize;
> 
> (nr_pages - 1) << PAGE_SHIFT can overflow an 'unsigned long'.
> (nr_pages - 1) needs to be cast to u64 before doing the shift.
> 
> But also it would make more sense to pass
> (pgoff_t start_index, unsigned long nr_pages) instead of
> (loff_t data_start_pos, unsigned long nr_pages),
> so that the two numbers have the same units.
> 
> start_idx and end_hidx could then be computed as follows:
> 
>     u64 start_hidx = (u64)start_index << params->log_blocks_per_page;
>     u64 end_hidx = (((u64)start_index + nr_pages) << params->log_blocks_per_page) - 1;
> 
> Note that fsverity_readahead() derives the position from the index.  If
> it just used the index directly, that would be more direct.

Yes, I've updated that.  Having proper types and/or conversion helpers
for the fsverity specific addressing would be really nice as well,
but I've not touched that for now.

> > +	int level;
> > +
> > +	if (!inode->i_sb->s_vop->readahead_merkle_tree)
> > +		return;
> > +	if (unlikely(data_start_pos >= inode->i_size))
> > +		return;
> 
> The check against i_size shouldn't be necessary: the caller should just
> call this only for data it's actually going to read.

This check is based on / copied from the check in verify_data_block.
Now we only kick off readahead now and and don't actually do anything
with the read blocks, so I'll take your word that this can be removed.

> > +	for (level = 0; level < params->num_levels; level++) {
> > +		unsigned long level_start = params->level_start[level];
> > +		unsigned long next_start_hidx = start_hidx >> params->log_arity;
> > +		unsigned long next_end_hidx = end_hidx >> params->log_arity;
> > +		unsigned long start_idx = (level_start + next_start_hidx) >>
> > +				params->log_blocks_per_page;
> > +		unsigned long end_idx = (level_start + next_end_hidx) >>
> > +				params->log_blocks_per_page;
> 
> start_idx and end_idx should have type pgoff_t to make it clear that
> they're page indices.

Fixed.

> > +/**
> > + * fsverity_readahead() - kick off readahead on fsverity hashes
> > + * @folio:		first folio that is being read
> 
> folio => file data folio
> 
> Otherwise it can be confused with the Merkle tree.

I've incoroporate the various suggested documentation improvements.
Thanks!