convert read_kcore(), vread() to use iterators

Having 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.

fs/proc/kcore.c +0 -0

index 08b795fd80b4..25b44b303b35 100644

fs/proc/kcore.c +22 -5

@@ -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) {

Quoted 7 lines · Lorenzo Stoakes

Having 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.

Quoted 8 lines

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.

I don't understand the sentences here. In vread_iter(), the actual content reading is done in aligned_vread_iter(), otherwise we zero filling the region. In aligned_vread_iter(), we will use vmalloc_to_page() to get the mapped page and read out, otherwise zero fill. While in this patch, fault_in_iov_iter_writeable() fault in memory of iter one time and will bail out if failed. I am wondering why we continue faulting in until the operation is complete, and how that is done.

If we look into the failing point in vread_iter(), it's mainly coming from copy_page_to_iter_nofault(), e.g page_copy_sane() checking failed, i->data_source checking failed. If these conditional checking failed, should we continue reading again and again? And this is not related to memory faulting in. I saw your discussion with David, but I am still a little lost. Hope I can learn it, thanks in advance.

......

Quoted 4 lines

fs/proc/kcore.c +0 -0

index 08b795fd80b4..25b44b303b35 100644

......

Quoted 36 lines

fs/proc/kcore.c +22 -5

@@ -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) {

Quoted 17 lines · Baoquan He

Quoted 7 lines · Lorenzo Stoakes

Having 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.

Quoted 8 lines

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.

Quoted 7 lines

I don't understand the sentences here. In vread_iter(), the actual content reading is done in aligned_vread_iter(), otherwise we zero filling the region. In aligned_vread_iter(), we will use vmalloc_to_page() to get the mapped page and read out, otherwise zero fill. While in this patch, fault_in_iov_iter_writeable() fault in memory of iter one time and will bail out if failed. I am wondering why we continue faulting in until the operation is complete, and how that is done.

This is refererrring to what's happening in kcore.c, not vread_iter(), i.e. the looped read/faultin.

The reason we bail out if failt_in_iov_iter_writeable() is that would indicate an error had occurred.

The whole point is to _optimistically_ try to perform the operation assuming the pages are faulted in. Ultimately we fault in via copy_to_user_nofault() which will either copy data or fail if the pages are not faulted in (will discuss this below a bit more in response to your other point).

If this fails, then we fault in, and try again. We loop because there could be some extremely unfortunate timing with a race on e.g. swapping out or migrating pages between faulting in and trying to write out again.

This is extremely unlikely, but to avoid any chance of breaking userland we repeat the operation until it completes. In nearly all real-world situations it'll either work immediately or loop once.

Quoted 9 lines

If we look into the failing point in vread_iter(), it's mainly coming from copy_page_to_iter_nofault(), e.g page_copy_sane() checking failed, i->data_source checking failed. If these conditional checking failed, should we continue reading again and again? And this is not related to memory faulting in. I saw your discussion with David, but I am still a little lost. Hope I can learn it, thanks in advance.

Actually neither of these are going to happen. page_copy_sane() checks the sanity of the _source_ pages, and the 'sanity' is defined by whether your offset and length sit within the (possibly compound) folio. Since we control this, we can arrange for it never to happen.

i->data_source is checking that it's an output iterator, however we would already have checked this when writing ELF headers at the bare minimum, so we cannot reach this point with an invalid iterator.

Therefore it is not possible either cause a failure. What could cause a failure, and what we are checking for, is specified in copyout_nofault() (in iov_iter.c) which we pass to the iterate_and_advance() macro. Now we have a fault-injection should_fail_usercopy() which would just trigger a redo, or copy_to_user_nofault() returning < 0 (e.g. -EFAULT).

This code is confusing as this function returns the number of bytes _not copied_ rather than copied. I have tested this to be sure by the way :)

Therefore the only way for a failure to occur is for memory to not be faulted in and thus the loop only triggers in this situation. If we fail to fault in pages for any reason, the whole operation aborts so this should cover all angles.

Quoted 43 lines

......

Quoted 4 lines

fs/proc/kcore.c +0 -0

index 08b795fd80b4..25b44b303b35 100644

......

Quoted 36 lines

fs/proc/kcore.c +22 -5

