On 08/05/2023 08:04, Kevin Brodsky wrote:
Following up on Tudor's comments, the size to require here is tricky. The purpose of this function is to bring in a (writeable) page in order to retry the atomic operation. This does not however mean that the pointer needs to be able to access the entire page. In fact, AFAICT the pointer we are manipulating here is only ever used to access a futex, i.e. a u32. So the required size should probably be that of a u32.
Maybe some elaboration is required here on why we need a check at all. In itself causing a page fault should have no functional effect in userspace, so from a capability model perspective we don't really need an explicit check. However, it is essential, for another reason: breaking the loop between futex_cmpxchg_value_locked() (or similar) when it returns -EFAULT, and fault_in_user_writeable() when it succeeds. The explicit check ensures that when the atomic operation faults, we figure out here that this was due to the pointer itself and break the loop, irrespective of the underlying page. Probably worth a comment in this function.
The required size is indeed u32, using PAGE_SIZE here was actually a mistake I didn't spot myself. So thank you for pointing it out! Good point made about explaining why we are applying a check there, will be there in the next patch.
Luca