From: Andrey Albershteyn <aalbersh@redhat.com>
To: fsverity@lists.linux.dev, ebiggers@kernel.org
Subject: fs-verity spinlock in is_hash_block_verified()
Date: Thu, 25 Jan 2024 00:02:07 +0100 [thread overview]
Message-ID: <vry5tz5aaqaewaaei4e6yux7knb27zf4sm774hqqyisgfj6go7@7x2htyodqfjc> (raw)
Hi Eric,
I'm continuing my attempts in adding fs-verity support into XFS :)
And I have a question
I'm trying to not break stuff for ext4 and keep all the
optimizations in place but I got a little bit confused about the
spinlock in is_hash_block_verified().
If we drop opportunistic lockless read for simplicity [1], we get
something like this:
static bool is_hash_block_verified(struct fsverity_info *vi, struct page *hpage,
unsigned long hblock_idx)
{
bool verified;
unsigned int blocks_per_page;
unsigned int i;
if (!vi->hash_block_verified)
return PageChecked(hpage);
spin_lock(&vi->hash_page_init_lock);
if (PageChecked(hpage)) {
verified = test_bit(hblock_idx, vi->hash_block_verified);
} else {
blocks_per_page = vi->tree_params.blocks_per_page;
hblock_idx = round_down(hblock_idx, blocks_per_page);
for (i = 0; i < blocks_per_page; i++)
clear_bit(hblock_idx + i, vi->hash_block_verified);
SetPageChecked(hpage);
verified = false;
}
spin_unlock(&vi->hash_page_init_lock);
return verified;
}
As you wrote in the comment [2] the spinlock is needed to set bitmap
and PG_checked together. But why do we need spinlock? Isn't memory
barrier between 'for loop' and SetPageChecked() enough?
...
for (i = 0; i < blocks_per_page; i++)
clear_bit(hblock_idx + i, vi->hash_block_verified);
smp_wmb();
SetPageChecked(hpage);
...
Even if two threads get into PG_checked == 0 case they will clear
bits and set page flag. The only requirement, I can see, is order
that page flag is set after bitmap bits, so, no other threads will
skip blocks which were evicted from memory but still have bit set to
1. As we clear bits we won't get false positive for not verified
block.
Other threads, which already passed verification and want to set bit
to 1 won't be affected by any of this; as they do verification they
have a reference to page, therefore, PG_checked was already set for
them.
Maybe I'm missing a case where things can go bad? Can you describe
in more details why you used spinklock here?
Thanks,
Andrey
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/tree/fs/verity/verify.c#n19
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/tree/fs/verity/verify.c#n39
next reply other threads:[~2024-01-24 23:02 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-24 23:02 Andrey Albershteyn [this message]
2024-01-25 2:08 ` fs-verity spinlock in is_hash_block_verified() Eric Biggers
2024-01-25 10:42 ` Andrey Albershteyn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vry5tz5aaqaewaaei4e6yux7knb27zf4sm774hqqyisgfj6go7@7x2htyodqfjc \
--to=aalbersh@redhat.com \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).