riscv: fix VM_MAXUSER_ADDRESS checks in asm routines
ClosedPublic
Actions

Authored by mhorne on Jul 17 2021, 7:50 PM.

Details

Reviewers

markj
jhb
jrtc27

Commits

rGd925085dcdbd: riscv: fix VM_MAXUSER_ADDRESS checks in asm routines
rG8babb5582eed: riscv: fix VM_MAXUSER_ADDRESS checks in asm routines

Summary

There are two issues with the checks against VM_MAXUSER_ADDRESS. First,
the comparison should consider the values as unsigned, otherwise
addresses with the high bit set will fail to branch. Second, the value
of VM_MAXUSER_ADDRESS is, by convention, one larger than the maximum
mappable user address and invalid itself. Thus, use the bgeu instruction
for these comparisons.

PR: 257193
Reported by: Robert Morris <rtm@lcs.mit.edu>

Test Plan

With this change, the test cases in PR 257193 will terminate as expected.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mhorne created this revision.Jul 17 2021, 7:50 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 17 2021, 7:50 PM

mhorne requested review of this revision.Jul 17 2021, 7:50 PM

Harbormaster completed remote builds in B40513: Diff 92315.Jul 17 2021, 7:51 PM

mhorne edited the test plan for this revision. (Show Details)Jul 17 2021, 7:56 PM

This bug is also repeated in support.S for all the fu/su/casu functions

In D31209#702537, @jrtc27 wrote:

This bug is also repeated in support.S for all the fu/su/casu functions

Indeed, thanks for catching this. I will include those in this review.

Is it straightforward to add at least a copyin() test for this case, in tests/sys/kern_copyin.c?

With this change, the test cases in PR 253706 will terminate as expected.

Wrong PR number, in case you were planning to include this sentence in the commit log.

mhorne edited the test plan for this revision. (Show Details)Jul 20 2021, 2:28 PM

Apply the change to routines in support.S as well.

Add a test case to kern_copyin.c. It seems the hang cannot be triggered with a call to write(2), so use fcntl(2) as is done in the PR.

Harbormaster completed remote builds in B41974: Diff 96293.Oct 5 2021, 6:09 PM

mhorne retitled this revision from riscv: fix bounds checking in copyinout.S routines to riscv: fix VM_MAXUSER_ADDRESS checks in asm routines.Oct 5 2021, 6:10 PM

Add a test case to kern_copyin.c.

Thanks.

It seems the hang cannot be triggered with a call to write(2), so use fcntl(2) as is done in the PR.

Any idea why?

This revision is now accepted and ready to land.Oct 6 2021, 1:15 PM

In D31209#729707, @markj wrote:

Add a test case to kern_copyin.c.

Thanks.

It seems the hang cannot be triggered with a call to write(2), so use fcntl(2) as is done in the PR.

Any idea why?

I looked for a bit but could not pinpoint it by code inspection. The path through the write(2) syscall to where copyin() is invoked is much more involved.

I suppose we eventually make it to ffs_write(), where one of vn_io_fault_uiomove() or vn_io_fault_pgmove() is called for the relevant uio. I _think_ the latter would cause the behaviour we are seeing, but I am not sure. I would expect the former to trigger the copyin() bug.