| Message ID | 20220423100751.1870771-1-catalin.marinas@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Avoid live-lock in btrfs fault-in+uaccess loop | expand |
On Sat, Apr 23, 2022 at 3:07 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > The series introduces fault_in_subpage_writeable() together with the > arm64 probing counterpart and the btrfs fix. Looks fine to me - and I think it can probably go through the arm64 tree since you'd be the only one really testing it anyway. I assume you checked that btrfs is the only one that uses fault_in_writeable() in this way? Everybody else updates to the right byte boundary and retries (or returns immediately)? Linus
On Sat, Apr 23, 2022 at 09:35:42AM -0700, Linus Torvalds wrote: > On Sat, Apr 23, 2022 at 3:07 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > The series introduces fault_in_subpage_writeable() together with the > > arm64 probing counterpart and the btrfs fix. > > Looks fine to me - and I think it can probably go through the arm64 > tree since you'd be the only one really testing it anyway. I'll queue it via arm64 then. > I assume you checked that btrfs is the only one that uses > fault_in_writeable() in this way? Everybody else updates to the right > byte boundary and retries (or returns immediately)? I couldn't find any other places (by inspection or testing). The buffered file I/O can already make progress in current fault_in_*() + copy_*_user() loops. O_DIRECT either goes via GUP (and memcpy() doesn't fault) or, if the user buffer is not PAGE aligned, it may fall back to buffered I/O. That's why I simplified the series, AFAICT it's only btrfs search_ioctl() with this problem.
Hi Catalin, On Sat, Apr 23, 2022 at 8:40 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > On Sat, Apr 23, 2022 at 09:35:42AM -0700, Linus Torvalds wrote: > > On Sat, Apr 23, 2022 at 3:07 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > > The series introduces fault_in_subpage_writeable() together with the > > > arm64 probing counterpart and the btrfs fix. > > > > Looks fine to me - and I think it can probably go through the arm64 > > tree since you'd be the only one really testing it anyway. > > I'll queue it via arm64 then. sounds good to me, thank you. > > I assume you checked that btrfs is the only one that uses > > fault_in_writeable() in this way? Everybody else updates to the right > > byte boundary and retries (or returns immediately)? > > I couldn't find any other places (by inspection or testing). The > buffered file I/O can already make progress in current fault_in_*() + > copy_*_user() loops. This started working correctly with commit bc1bb416bbb9 ("generic_perform_write()/iomap_write_actor(): saner logics for short copy") by Al from last May. > O_DIRECT either goes via GUP (and memcpy() doesn't > fault) or, if the user buffer is not PAGE aligned, it may fall back to > buffered I/O. That's why I simplified the series, AFAICT it's only btrfs > search_ioctl() with this problem. Thanks, Andreas
On Sat, 23 Apr 2022 11:07:48 +0100, Catalin Marinas wrote: > A minor update from v3 here: > > https://lore.kernel.org/r/20220406180922.1522433-1-catalin.marinas@arm.com > > In patch 3/3 I dropped the 'len' local variable, so the btrfs patch > simply replaces fault_in_writeable() with fault_in_subpage_writeable() > and adds a comment. I kept David's ack as there's no functional change > since v3. > > [...] Applied to arm64 (for-next/fault-in-subpage). Also changed the probe_subpage_writeable() prototype to use char __user * instead of void __user * (as per Andrew's suggestion). [1/3] mm: Add fault_in_subpage_writeable() to probe at sub-page granularity https://git.kernel.org/arm64/c/da32b5817253 [2/3] arm64: Add support for user sub-page fault probing https://git.kernel.org/arm64/c/f3ba50a7a100 [3/3] btrfs: Avoid live-lock in search_ioctl() on hardware with sub-page faults https://git.kernel.org/arm64/c/18788e34642e
Hi, A minor update from v3 here: https://lore.kernel.org/r/20220406180922.1522433-1-catalin.marinas@arm.com In patch 3/3 I dropped the 'len' local variable, so the btrfs patch simply replaces fault_in_writeable() with fault_in_subpage_writeable() and adds a comment. I kept David's ack as there's no functional change since v3. Andrew, since there was no objection last time around, I'd like this series to land in 5.19. As it touches arch, fs and mm, it should probably go in via the mm tree but I'm also happy to merge the series via arm64. Please let me know if you have any preference. The btrfs search_ioctl() function can potentially live-lock on arm64 with MTE enabled due to a fault_in_writeable() + copy_to_user_nofault() unbounded loop. The uaccess can fault in the middle of a page (MTE tag check fault) even if a prior fault_in_writeable() successfully wrote to the beginning of that page. The btrfs loop always restarts the fault-in loop from the beginning of the user buffer, hence the live-lock. The series introduces fault_in_subpage_writeable() together with the arm64 probing counterpart and the btrfs fix. Thanks. Catalin Marinas (3): mm: Add fault_in_subpage_writeable() to probe at sub-page granularity arm64: Add support for user sub-page fault probing btrfs: Avoid live-lock in search_ioctl() on hardware with sub-page faults arch/Kconfig | 7 +++++++ arch/arm64/Kconfig | 1 + arch/arm64/include/asm/mte.h | 1 + arch/arm64/include/asm/uaccess.h | 15 +++++++++++++++ arch/arm64/kernel/mte.c | 30 ++++++++++++++++++++++++++++++ fs/btrfs/ioctl.c | 7 ++++++- include/linux/pagemap.h | 1 + include/linux/uaccess.h | 22 ++++++++++++++++++++++ mm/gup.c | 29 +++++++++++++++++++++++++++++ 9 files changed, 112 insertions(+), 1 deletion(-) base-commit: b2d229d4ddb17db541098b83524d901257e93845