convert read_kcore(), vread() to use iterators
Subsystems (9)
v8 cover convert read_kcore(), vread() to use iterators
While reviewing Baoquan's recent changes to permit vread() access to vm_map_ram regions of vmalloc allocations, Willy pointed out [1] that it would be nice to refactor vread() as a whole, since its only user is read_kcore() and the existing form of vread() necessitates the use of a bounce buffer.
This patch series does exactly that, as well as adjusting how we read the kernel text section to avoid the use of a bounce buffer in this case as well.
This has been tested against the test case which motivated Baoquan's changes in the first place [2] which continues to function correctly, as do the vmalloc self tests.
[1] https://lore.kernel.org/all/Y8WfDSRkc%[email protected]/
[2] https://lore.kernel.org/all/[email protected]/T/#u
v8:
- Make zero_iter() static.
v7:
- Keep trying to fault in memory until the vmalloc read operation completes.
https://lore.kernel.org/all/[email protected]/
v6:
- Correct copy_page_to_iter_nofault() to handle -EFAULT case correctly.
https://lore.kernel.org/all/[email protected]/
v5:
- Do not rename fpos to ppos in read_kcore_iter() to avoid churn.
- Fix incorrect commit messages after prior revisions altered the approach.
- Replace copy_page_to_iter_atomic() with copy_page_to_iter_nofault() and adjust it to be able to handle compound pages. This uses copy_to_user_nofault() which ensures page faults are disabled during copy which kmap_local_page() was not doing.
- Only try to fault in pages if we are unable to copy in the first place and try only once to avoid any risk of spinning.
- Do not zero memory in aligned_vread_iter() if we couldn't copy it.
- Fix mistake in zeroing missing or unpopulated blocks in vmap_ram_vread_iter().
https://lore.kernel.org/linux-mm/[email protected]/
v4:
- Fixup mistake in email client which orphaned patch emails from the cover letter.
https://lore.kernel.org/all/[email protected]
v3:
- Revert introduction of mutex/rwsem in vmalloc
- Introduce copy_page_to_iter_atomic() iovec function
- Update vread_iter() and descendent functions to use only this
- Fault in user pages before calling vread_iter()
- Use const char* in vread_iter() and descendent functions
- Updated commit messages based on feedback
- Extend vread functions to always check how many bytes we could copy. If at any stage we are unable to copy/zero, abort and return the number of bytes we did copy.
https://lore.kernel.org/all/[email protected]/
v2:
- Fix ordering of vread_iter() parameters
- Fix nommu vread() -> vread_iter()
https://lore.kernel.org/all/[email protected]/
v1:
https://lore.kernel.org/all/[email protected]/
Lorenzo Stoakes (4):
fs/proc/kcore: avoid bounce buffer for ktext data fs/proc/kcore: convert read_kcore() to read_kcore_iter()
iov_iter: add copy_page_to_iter_nofault()
mm: vmalloc: convert vread() to vread_iter()
fs/proc/kcore.c | 85 +++++++-------- include/linux/uio.h | 2 + include/linux/vmalloc.h | 3 +- lib/iov_iter.c | 48 +++++++++ mm/nommu.c | 10 +- mm/vmalloc.c | 234 +++++++++++++++++++++++++--------------- 6 files changed, 243 insertions(+), 139 deletions(-)
--
2.39.2
PATCH v8 1/4 fs/proc/kcore: avoid bounce buffer for ktext data
Commit df04abfd181a ("fs/proc/kcore.c: Add bounce buffer for ktext data") introduced the use of a bounce buffer to retrieve kernel text data for /proc/kcore in order to avoid failures arising from hardened user copies enabled by CONFIG_HARDENED_USERCOPY in check_kernel_text_object().
We can avoid doing this if instead of copy_to_user() we use _copy_to_user() which bypasses the hardening check. This is more efficient than using a bounce buffer and simplifies the code.
We do so as part an overall effort to eliminate bounce buffer usage in the function with an eye to converting it an iterator read.
--- fs/proc/kcore.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
fs/proc/kcore.c +5 -12
@@ -541,19 +541,12 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)case KCORE_VMEMMAP :case KCORE_TEXT :/* -* Using bounce buffer to bypass the -* hardened user copy kernel text checks. +* We use _copy_to_user() to bypass usermode hardening +* which would otherwise prevent this operation. */ -if (copy_from_kernel_nofault (buf, (void *)start, tsz)) { -if (clear_user (buffer, tsz)) { - ret = -EFAULT ; - goto out; - } - }else { -if (copy_to_user (buffer, buf, tsz)) { - ret = -EFAULT ; - goto out; - } +if (_copy_to_user (buffer, (char *)start, tsz)) { + ret = -EFAULT ; + goto out; }break ;default :
Signature · 1 line
2.39.2
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 9 lines · Lorenzo Stoakes
Commit
df04abfd181a("fs/proc/kcore.c: Add bounce buffer for ktext data") introduced the use of a bounce buffer to retrieve kernel text data for/proc/kcorein order to avoid failures arising from hardened user copies enabled byCONFIG_HARDENED_USERCOPYincheck_kernel_text_object().We can avoid doing this if instead of
copy_to_user()we use_copy_to_user()which bypasses the hardening check. This is more efficient than using a bounce buffer and simplifies the code.
Quoted 5 lines
We do so as part an overall effort to eliminate bounce buffer usage in the function with an eye to converting it an iterator read.
Signed-off-by Lorenzo Stoakes <[email protected]>Reviewed-by David Hildenbrand <[email protected]>
hi,
sorry for late feedback, but looks like this one breaks reading /proc/kcore with objdump for me:
# cat /proc/kallsyms | grep ksys_read ffffffff8150ebc0 T ksys_read # objdump -d --start-address=0xffffffff8150ebc0 --stop-address=0xffffffff8150ebd0 /proc/kcore
/proc/kcore: file format elf64-x86-64
objdump: Reading section load1 failed because: Bad address
reverting this makes it work again
thanks,
jirka
Quoted 36 lines
--- fs/proc/kcore.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)fs/proc/kcore.c +5
-12@@ -541,19 +541,12 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)case KCORE_VMEMMAP :case KCORE_TEXT :/* -* Using bounce buffer to bypass the -* hardened user copy kernel text checks. +* We use _copy_to_user() to bypass usermode hardening +* which would otherwise prevent this operation. */ -if (copy_from_kernel_nofault (buf, (void *)start, tsz)) { -if (clear_user (buffer, tsz)) { - ret = -EFAULT ; - goto out; - } - }else { -if (copy_to_user (buffer, buf, tsz)) { - ret = -EFAULT ; - goto out; - } +if (_copy_to_user (buffer, (char *)start, tsz)) { + ret = -EFAULT ; + goto out; }break ;default :Signature · 1 line
2.39.2
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Hi Jiri,
Quoted 24 lines · Jiri Olsa
Quoted 9 lines · Lorenzo Stoakes
Commit
df04abfd181a("fs/proc/kcore.c: Add bounce buffer for ktext data") introduced the use of a bounce buffer to retrieve kernel text data for/proc/kcorein order to avoid failures arising from hardened user copies enabled byCONFIG_HARDENED_USERCOPYincheck_kernel_text_object().We can avoid doing this if instead of
copy_to_user()we use_copy_to_user()which bypasses the hardening check. This is more efficient than using a bounce buffer and simplifies the code.Quoted 5 lines
We do so as part an overall effort to eliminate bounce buffer usage in the function with an eye to converting it an iterator read.
Signed-off-by Lorenzo Stoakes <[email protected]>Reviewed-by David Hildenbrand <[email protected]>hi,
sorry for late feedback, but looks like this one breaks reading/proc/kcorewith objdump for me:# cat /proc/kallsyms | grep ksys_read ffffffff8150ebc0 T ksys_read # objdump -d --start-address=0xffffffff8150ebc0 --stop-address=0xffffffff8150ebd0 /proc/kcore
Quoted 5 lines
/proc/kcore: file format elf64-x86-64objdump: Reading section load1 failed because: Bad address
reverting this makes it work again
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcore
Later I tried your above objdump testing, it corrupted system too.
Is there any conclusion about this issue you reported? I could miss things in the discussion or patch posting to fix this.
Thanks
Baoquan
Quoted 39 lines
Quoted 38 lines
--- fs/proc/kcore.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)fs/proc/kcore.c +5
-12@@ -541,19 +541,12 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)case KCORE_VMEMMAP :case KCORE_TEXT :/* -* Using bounce buffer to bypass the -* hardened user copy kernel text checks. +* We use _copy_to_user() to bypass usermode hardening +* which would otherwise prevent this operation. */ -if (copy_from_kernel_nofault (buf, (void *)start, tsz)) { -if (clear_user (buffer, tsz)) { - ret = -EFAULT ; - goto out; - } - }else { -if (copy_to_user (buffer, buf, tsz)) { - ret = -EFAULT ; - goto out; - } +if (_copy_to_user (buffer, (char *)start, tsz)) { + ret = -EFAULT ; + goto out; }break ;default :Signature · 1 line
2.39.2
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 36 lines · Baoquan He
Hi Jiri,
Quoted 24 lines · Jiri Olsa
Quoted 9 lines · Lorenzo Stoakes
Commit
df04abfd181a("fs/proc/kcore.c: Add bounce buffer for ktext data") introduced the use of a bounce buffer to retrieve kernel text data for/proc/kcorein order to avoid failures arising from hardened user copies enabled byCONFIG_HARDENED_USERCOPYincheck_kernel_text_object().We can avoid doing this if instead of
copy_to_user()we use_copy_to_user()which bypasses the hardening check. This is more efficient than using a bounce buffer and simplifies the code.Quoted 5 lines
We do so as part an overall effort to eliminate bounce buffer usage in the function with an eye to converting it an iterator read.
Signed-off-by Lorenzo Stoakes <[email protected]>Reviewed-by David Hildenbrand <[email protected]>hi,
sorry for late feedback, but looks like this one breaks reading/proc/kcorewith objdump for me:# cat /proc/kallsyms | grep ksys_read ffffffff8150ebc0 T ksys_read # objdump -d --start-address=0xffffffff8150ebc0 --stop-address=0xffffffff8150ebd0 /proc/kcoreQuoted 5 lines
/proc/kcore: file format elf64-x86-64objdump: Reading section load1 failed because: Bad address
reverting this makes it work again
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
Quoted 6 lines
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
Is there any conclusion about this issue you reported? I could miss things in the discussion or patch posting to fix this.
hi,
thanks for your reply, I meant to ping on this again
AFAIK there was no answer yet.. I managed to cleanly revert the patch when I needed the functionality, then got sidetracked and forgot about this
I just re-tested and it's still failing for me, would be great to get it fixed
Lorenzo, any idea?
thanks,
jirka
Quoted 44 lines
Thanks
BaoquanQuoted 39 lines
Quoted 38 lines
--- fs/proc/kcore.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)fs/proc/kcore.c +5
-12@@ -541,19 +541,12 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)case KCORE_VMEMMAP :case KCORE_TEXT :/* -* Using bounce buffer to bypass the -* hardened user copy kernel text checks. +* We use _copy_to_user() to bypass usermode hardening +* which would otherwise prevent this operation. */ -if (copy_from_kernel_nofault (buf, (void *)start, tsz)) { -if (clear_user (buffer, tsz)) { - ret = -EFAULT ; - goto out; - } - }else { -if (copy_to_user (buffer, buf, tsz)) { - ret = -EFAULT ; - goto out; - } +if (_copy_to_user (buffer, (char *)start, tsz)) { + ret = -EFAULT ; + goto out; }break ;default :Signature · 1 line
2.39.2
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
In reply to Baoquan HeHi,
Quoted 8 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
Signature · 3 lines
Cheers, David / dhildenb
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 6 lines · David Hildenbrand
Hi,
Quoted 4 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
Quoted 7 lines
Quoted 4 lines
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
From my testing, reading kcore will cause system panic, then reboot. Not sure if Jiri saw the same phenomenon.
Quoted 3 lines
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
When tested on a arm64 system, the reproducution is stable. I will have a look too to see if I have some finding this week.
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 9 lines · Baoquan He
Quoted 8 lines · David Hildenbrand
Hi,
Quoted 6 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcore
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Later I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
From my testing, reading kcore will cause system panic, then reboot. Not sure if Jiri saw the same phenomenon.
it did not crash for me, just the read error
could you get console output from that?
jirka
Quoted 8 lines
Quoted 4 lines
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
When tested on a arm64 system, the reproducution is stable. I will have a look too to see if I have some finding this week.
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 13 lines · Jiri Olsa
Quoted 12 lines · Baoquan He
Quoted 11 lines · David Hildenbrand
Hi,
Quoted 8 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
From my testing, reading kcore will cause system panic, then reboot. Not sure if Jiri saw the same phenomenon.
it did not crash for me, just the read error
could you get console output from that?
I got a new arm64 machine, then executing "makedumpfile --mem-usage /proc/kcore" won't trigger panic, your objdump command can trigger panic. The call trace is pasted at below. It's the same as the panic and call trace I met on my last arm64 machine.
[13270.314323] Mem abort info: [13270.317162] ESR = 0x0000000096000007 [13270.320901] EC = 0x25: DABT (current EL), IL = 32 bits [13270.326217] SET = 0, FnV = 0 [13270.329261] EA = 0, S1PTW = 0 [13270.332390] FSC = 0x07: level 3 translation fault [13270.337270] Data abort info: [13270.340139] ISV = 0, ISS = 0x00000007, ISS2 = 0x00000000 [13270.345626] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [13270.350666] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [13270.355981] swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000400651d64000 [13270.362672] [ffffdc9cf3ea0000] pgd=1000401ffffff003, p4d=1000401ffffff003, pud=1000401fffffe003, pmd=1000401fffffd003, pte=0000000000000000 [13270.375367] Internal error: Oops: 0000000096000007 [#4] SMP [13270.380934] Modules linked in: mlx5_ib ib_uverbs ib_core rfkill vfat fat joydev cdc_ether usbnet mii mlx5_core acpi_ipmi mlxfw ipmi_ssif psample tls ipmi_devintf pci_hyperv_intf arm_spe_pmu ipmi_msghandler arm_cmn arm_dmc620_pmu arm_dsu_pmu cppc_cpufreq acpi_tad fuse zram xfs crct10dif_ce polyval_ce polyval_generic ghash_ce uas sbsa_gwdt nvme nvme_core ast usb_storage nvme_common i2c_algo_bit xgene_hwmon [13270.416751] CPU: 15 PID: 8803 Comm: objdump Tainted: G D 6.5.0-rc3 #1 [13270.424570] Hardware name: WIWYNN Mt.Jade Server System B81.030Z1.0007/Mt.Jade Motherboard, BIOS 2.10.20220531 (SCP: 2.10.20220531) 2022/05/31 [13270.437337] pstate: 20400009 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [13270.444289] pc : __arch_copy_to_user+0x180/0x240 [13270.448910] lr : _copy_to_iter+0x11c/0x5d0 [13270.453002] sp : ffff8000b15a37c0 [13270.456306] x29: ffff8000b15a37c0 x28: ffffdc9cf3ea0000 x27: ffffdc9cf6938158 [13270.463431] x26: ffff8000b15a3ba8 x25: 0000000000000690 x24: ffff8000b15a3b80 [13270.470556] x23: 00000000000038ac x22: ffffdc9cf3ea0000 x21: ffff8000b15a3b80 [13270.477682] x20: ffffdc9cf64fdf00 x19: 0000000000000400 x18: 0000000000000000 [13270.484806] x17: 0000000000000000 x16: 0000000000000000 x15: ffffdc9cf3ea0000 [13270.491931] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 [13270.499056] x11: 0001000000000000 x10: ffffdc9cf64fdf00 x9 : 0000000000000690 [13270.506182] x8 : 000000007c000000 x7 : 0000fd007e000000 x6 : 000000000eee0b60 [13270.513306] x5 : 000000000eee0f60 x4 : 0000000000000000 x3 : 0000000000000400 [13270.520431] x2 : 0000000000000380 x1 : ffffdc9cf3ea0000 x0 : 000000000eee0b60 [13270.527556] Call trace: [13270.529992] __arch_copy_to_user+0x180/0x240 [13270.534250] read_kcore_iter+0x718/0x878 [13270.538167] proc_reg_read_iter+0x8c/0xe8 [13270.542168] vfs_read+0x214/0x2c0 [13270.545478] ksys_read+0x78/0x118 [13270.548782] __arm64_sys_read+0x24/0x38 [13270.552608] invoke_syscall+0x78/0x108 [13270.556351] el0_svc_common.constprop.0+0x4c/0xf8 [13270.561044] do_el0_svc+0x34/0x50 [13270.564347] el0_svc+0x34/0x108 [13270.567482] el0t_64_sync_handler+0x100/0x130 [13270.571829] el0t_64_sync+0x194/0x198 [13270.575483] Code: d503201f d503201f d503201f d503201f (a8c12027) [13270.581567] ---[ end trace 0000000000000000 ]---
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
In reply to David HildenbrandQuoted 14 lines · David Hildenbrand
Hi,
Quoted 8 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
Quoted 8 lines
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
--
Cheers,David / dhildenb
Apologies I mised this, I have been very busy lately not least with book :)
Concerning, I will take a look as I get a chance. I think the whole series would have to be reverted which would be... depressing... as other patches in series eliminates the bounce buffer altogether.
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 21 lines · Lorenzo Stoakes
Quoted 20 lines · David Hildenbrand
Hi,
Quoted 8 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
--
Cheers,
Quoted 9 lines
Quoted 2 lines
David / dhildenb
Apologies I mised this, I have been very busy lately not least with book :)
Concerning, I will take a look as I get a chance. I think the whole series would have to be reverted which would be... depressing... as other patches in series eliminates the bounce buffer altogether.
I spotted
https://lkml.kernel.org/r/[email protected]
today, maybe that's related.
Signature · 3 lines
Cheers, David / dhildenb
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 27 lines · David Hildenbrand
Quoted 26 lines · Lorenzo Stoakes
Quoted 14 lines · David Hildenbrand
Hi,
Quoted 8 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
Quoted 8 lines
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
--
Cheers,David / dhildenb
Apologies I mised this, I have been very busy lately not least with book :)
Quoted 9 lines
Quoted 4 lines
Concerning, I will take a look as I get a chance. I think the whole series would have to be reverted which would be... depressing... as other patches in series eliminates the bounce buffer altogether.
I spotted
Find that slightly confusing, they talk about just reveritng the patch but then also add a kern_addr_valid()?
I'm also confused about people talking about just reverting the patch, as 4c91c07c93bb drops the bounce buffer altogether... presumably they mean reverting both?
Clearly this is an arm64 thing (obviously), I have some arm64 hardware let me see if I can repro...
Baoquan, Jiri - are you reverting more than just the one commit? And does doing this go from not working -> working? Or from not working (worst case oops) -> error?
Quoted 7 lines
today, maybe that's related.
--
Cheers,David / dhildenb
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 41 lines · Lorenzo Stoakes
Quoted 27 lines · David Hildenbrand
Quoted 26 lines · Lorenzo Stoakes
Quoted 14 lines · David Hildenbrand
Hi,
Quoted 8 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
Quoted 8 lines
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
--
Cheers,David / dhildenb
Apologies I mised this, I have been very busy lately not least with book :)
Quoted 9 lines
Quoted 4 lines
Concerning, I will take a look as I get a chance. I think the whole series would have to be reverted which would be... depressing... as other patches in series eliminates the bounce buffer altogether.
I spotted
Find that slightly confusing, they talk about just reveritng the patch but then also add a
kern_addr_valid()?
Quoted 6 lines
I'm also confused about people talking about just reverting the patch, as
4c91c07c93bbdrops the bounce buffer altogether... presumably they mean reverting both?Clearly this is an arm64 thing (obviously), I have some arm64 hardware let me see if I can repro...
I see the issue on x86
Quoted 4 lines
Baoquan, Jiri - are you reverting more than just the one commit? And does doing this go from not working -> working? Or from not working (worst case oops) -> error?
yes, I used to revert all 4 patches
I did quick check and had to revert 2 more patches to get clean revert
38b138abc355 Revert "fs/proc/kcore: avoid bounce buffer for ktext data" e2c3b418d365 Revert "fs/proc/kcore: convert read_kcore() to read_kcore_iter()" d8bc432cb314 Revert "iov_iter: add copy_page_to_iter_nofault()" bf2c6799f68c Revert "iov_iter: Kill ITER_PIPE"ccf4b2c5c5ce Revert "mm: vmalloc: convert vread() to vread_iter()" de400d383a7e Revert "mm/vmalloc: replace the ternary conditional operator with min()"
jirka
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 42 lines · Jiri Olsa
Quoted 41 lines · Lorenzo Stoakes
Quoted 27 lines · David Hildenbrand
Quoted 26 lines · Lorenzo Stoakes
Quoted 14 lines · David Hildenbrand
Hi,
Quoted 8 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
Quoted 8 lines
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
--
Cheers,David / dhildenb
Apologies I mised this, I have been very busy lately not least with book :)
Quoted 9 lines
Quoted 4 lines
Concerning, I will take a look as I get a chance. I think the whole series would have to be reverted which would be... depressing... as other patches in series eliminates the bounce buffer altogether.
I spotted
Find that slightly confusing, they talk about just reveritng the patch but then also add a
kern_addr_valid()?
Quoted 8 lines
Quoted 6 lines
I'm also confused about people talking about just reverting the patch, as
4c91c07c93bbdrops the bounce buffer altogether... presumably they mean reverting both?Clearly this is an arm64 thing (obviously), I have some arm64 hardware let me see if I can repro...
I see the issue on x86
Ummmm what? I can't! What repro are you seeing on x86, exactly?
Quoted 10 lines
Quoted 5 lines
Baoquan, Jiri - are you reverting more than just the one commit? And does doing this go from not working -> working? Or from not working (worst case oops) -> error?
yes, I used to revert all 4 patches
I did quick check and had to revert 2 more patches to get clean revert
Quoted 8 lines
38b138abc355Revert "fs/proc/kcore: avoid bounce buffer for ktext data"e2c3b418d365Revert "fs/proc/kcore: convertread_kcore()toread_kcore_iter()"d8bc432cb314Revert "iov_iter: addcopy_page_to_iter_nofault()"bf2c6799f68cRevert "iov_iter: KillITER_PIPE"ccf4b2c5c5ceRevert "mm: vmalloc: convertvread()tovread_iter()"de400d383a7eRevert "mm/vmalloc: replace the ternary conditional operator withmin()"jirka
That's quite a few more reverts and obviously not an acceptable solution here.
Looking at
https://lore.kernel.org/all/CAA5enKaUYehLZGL3abv4rsS7caoUG-pN9wF3R+qek-DGNZufbA@mail.gmail.com
a parallel thread on this, it looks like the issue is that we are no longer using a no-fault kernel copy in KCORE_TEXT path and arm64 doesn't map everything in the text range.
Solution would be to reinstate the bounce buffer in this case (ugh). Longer term solution I think would be to create some iterator helper that does no fault copies from the kernel.
I will try to come up with a semi-revert that keeps the iterator stuff but keeps a hideous bounce buffer for the KCORE_TEXT bit with a comment explaining why...
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 47 lines · Lorenzo Stoakes
Quoted 46 lines · Jiri Olsa
Quoted 45 lines · Lorenzo Stoakes
Quoted 27 lines · David Hildenbrand
Quoted 26 lines · Lorenzo Stoakes
Quoted 14 lines · David Hildenbrand
Hi,
Quoted 8 lines
I met this too when I executed below command to trigger a kcore reading. I wanted to do a simple testing during system running and got this.
makedumpfile --mem-usage /proc/kcoreLater I tried your above objdump testing, it corrupted system too.
What do you mean with "corrupted system too" -- did it not only fail to dump the system, but also actually harmed the system?
Quoted 8 lines
@Lorenzo do you plan on reproduce + fix, or should we consider reverting that change?
--
Cheers,David / dhildenb
Apologies I mised this, I have been very busy lately not least with book :)
Quoted 9 lines
Quoted 4 lines
Concerning, I will take a look as I get a chance. I think the whole series would have to be reverted which would be... depressing... as other patches in series eliminates the bounce buffer altogether.
I spotted
Find that slightly confusing, they talk about just reveritng the patch but then also add a
kern_addr_valid()?I'm also confused about people talking about just reverting the patch, as
4c91c07c93bbdrops the bounce buffer altogether... presumably they mean reverting both?
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Clearly this is an arm64 thing (obviously), I have some arm64 hardware let me see if I can repro...
I see the issue on x86
Ummmm what? I can't! What repro are you seeing on x86, exactly?
# cat /proc/kallsyms | grep ksys_read ffffffff8151e450 T ksys_read
# objdump -d --start-address=0xffffffff8151e450 --stop-address=0xffffffff8151e460 /proc/kcore
/proc/kcore: file format elf64-x86-64
objdump: Reading section load1 failed because: Bad address
jirka
Quoted 35 lines
Quoted 11 lines
Quoted 6 lines
Baoquan, Jiri - are you reverting more than just the one commit? And does doing this go from not working -> working? Or from not working (worst case oops) -> error?
yes, I used to revert all 4 patches
I did quick check and had to revert 2 more patches to get clean revert
Quoted 8 lines
38b138abc355Revert "fs/proc/kcore: avoid bounce buffer for ktext data"e2c3b418d365Revert "fs/proc/kcore: convertread_kcore()toread_kcore_iter()"d8bc432cb314Revert "iov_iter: addcopy_page_to_iter_nofault()"bf2c6799f68cRevert "iov_iter: KillITER_PIPE"ccf4b2c5c5ceRevert "mm: vmalloc: convertvread()tovread_iter()"de400d383a7eRevert "mm/vmalloc: replace the ternary conditional operator withmin()"jirka
That's quite a few more reverts and obviously not an acceptable solution here.
Looking at
https://lore.kernel.org/all/CAA5enKaUYehLZGL3abv4rsS7caoUG-pN9wF3R+qek-DGNZufbA@mail.gmail.com
a parallel thread on this, it looks like the issue is that we are no longer using a no-fault kernel copy inKCORE_TEXTpath and arm64 doesn't map everything in the text range.Solution would be to reinstate the bounce buffer in this case (ugh). Longer term solution I think would be to create some iterator helper that does no fault copies from the kernel.
I will try to come up with a semi-revert that keeps the iterator stuff but keeps a hideous bounce buffer for the
KCORE_TEXTbit with a comment explaining why...
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
Quoted 5 lines · Jiri Olsa
Quoted 1 line
Ummmm what? I can't! What repro are you seeing on x86, exactly?
# cat /proc/kallsyms | grep ksys_read ffffffff8151e450 T ksys_read
Quoted 8 lines
# objdump -d --start-address=0xffffffff8151e450 --stop-address=0xffffffff8151e460 /proc/kcore
/proc/kcore: file format elf64-x86-64objdump: Reading section load1 failed because: Bad address
jirka
Locally I don't see this issue. How odd. The bug doesn't manifest as a 'bad address' in the arm64 repros either. I wonder if this is something unrelated...
In any case I have a candidate fix for the bug at
https://lore.kernel.org/all/[email protected]/ which should hopefully address the underlying issue with minimum change.
Re: [PATCH v8 1/4] fs/proc/kcore: avoid bounce buffer for ktext data
In reply to Jiri Olsa[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]
[TLDR: I'm adding this report to the list of tracked Linux kernel regressions; the text you find below is based on a few templates paragraphs you might have encountered already in similar form. See link in footer if these mails annoy you.]
Quoted 16 lines · Jiri Olsa
Quoted 7 lines · Lorenzo Stoakes
Commit
df04abfd181a("fs/proc/kcore.c: Add bounce buffer for ktext data") introduced the use of a bounce buffer to retrieve kernel text data for/proc/kcorein order to avoid failures arising from hardened user copies enabled byCONFIG_HARDENED_USERCOPYincheck_kernel_text_object().[...]Signed-off-by Lorenzo Stoakes <[email protected]>Reviewed-by David Hildenbrand <[email protected]>sorry for late feedback, but looks like this one breaks reading
/proc/kcorewith objdump for me:# cat /proc/kallsyms | grep ksys_read ffffffff8150ebc0 T ksys_read # objdump -d --start-address=0xffffffff8150ebc0 --stop-address=0xffffffff8150ebd0 /proc/kcore
Quoted 5 lines
/proc/kcore: file format elf64-x86-64objdump: Reading section load1 failed because: Bad address
reverting this makes it work again
Thanks for the report. To be sure the issue doesn't fall through the cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression tracking bot:
#regzbot ^introduced 2e1c017077
#regzbot title mm / fs/proc/kcore: reading /proc/kcore with objdump broke #regzbot ignore-activity
This isn't a regression? This issue or a fix for it are already discussed somewhere else? It was fixed already? You want to clarify when the regression started to happen? Or point out I got the title or something else totally wrong? Then just reply and tell me -- ideally while also telling regzbot about it, as explained by the page listed in the footer of this mail.
Developers: When fixing the issue, remember to add 'Link:' tags pointing to the report (the parent of this mail). See page linked in footer for details.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) --
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.
PATCH v8 2/4 fs/proc/kcore: convert read_kcore() to read_kcore_iter()
In reply to Lorenzo StoakesFor the time being we still use a bounce buffer for vread(), however in the next patch we will convert this to interact directly with the iterator and eliminate the bounce buffer altogether.
--- fs/proc/kcore.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)
fs/proc/kcore.c +18 -18
@@ -24,7 +24,7 @@#include <linux/memblock.h> #include <linux/init.h> #include <linux/slab.h> -#include <linux/uaccess.h> +#include <linux/uio.h> #include <asm/io.h> #include <linux/list.h> #include <linux/ioport.h>
@@ -308,9 +308,12 @@ static void append_kcore_note(char *notes, size_t *i, const char *name, }static ssize_t -read_kcore (struct file *file,char __user *buffer,size_t buflen,loff_t *fpos) +read_kcore_iter (struct kiocb *iocb,struct iov_iter *iter) { +struct file *file = iocb->ki_filp;char *buf = file->private_data; +loff_t *fpos = &iocb->ki_pos; +size_t phdrs_offset, notes_offset, data_offset;size_t page_offline_frozen =1 ;size_t phdrs_len, notes_len;
@@ -318,6 +321,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)size_t tsz;int nphdr;unsigned long start; +size_t buflen =iov_iter_count (iter);size_t orig_buflen = buflen;int ret =0 ;
@@ -356,12 +360,11 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) }; tsz =min_t (size_t, buflen,sizeof (struct elfhdr ) - *fpos); -if (copy_to_user (buffer, (char *)&ehdr + *fpos, tsz)) { +if (copy_to_iter ((char *)&ehdr + *fpos, tsz, iter) != tsz) { ret = -EFAULT ; goto out; } - buffer += tsz; buflen -= tsz; *fpos += tsz; }
@@ -398,15 +401,14 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) } tsz =min_t (size_t, buflen, phdrs_offset + phdrs_len - *fpos); -if (copy_to_user (buffer, (char *)phdrs + *fpos - phdrs_offset, - tsz)) { +if (copy_to_iter ((char *)phdrs + *fpos - phdrs_offset, tsz, + iter) != tsz) {kfree (phdrs); ret = -EFAULT ; goto out; }kfree (phdrs); - buffer += tsz; buflen -= tsz; *fpos += tsz; }
@@ -448,14 +450,13 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) min(vmcoreinfo_size, notes_len - i)); tsz =min_t (size_t, buflen, notes_offset + notes_len - *fpos); -if (copy_to_user (buffer, notes + *fpos - notes_offset, tsz)) { +if (copy_to_iter (notes + *fpos - notes_offset, tsz, iter) != tsz) {kfree (notes); ret = -EFAULT ; goto out; }kfree (notes); - buffer += tsz; buflen -= tsz; *fpos += tsz; }
@@ -497,7 +498,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) }if (!m) { -if (clear_user (buffer, tsz)) { +if (iov_iter_zero (tsz, iter) != tsz) { ret = -EFAULT ; goto out; }
@@ -508,14 +509,14 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)case KCORE_VMALLOC :vread (buf, (char *)start, tsz);/* we have to zero-fill user buffer even if no read */ -if (copy_to_user (buffer, buf, tsz)) { +if (copy_to_iter (buf, tsz, iter) != tsz) { ret = -EFAULT ; goto out; }break ;case KCORE_USER :/* User page is handled prior to normal kernel page: */ -if (copy_to_user (buffer, (char *)start, tsz)) { +if (copy_to_iter ((char *)start, tsz, iter) != tsz) { ret = -EFAULT ; goto out; }
@@ -531,7 +532,7 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)*/ if (!page ||PageOffline (page) ||is_page_hwpoison (page) || !pfn_is_ram (pfn)) { -if (clear_user (buffer, tsz)) { +if (iov_iter_zero (tsz, iter) != tsz) { ret = -EFAULT ; goto out; }
@@ -541,17 +542,17 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos)case KCORE_VMEMMAP :case KCORE_TEXT :/* -* We use _copy_to_user() to bypass usermode hardening +* We use _copy_to_iter() to bypass usermode hardening * which would otherwise prevent this operation. */ -if (_copy_to_user (buffer, (char *)start, tsz)) { +if (_copy_to_iter ((char *)start, tsz, iter) != tsz) { ret = -EFAULT ; goto out; }break ;default :pr_warn_once ("Unhandled KCORE type: %d\n" , m->type); -if (clear_user (buffer, tsz)) { +if (iov_iter_zero (tsz, iter) != tsz) { ret = -EFAULT ; goto out; }
@@ -559,7 +560,6 @@ read_kcore(struct file *file, char __user *buffer, size_t buflen, loff_t *fpos) skip: buflen -= tsz; *fpos += tsz; - buffer += tsz; start += tsz; tsz = (buflen >PAGE_SIZE ?PAGE_SIZE : buflen); }
@@ -603,7 +603,7 @@ static int release_kcore(struct inode *inode, struct file *file) }static const struct proc_ops kcore_proc_ops = { - .proc_read = read_kcore, + .proc_read_iter = read_kcore_iter, .proc_open = open_kcore, .proc_release = release_kcore, .proc_lseek = default_llseek,
Signature · 1 line
2.39.2
PATCH v8 3/4 iov_iter: add copy_page_to_iter_nofault()
In reply to Lorenzo StoakesProvide a means to copy a page to user space from an iterator, aborting if a page fault would occur. This supports compound pages, but may be passed a tail page with an offset extending further into the compound page, so we cannot pass a folio.
This allows for this function to be called from atomic context and _try_ to user pages if they are faulted in, aborting if not.
The function does not use _copy_to_iter() in order to not specify might_fault(), this is similar to copy_page_from_iter_atomic().
This is being added in order that an iteratable form of vread() can be implemented while holding spinlocks.
--- include/linux/uio.h | 2 ++ lib/iov_iter.c | 48 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)
include/linux/uio.h +2 -0
@@ -173,6 +173,8 @@ static inline size_t copy_folio_to_iter(struct folio *folio, size_t offset, {return copy_page_to_iter (&folio->page, offset, bytes, i); } +size_t copy_page_to_iter_nofault (struct page *page,unsigned offset, +size_t bytes,struct iov_iter *i);static __always_inline __must_check size_t copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i)
lib/iov_iter.c +48 -0
60 lines not on this page · raw message
Signature · 1 line
2.39.2
PATCH v8 4/4 mm: vmalloc: convert vread() to vread_iter()
In reply to Lorenzo StoakesHaving previously laid the foundation for converting vread() to an iterator function, pull the trigger and do so.
This patch attempts to provide minimal refactoring and to reflect the existing logic as best we can, for example we continue to zero portions of memory not read, as before.
Overall, there should be no functional difference other than a performance improvement in /proc/kcore access to vmalloc regions.
Now we have eliminated the need for a bounce buffer in read_kcore_iter(), we dispense with it, and try to write to user memory optimistically but with faults disabled via copy_page_to_iter_nofault(). We already have preemption disabled by holding a spin lock. We continue faulting in until the operation is complete.
Additionally, we must account for the fact that at any point a copy may fail (most likely due to a fault not being able to occur), we exit indicating fewer bytes retrieved than expected.
--- fs/proc/kcore.c | 44 ++++---- include/linux/vmalloc.h | 3 +- mm/nommu.c | 10 +- mm/vmalloc.c | 234 +++++++++++++++++++++++++--------------- 4 files changed, 176 insertions(+), 115 deletions(-)
fs/proc/kcore.c +23 -21
@@ -307,13 +307,9 @@ static void append_kcore_note(char *notes, size_t *i, const char *name, *i =ALIGN (*i + descsz,4 ); } -static ssize_t -read_kcore_iter (struct kiocb *iocb,struct iov_iter *iter) +static ssize_t read_kcore_iter (struct kiocb *iocb,struct iov_iter *iter) { -struct file *file = iocb->ki_filp; -char *buf = file->private_data;loff_t *fpos = &iocb->ki_pos; -size_t phdrs_offset, notes_offset, data_offset;size_t page_offline_frozen =1 ;size_t phdrs_len, notes_len;
@@ -507,13 +503,30 @@ read_kcore_iter(struct kiocb *iocb, struct iov_iter *iter)switch (m->type) {case KCORE_VMALLOC : -vread (buf, (char *)start, tsz); -/* we have to zero-fill user buffer even if no read */ -if (copy_to_iter (buf, tsz, iter) != tsz) { - ret = -EFAULT ; - goto out; + { +const char *src = (char *)start; +size_t read =0 , left = tsz; + +/* +* vmalloc uses spinlocks, so we optimistically try to +* read memory. If this fails, fault pages in and try +* again until we are done. +*/ +while (true) { + read +=vread_iter (iter, src, left); +if (read == tsz) +break ; + + src += read; + left -= read; + +if (fault_in_iov_iter_writeable (iter, left)) { + ret = -EFAULT ; + goto out; + } }break ; + }case KCORE_USER :/* User page is handled prior to normal kernel page: */ if (copy_to_iter ((char *)start, tsz, iter) != tsz) {
@@ -582,10 +595,6 @@ static int open_kcore(struct inode *inode, struct file *filp)if (ret)return ret; - filp->private_data =kmalloc (PAGE_SIZE ,GFP_KERNEL ); -if (!filp->private_data) -return -ENOMEM ; -if (kcore_need_update)kcore_update_ram ();if (i_size_read (inode) != proc_root_kcore->size) {
@@ -596,16 +605,9 @@ static int open_kcore(struct inode *inode, struct file *filp)return 0 ; } -static int release_kcore (struct inode *inode,struct file *file) -{ -kfree (file->private_data); -return 0 ; -} -static const struct proc_ops kcore_proc_ops = { .proc_read_iter = read_kcore_iter, .proc_open = open_kcore, - .proc_release = release_kcore, .proc_lseek = default_llseek, };
include/linux/vmalloc.h +2 -1
@@ -9,6 +9,7 @@#include <asm/page.h> /* pgprot_t */ #include <linux/rbtree.h> #include <linux/overflow.h> +#include <linux/uio.h> #include <asm/vmalloc.h>
@@ -251,7 +252,7 @@ static inline void set_vm_flush_reset_perms(void *addr)#endif /* for /proc/kcore */ -extern long vread (char *buf,char *addr,unsigned long count); +extern long vread_iter (struct iov_iter *iter,const char *addr,size_t count);/* * Internals. Don't use..
mm/nommu.c +5 -5
@@ -36,6 +36,7 @@#include <linux/printk.h> #include <linux/uaccess.h> +#include <linux/uio.h> #include <asm/tlb.h> #include <asm/tlbflush.h> #include <asm/mmu_context.h>
@@ -198,14 +199,13 @@ unsigned long vmalloc_to_pfn(const void *addr) }EXPORT_SYMBOL (vmalloc_to_pfn); -long vread (char *buf,char *addr,unsigned long count) +long vread_iter (struct iov_iter *iter,const char *addr,size_t count) {/* Don't allow overflow */ -if ((unsigned long ) buf + count < count) - count = -(unsigned long ) buf; +if ((unsigned long ) addr + count < count) + count = -(unsigned long ) addr; -memcpy (buf, addr, count); -return count; +return copy_to_iter (addr, count, iter); }/*
mm/vmalloc.c +146 -88
340 lines not on this page · raw message
Signature · 1 line
2.39.2
Quoted 10 lines · Lorenzo Stoakes
While reviewing Baoquan's recent changes to permit
vread()access tovm_map_ramregions of vmalloc allocations, Willy pointed out [1] that it would be nice to refactorvread()as a whole, since its only user isread_kcore()and the existing form ofvread()necessitates the use of a bounce buffer.This patch series does exactly that, as well as adjusting how we read the kernel text section to avoid the use of a bounce buffer in this case as well.
Quoted 6 lines
This has been tested against the test case which motivated Baoquan's changes in the first place [2] which continues to function correctly, as do the vmalloc self tests.
[1] https://lore.kernel.org/all/Y8WfDSRkc%[email protected]/
[2] https://lore.kernel.org/all/[email protected]/T/#u
The whole series looks good to me.