[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aNvwB2fr2p45hhC0@google.com>
Date: Tue, 30 Sep 2025 07:58:15 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Ackerley Tng <ackerleytng@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Christian Borntraeger <borntraeger@...ux.ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>, Claudio Imbrenda <imbrenda@...ux.ibm.com>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, David Hildenbrand <david@...hat.com>,
Fuad Tabba <tabba@...gle.com>
Subject: Re: [PATCH 6/6] KVM: selftests: Verify that faulting in private
guest_memfd memory fails
On Tue, Sep 30, 2025, Ackerley Tng wrote:
> Sean Christopherson <seanjc@...gle.com> writes:
> > How's this look?
> >
> > static void test_fault_sigbus(int fd, size_t accessible_size, size_t mmap_size)
> > {
> > struct sigaction sa_old, sa_new = {
> > .sa_handler = fault_sigbus_handler,
> > };
> > const uint8_t val = 0xaa;
> > uint8_t *mem;
> > size_t i;
> >
> > mem = kvm_mmap(mmap_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd);
> >
> > sigaction(SIGBUS, &sa_new, &sa_old);
> > if (sigsetjmp(jmpbuf, 1) == 0) {
> > memset(mem, val, mmap_size);
> > TEST_FAIL("memset() should have triggered SIGBUS");
> > }
> > if (sigsetjmp(jmpbuf, 1) == 0) {
> > (void)READ_ONCE(mem[accessible_size]);
> > TEST_FAIL("load at first unaccessible byte should have triggered SIGBUS");
> > }
> > sigaction(SIGBUS, &sa_old, NULL);
> >
> > for (i = 0; i < accessible_size; i++)
> > TEST_ASSERT_EQ(READ_ONCE(mem[i]), val);
> >
> > kvm_munmap(mem, mmap_size);
> > }
> >
> > static void test_fault_overflow(int fd, size_t total_size)
> > {
> > test_fault_sigbus(fd, total_size, total_size * 4);
> > }
> >
>
> Is it intentional that the same SIGBUS on offset mem + total_size is
> triggered twice? The memset would have worked fine until offset mem +
> total_size, which is the same SIGBUS case as mem[accessible_size]. Or
> was it meant to test that both read and write trigger SIGBUS?
The latter (test both read and write). I plan on adding this in a separate
commit, i.e. it should be obvious in the actual patches.
> > static void test_fault_private(int fd, size_t total_size)
> > {
> > test_fault_sigbus(fd, 0, total_size);
> > }
> >
>
> I would prefer more unrolling to avoid mental hoops within test code,
> perhaps like (not compile tested):
>
> static void assert_host_fault_sigbus(uint8_t *mem)
> {
> struct sigaction sa_old, sa_new = {
> .sa_handler = fault_sigbus_handler,
> };
>
> sigaction(SIGBUS, &sa_new, &sa_old);
> if (sigsetjmp(jmpbuf, 1) == 0) {
> (void)READ_ONCE(*mem);
> TEST_FAIL("Reading %p should have triggered SIGBUS", mem);
> }
> sigaction(SIGBUS, &sa_old, NULL);
> }
>
> static void test_fault_overflow(int fd, size_t total_size)
> {
> uint8_t *mem = kvm_mmap(total_size * 2, PROT_READ | PROT_WRITE, MAP_SHARED, fd);
> int i;
>
> for (i = 0; i < total_size; i++)
> TEST_ASSERT_EQ(READ_ONCE(mem[i]), val);
>
> assert_host_fault_sigbus(mem + total_size);
>
> kvm_munmap(mem, mmap_size);
> }
>
> static void test_fault_private(int fd, size_t total_size)
> {
> uint8_t *mem = kvm_mmap(total_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd);
> int i;
>
> assert_host_fault_sigbus(mem);
>
> kvm_munmap(mem, mmap_size);
> }
Why? That loses coverage for read to private memory getting SIBUGS. I genuinely
don't understand the desire to copy+paste uninteresting code.
> assert_host_fault_sigbus() can then be flexibly reused for conversion
assert_host_fault_sigbus() is a misleading name in the sense that it suggests
that the _only_ thing the helper does is assert that a SIGBUS occurred. It's
not at all obvious that there's a write to "mem" in there.
> tests (coming up) at various offsets from the mmap()-ed addresses.
>
> At some point, sigaction, sigsetjmp, etc could perhaps even be further
> wrapped. For testing memory_failure() for guest_memfd we will want to
> check for SIGBUS on memory failure injection instead of on host fault.
>
> Would be nice if it looked like this (maybe not in this patch series):
>
> + TEST_ASSERT_WILL_SIGBUS(READ_ONCE(mem[i]))
> + TEST_ASSERT_WILL_SIGBUS(WRITE_ONCE(mem[i]))
> + TEST_ASSERT_WILL_SIGBUS(madvise(MADV_HWPOISON))
Ooh, me likey. Definitely can do it now. Using a macro means we can print out
the actual action that didn't generate a SIGUBS, e.g. hacking the test to read
byte 0 generates:
'(void)READ_ONCE(mem[0])' should have triggered SIGBUS
Hmm, how about TEST_EXPECT_SIGBUS? TEST_ASSERT_xxx() typically asserts on a
value, i.e. on the result of a previous action. And s/WILL/EXPECT to make it
clear that the action is expected to SIGBUS _now_.
And if we use a descriptive global variable, we can extract the macro to e.g.
test_util.h or kvm_util.h (not sure we want to do that right away; probably best
left to the future).
static sigjmp_buf expect_sigbus_jmpbuf;
void fault_sigbus_handler(int signum)
{
siglongjmp(expect_sigbus_jmpbuf, 1);
}
#define TEST_EXPECT_SIGBUS(action) \
do { \
struct sigaction sa_old, sa_new = { \
.sa_handler = fault_sigbus_handler, \
}; \
\
sigaction(SIGBUS, &sa_new, &sa_old); \
if (sigsetjmp(expect_sigbus_jmpbuf, 1) == 0) { \
action; \
TEST_FAIL("'%s' should have triggered SIGBUS", #action); \
} \
sigaction(SIGBUS, &sa_old, NULL); \
} while (0)
static void test_fault_sigbus(int fd, size_t accessible_size, size_t map_size)
{
const char val = 0xaa;
char *mem;
size_t i;
mem = kvm_mmap(map_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd);
TEST_EXPECT_SIGBUS(memset(mem, val, map_size));
TEST_EXPECT_SIGBUS((void)READ_ONCE(mem[accessible_size]));
for (i = 0; i < accessible_size; i++)
TEST_ASSERT_EQ(READ_ONCE(mem[i]), val);
kvm_munmap(mem, map_size);
}
> >> If split up as described above, this could be
> >>
> >> if (flags & GUEST_MEMFD_FLAG_MMAP &&
> >> flags & GUEST_MEMFD_FLAG_DEFAULT_SHARED) {
> >> gmem_test(mmap_supported_fault_supported, vm, flags);
> >> gmem_test(fault_overflow, vm, flags);
> >> } else if (flags & GUEST_MEMFD_FLAG_MMAP) {
> >> gmem_test(mmap_supported_fault_sigbus, vm, flags);
> >
> > I find these unintuitive, e.g. is this one "mmap() supported, test fault sigbus",
> > or is it "mmap(), test supported fault sigbus". I also don't like that some of
> > the test names describe the _result_ (SIBGUS), where as others describe _what_
> > is being tested.
> >
>
> I think of the result (SIGBUS) as part of what's being tested. So
> test_supported_fault_sigbus() is testing that mmap is supported, and
> faulting will result in a SIGBUS.
For an utility helper, e.g. test_fault_sigbus(), or test_write_sigbus(), that's
a-ok. But it doesn't work for the top-level test functions because trying to
follow that pattern effectively prevents bundling multiple individual testcases,
e.g. test_fallocate() becomes what? And test_invalid_punch_hole_einval() is
quite obnoxious.
> > In general, I don't like test names that describe the result, because IMO what
> > is being tested is far more interesting. E.g. from a test coverage persective,
> > I don't care if attempting to fault in (CoCO) private memory gets SIGBUS versus
> > SIGSEGV, but I most definitely care that we have test coverage for the "what".
> >
>
> The SIGBUS is part of the contract with userspace and that's also part
> of what's being tested IMO.
I don't disagree, but IMO bleeding those details into the top-level functions
isn't necessary. Random developer that comes along isn't going to care whether
KVM is supposed to SIGBUS or SIGSEGV unless there is a failure. And as above,
doing so either singles out sigbus or necessitates truly funky names.
Powered by blists - more mailing lists