Copyout(9) on 4/4 i386 needs correct vm_page_array[].

[kib]

Summary

On the 4/4 i386, copyout(9) may need to call pmap_extract_and_hold() on arbitrary userspace mapping. If the mapping is backed by the non-managed cdev pager or by the sg pager, on dense configs we might access arbitrary element of vm_page_array[], in particular, not corresponding to a page from the memory segment. Initialize such pages as fictitious with the corresponding physical address.

Also, if the address falls out of the vm_page_array[] boundaries, PHYS_TO_VM_PAGE() returns NULL, which should be handled by pmap_extract_and_hold().

N.B. More elegant and probably reasonable patch would only touch uninitialized gaps in the vm_page_array[], then i386 ifdef can be removed, but I do not see an easy way to enumerate gaps.

Test Plan

This was demonstrated by bde, using acpidump -dt.

[alc]

sys/i386/i386/pmap.c

1687–1688 ↗

(On Diff #44722)

Is this a universal problem, i.e., a problem on all architectures? For example, couldn't unmanaged pages be the source data for the fast path in the pipe subsystem?

[kib]

sys/i386/i386/pmap.c

1687–1688 ↗

(On Diff #44722)

At least for amd64 yes. For other pmaps its needs an inspection.

[kib]

Add amd64 pmap_extract_and_hold() fix.

I propose to go with this patch for now, and I will look at other pmaps when I am more operational.

[alc]

In D16085#341502, @kib wrote:

Add amd64 pmap_extract_and_hold() fix.

I propose to go with this patch for now, and I will look at other pmaps when I am more operational.

No objections.

sys/amd64/amd64/pmap.c

2321 ↗

(On Diff #44806)

I speculate that passing "pa" to PHYS_TO_VM_PAGE() here (and in the similar snippet on i386) will result in smaller, faster code because the compiler won't need to keep "pte" live across a function call.

sys/i386/i386/pmap.c

1687–1688 ↗

(On Diff #44722)

Can't the same problem arise at the PDE level because of pmap_object_init_pt() creating a fictitious, unmanaged mapping? I would suggest adding the conditional there too.

This applies to amd64 too.

sys/vm/vm_page.c

798 ↗

(On Diff #44806)

Isn't this "off-by-one" on the typical i386 machine? I thought that the first entry in the array typically had the physical address 4096.

[kib]

Alan, I received your comments right after I committed the change. Sorry.

Use pa for PHYS_TO_VM_PAGE().
Check m != NULL for superpages as well.
Add first_page to correct phys_addr for the fictitious pages in vm_page_array (strange that it worked for bde).
Only initialize for DENSE configs on i386, PAE is SPARCE.

[alc]

In D16085#342041, @kib wrote:

Alan, I received your comments right after I committed the change. Sorry.

No apology necessary.

Use pa for PHYS_TO_VM_PAGE().
Check m != NULL for superpages as well.
Add first_page to correct phys_addr for the fictitious pages in vm_page_array (strange that it worked for bde).

Isn't that because we always initialize the physical address of a fictitious page?

Only initialize for DENSE configs on i386, PAE is SPARCE.

sys/amd64/amd64/pmap.c

2310–2311 ↗

(On Diff #44875)

You can use "pa" here too. (I checked the assembly, and the obvious difference is that the new stack frame is 16 bytes smaller.)

sys/i386/i386/pmap.c

1675–1676 ↗

(On Diff #44875)

Ditto.

sys/vm/vm_page.c

554 ↗

(On Diff #44875)

&& defined(VM_PHYSSEG_DENSE)

[alc]

In D16085#342120, @alc wrote:

In D16085#342041, @kib wrote:

Add first_page to correct phys_addr for the fictitious pages in vm_page_array (strange that it worked for bde).

Isn't that because we always initialize the physical address of a fictitious page?

We initialize the physical address in the dynamically allocated vm_page structure from the fakepg_zone, and that address gets used when we establish a virtual-to-physical mapping. But, I don't think that we ever touch the physical address field in the vm_page structure that we obtain from vm_page_array through pmap_extract_and_hold().

[kib]

In D16085#342129, @alc wrote:

We initialize the physical address in the dynamically allocated vm_page structure from the fakepg_zone, and that address gets used when we establish a virtual-to-physical mapping. But, I don't think that we ever touch the physical address field in the vm_page structure that we obtain from vm_page_array through pmap_extract_and_hold().

Please look at the i386/copyout.c:cp_slow0(). I do vm_fault_quick_hold_pages() there, and then I use the physical address of the returned pages to establish ephemeral kernel mapping for the copyout. The pages returned from _quick_hold are from vm_page_array[].

In fact, the same note is valid for most other users of _quick_hold, e.g. pipe buffer mapping or kern_physio.c.

[kib]

Use pa for superpages as well. Guard ii declaration properly.

[alc]

In D16085#342214, @kib wrote:

In D16085#342129, @alc wrote:

We initialize the physical address in the dynamically allocated vm_page structure from the fakepg_zone, and that address gets used when we establish a virtual-to-physical mapping. But, I don't think that we ever touch the physical address field in the vm_page structure that we obtain from vm_page_array through pmap_extract_and_hold().

Please look at the i386/copyout.c:cp_slow0(). I do vm_fault_quick_hold_pages() there, and then I use the physical address of the returned pages to establish ephemeral kernel mapping for the copyout. The pages returned from _quick_hold are from vm_page_array[].

In fact, the same note is valid for most other users of _quick_hold, e.g. pipe buffer mapping or kern_physio.c.

Then, I haven't a clue as to how the earlier version worked for bde@. :-)

P.S. Can't we use vm_page_unhold_pages() at the end of cp_slow()?