Re: kern/55658: rumpvfs:t_etfs triggers bug in ufs_balloc_range() for 16KB page

[Rin Okuyama <rokuyama.rk%gmail.com@localhost>]

The following reply was made to PR kern/55658; it has been noted by GNATS.

From: Rin Okuyama <rokuyama.rk%gmail.com@localhost>
To: gnats-bugs%netbsd.org@localhost, Chuck Silvers <chuq%chuq.com@localhost>
Cc: 
Subject: Re: kern/55658: rumpvfs:t_etfs triggers bug in ufs_balloc_range() for
 16KB page
Date: Wed, 16 Sep 2020 01:09:23 +0900

 On 2020/09/15 2:45, Chuck Silvers wrote:
 >   This isn't the right fix for this problem.  Pages which are only
 >   partially backed by allocated blocks in the file system must not
 >   be mapped writable (at the pmap level) for user mappings,
 >   because the page fault that is taken by user code writing to
 >   the mapping of such a partially-backed page is the point where
 >   we are able to allocate the rest of the blocks for the page.
 >   If we allow user code to modify the page without a page fault,
 >   then when the page is cleaned, any modifications to the parts
 >   of the page that have no blocks allocated will be discarded.
 >   
 >   I think the right fix will be to continue setting PG_RDONLY as
 >   we do currently so that user mappings will behave correctly,
 >   and instead change ubc_fault_page() to ignore the PG_RDONLY flag
 >   and always pmap_enter() the page with the permissions of the
 >   original access_type.  It is the file system's responsibility
 >   to allocate blocks for any part of the file that is being modified
 >   by write() before calling into UBC to fill the pages for that
 >   range with the desired data.
 >   
 >   Note that I haven't tested this and there may well be
 >   other adjustments that need to be made elsewhere.
 
 Thank you very much for kind explanation!
 
 I've modified ubc_fault_page() accordingly:
 
 http://www.netbsd.org/~rin/uvm_bio_20200915.patch
 
 --- sys/uvm/uvm_bio.c	9 Jul 2020 09:24:32 -0000	1.121
 +++ sys/uvm/uvm_bio.c	15 Sep 2020 02:24:33 -0000
 @@ -235,9 +235,7 @@ static inline int
   ubc_fault_page(const struct uvm_faultinfo *ufi, const struct ubc_map *umap,
       struct vm_page *pg, vm_prot_t prot, vm_prot_t access_type, vaddr_t va)
   {
 -	vm_prot_t mask;
   	int error;
 -	bool rdonly;
   
   	KASSERT(rw_write_held(pg->uobject->vmobjlock));
   
 @@ -280,11 +278,8 @@ ubc_fault_page(const struct uvm_faultinf
   	    pg->offset < umap->writeoff ||
   	    pg->offset + PAGE_SIZE > umap->writeoff + umap->writelen);
   
 -	rdonly = uvm_pagereadonly_p(pg);
 -	mask = rdonly ? ~VM_PROT_WRITE : VM_PROT_ALL;
 -
   	error = pmap_enter(ufi->orig_map->pmap, va, VM_PAGE_TO_PHYS(pg),
 -	    prot & mask, PMAP_CANFAIL | (access_type & mask));
 +	    prot, PMAP_CANFAIL | access_type);
   
   	uvm_pagelock(pg);
   	uvm_pageactivate(pg);
 ----
 
 With this patch, rumpvfs:t_etfs test successfully passes on ibm4xx.
 
 Also, I've confirmed that the system boots multiuser and there's no
 regression in ATF for ibm4xx, as well as for macppc (normal 4KB page).
 
 Does this patch seem OK to you? Is there anything else to be checked
 before commit?
 
 Thanks,
 rin