@@ -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) {

Quoted 38 lines · Lorenzo Stoakes

Quoted 17 lines · Baoquan He

Quoted 7 lines · Lorenzo Stoakes

Having 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.

Quoted 8 lines

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.

Quoted 7 lines

I don't understand the sentences here. In vread_iter(), the actual content reading is done in aligned_vread_iter(), otherwise we zero filling the region. In aligned_vread_iter(), we will use vmalloc_to_page() to get the mapped page and read out, otherwise zero fill. While in this patch, fault_in_iov_iter_writeable() fault in memory of iter one time and will bail out if failed. I am wondering why we continue faulting in until the operation is complete, and how that is done.

This is refererrring to what's happening in kcore.c, not vread_iter(), i.e. the looped read/faultin.

The reason we bail out if failt_in_iov_iter_writeable() is that would indicate an error had occurred.

The whole point is to _optimistically_ try to perform the operation assuming the pages are faulted in. Ultimately we fault in via copy_to_user_nofault() which will either copy data or fail if the pages are not faulted in (will discuss this below a bit more in response to your other point).

Quoted 7 lines

If this fails, then we fault in, and try again. We loop because there could be some extremely unfortunate timing with a race on e.g. swapping out or migrating pages between faulting in and trying to write out again.

This is extremely unlikely, but to avoid any chance of breaking userland we repeat the operation until it completes. In nearly all real-world situations it'll either work immediately or loop once.

Thanks a lot for these helpful details with patience. I got it now. I was mainly confused by the while(true) loop in KCORE_VMALLOC case of read_kcore_iter.

Now is there any chance that the faulted in memory is swapped out or migrated again before vread_iter()? fault_in_iov_iter_writeable() will pin the memory? I didn't find it from code and document. Seems it only falults in memory. If yes, there's window between faluting in and copy_to_user_nofault().

Quoted 47 lines · Baoquan He

Quoted 38 lines · Lorenzo Stoakes

Quoted 17 lines · Baoquan He

Quoted 7 lines · Lorenzo Stoakes

Having 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.

Quoted 8 lines

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.

Quoted 7 lines

I don't understand the sentences here. In vread_iter(), the actual content reading is done in aligned_vread_iter(), otherwise we zero filling the region. In aligned_vread_iter(), we will use vmalloc_to_page() to get the mapped page and read out, otherwise zero fill. While in this patch, fault_in_iov_iter_writeable() fault in memory of iter one time and will bail out if failed. I am wondering why we continue faulting in until the operation is complete, and how that is done.

This is refererrring to what's happening in kcore.c, not vread_iter(), i.e. the looped read/faultin.

The reason we bail out if failt_in_iov_iter_writeable() is that would indicate an error had occurred.

The whole point is to _optimistically_ try to perform the operation assuming the pages are faulted in. Ultimately we fault in via copy_to_user_nofault() which will either copy data or fail if the pages are not faulted in (will discuss this below a bit more in response to your other point).

Quoted 7 lines

If this fails, then we fault in, and try again. We loop because there could be some extremely unfortunate timing with a race on e.g. swapping out or migrating pages between faulting in and trying to write out again.

This is extremely unlikely, but to avoid any chance of breaking userland we repeat the operation until it completes. In nearly all real-world situations it'll either work immediately or loop once.

Quoted 9 lines

Thanks a lot for these helpful details with patience. I got it now. I was mainly confused by the while(true) loop in KCORE_VMALLOC case of read_kcore_iter.

Now is there any chance that the faulted in memory is swapped out or migrated again before vread_iter()? fault_in_iov_iter_writeable() will pin the memory? I didn't find it from code and document. Seems it only falults in memory. If yes, there's window between faluting in and copy_to_user_nofault().

See the documentation of fault_in_safe_writeable():

"Note that we don't pin or otherwise hold the pages referenced that we fault in. There's no guarantee that they'll stay in memory for any duration of time."

Sent: 23 March 2023 13:32

Quoted 18 lines

Quoted 17 lines

Quoted 7 lines

If this fails, then we fault in, and try again. We loop because there could be some extremely unfortunate timing with a race on e.g. swapping out or migrating pages between faulting in and trying to write out again.

This is extremely unlikely, but to avoid any chance of breaking userland we repeat the operation until it completes. In nearly all real-world situations it'll either work immediately or loop once.

Thanks a lot for these helpful details with patience. I got it now. I was mainly confused by the while(true) loop in KCORE_VMALLOC case of read_kcore_iter.

Now is there any chance that the faulted in memory is swapped out or migrated again before vread_iter()? fault_in_iov_iter_writeable() will pin the memory? I didn't find it from code and document. Seems it only falults in memory. If yes, there's window between faluting in and copy_to_user_nofault().

Quoted 5 lines

See the documentation of fault_in_safe_writeable():

"Note that we don't pin or otherwise hold the pages referenced that we fault in. There's no guarantee that they'll stay in memory for any duration of time."

Thanks for the info. Then swapping out/migration could happen again, so that's why while(true) loop is meaningful.

From: Baoquan He

Quoted 1 line

Sent: 23 March 2023 13:32

...

Quoted 24 lines

Quoted 23 lines

Quoted 8 lines

Quoted 7 lines

If this fails, then we fault in, and try again. We loop because there could be some extremely unfortunate timing with a race on e.g. swapping out or migrating pages between faulting in and trying to write out again.

This is extremely unlikely, but to avoid any chance of breaking userland we repeat the operation until it completes. In nearly all real-world situations it'll either work immediately or loop once.

Quoted 9 lines

Thanks a lot for these helpful details with patience. I got it now. I was mainly confused by the while(true) loop in KCORE_VMALLOC case of read_kcore_iter.

Now is there any chance that the faulted in memory is swapped out or migrated again before vread_iter()? fault_in_iov_iter_writeable() will pin the memory? I didn't find it from code and document. Seems it only falults in memory. If yes, there's window between faluting in and copy_to_user_nofault().

See the documentation of fault_in_safe_writeable():

"Note that we don't pin or otherwise hold the pages referenced that we fault in. There's no guarantee that they'll stay in memory for any duration of time."

Quoted 2 lines

Thanks for the info. Then swapping out/migration could happen again, so that's why while(true) loop is meaningful.

One of the problems is that is the system is under severe memory pressure and you try to fault in (say) 20 pages, the first page might get unmapped in order to map the last one in.

So it is quite likely better to retry 'one page at a time'.

There have also been cases where the instruction to copy data has faulted for reasons other than 'page fault'.
ISTR an infinite loop being caused by misaligned accesses failing due to 'bad instruction choice' in the copy code.
While this is rally a bug, an infinite retry in a file read/write didn't make it easy to spot.

So maybe there are cases where a dropping back to a 'bounce buffer' may be necessary.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)