* Re: [RFC PATCH] 9p: create writeback fid on shared mmap [not found] <20201205130904.518104-1-cgxu519@mykernel.net> @ 2020-12-06 9:16 ` Dominique Martinet 2020-12-06 20:53 ` [V9fs-developer] " Dominique Martinet 0 siblings, 1 reply; 5+ messages in thread From: Dominique Martinet @ 2020-12-06 9:16 UTC (permalink / raw) To: Chengguang Xu; +Cc: ericvh, lucho, v9fs-developer, linux-kernel, linux-fsdevel Chengguang Xu wrote on Sat, Dec 05, 2020: > If vma is shared and the file was opened for writing, > we should also create writeback fid because vma may be > mprotected writable even if now readonly. Hm, I guess it makes sense. > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net> > --- > Caveat: Only compile tested. Will test later and add it to next then, might be a bit. > fs/9p/vfs_file.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/9p/vfs_file.c b/fs/9p/vfs_file.c > index b177fd3b1eb3..791839c2dd5c 100644 > --- a/fs/9p/vfs_file.c > +++ b/fs/9p/vfs_file.c > @@ -516,8 +516,7 @@ v9fs_mmap_file_mmap(struct file *filp, struct vm_area_struct *vma) > v9inode = V9FS_I(inode); > mutex_lock(&v9inode->v_mutex); > if (!v9inode->writeback_fid && > - (vma->vm_flags & VM_SHARED) && > - (vma->vm_flags & VM_WRITE)) { > + mapping_writably_mapped(filp->f_mapping)) { > /* > * clone a fid and add it to writeback_fid > * we do it during mmap instead of -- Dominique ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [V9fs-developer] [RFC PATCH] 9p: create writeback fid on shared mmap 2020-12-06 9:16 ` [RFC PATCH] 9p: create writeback fid on shared mmap Dominique Martinet @ 2020-12-06 20:53 ` Dominique Martinet 2020-12-07 6:02 ` Chengguang Xu 0 siblings, 1 reply; 5+ messages in thread From: Dominique Martinet @ 2020-12-06 20:53 UTC (permalink / raw) To: Chengguang Xu; +Cc: ericvh, lucho, linux-fsdevel, linux-kernel, v9fs-developer Dominique Martinet wrote on Sun, Dec 06, 2020: > Chengguang Xu wrote on Sat, Dec 05, 2020: > > If vma is shared and the file was opened for writing, > > we should also create writeback fid because vma may be > > mprotected writable even if now readonly. > > Hm, I guess it makes sense. I had a second look, and generic_file_readonly_mmap uses vma's `vma->vm_flags & VM_MAYWRITE` instead (together with VM_SHARED), while mapping_writably_mapped ultimately basically only seems to validate that the mapping is shared from a look at mapping_map_writable callers? It's not very clear to me. OTOH, VM_MAYWRITE is set anytime we have a shared map where file has been opened read-write, which seems to be what you want with regards to protecting from mprotect calls. How about simply changing check from WRITE to MAYWRITE? v9inode = V9FS_I(inode); mutex_lock(&v9inode->v_mutex); if (!v9inode->writeback_fid && (vma->vm_flags & VM_SHARED) && - (vma->vm_flags & VM_WRITE)) { + (vma->vm_flags & VM_MAYWRITE)) { /* * clone a fid and add it to writeback_fid * we do it during mmap instead of -- Dominique ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [V9fs-developer] [RFC PATCH] 9p: create writeback fid on shared mmap 2020-12-06 20:53 ` [V9fs-developer] " Dominique Martinet @ 2020-12-07 6:02 ` Chengguang Xu 2020-12-07 11:24 ` Dominique Martinet 0 siblings, 1 reply; 5+ messages in thread From: Chengguang Xu @ 2020-12-07 6:02 UTC (permalink / raw) To: Dominique Martinet Cc: ericvh, lucho, linux-fsdevel, linux-kernel, v9fs-developer ---- 在 星期一, 2020-12-07 04:53:18 Dominique Martinet <asmadeus@codewreck.org> 撰写 ---- > Dominique Martinet wrote on Sun, Dec 06, 2020: > > Chengguang Xu wrote on Sat, Dec 05, 2020: > > > If vma is shared and the file was opened for writing, > > > we should also create writeback fid because vma may be > > > mprotected writable even if now readonly. > > > > Hm, I guess it makes sense. > > I had a second look, and generic_file_readonly_mmap uses vma's > `vma->vm_flags & VM_MAYWRITE` instead (together with VM_SHARED), > while mapping_writably_mapped ultimately basically only seems to > validate that the mapping is shared from a look at mapping_map_writable > callers? It's not very clear to me. > > , VM_MAYWRITE is set anytime we have a shared map where file has > been opened read-write, which seems to be what you want with regards to > protecting from mprotect calls. > > How about simply changing check from WRITE to MAYWRITE? It would be fine and based on the code in do_mmap(), it seems we even don't need extra check here. The condition (vma->vm_flags & VM_SHARED) will be enough. Am I missing something? Thanks, Chengguang > > v9inode = V9FS_I(inode); > mutex_lock(&v9inode->v_mutex); > if (!v9inode->writeback_fid && > (vma->vm_flags & VM_SHARED) && > - (vma->vm_flags & VM_WRITE)) { > + (vma->vm_flags & VM_MAYWRITE)) { > /* > * clone a fid and add it to writeback_fid > * we do it during mmap instead of ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [V9fs-developer] [RFC PATCH] 9p: create writeback fid on shared mmap 2020-12-07 6:02 ` Chengguang Xu @ 2020-12-07 11:24 ` Dominique Martinet 2020-12-07 13:13 ` Chengguang Xu 0 siblings, 1 reply; 5+ messages in thread From: Dominique Martinet @ 2020-12-07 11:24 UTC (permalink / raw) To: Chengguang Xu; +Cc: ericvh, lucho, linux-fsdevel, linux-kernel, v9fs-developer Chengguang Xu wrote on Mon, Dec 07, 2020: > > , VM_MAYWRITE is set anytime we have a shared map where file has > > been opened read-write, which seems to be what you want with regards to > > protecting from mprotect calls. > > > > How about simply changing check from WRITE to MAYWRITE? > > It would be fine and based on the code in do_mmap(), it seems we even don't > need extra check here. The condition (vma->vm_flags & VM_SHARED) will be enough. > Am I missing something? VM_MAYWRITE is unset if the file hasn't been open for writing (in which case the mapping can't be mprotect()ed to writable map), so checking it is a bit more efficient. Anyway I'd like to obsolete the writeback fid uses now that fids have a refcount (this usecase can be a simple refcount increase), in which case efficiency is less of a problem, but we're not there yet... Please resend with MAYWRITE if you want authorship and I'll try to take some time to test incl. the mprotect usecase. -- Dominique ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [V9fs-developer] [RFC PATCH] 9p: create writeback fid on shared mmap 2020-12-07 11:24 ` Dominique Martinet @ 2020-12-07 13:13 ` Chengguang Xu 0 siblings, 0 replies; 5+ messages in thread From: Chengguang Xu @ 2020-12-07 13:13 UTC (permalink / raw) To: Dominique Martinet Cc: ericvh, lucho, linux-fsdevel, linux-kernel, v9fs-developer ---- 在 星期一, 2020-12-07 19:24:10 Dominique Martinet <asmadeus@codewreck.org> 撰写 ---- > Chengguang Xu wrote on Mon, Dec 07, 2020: > > > , VM_MAYWRITE is set anytime we have a shared map where file has > > > been opened read-write, which seems to be what you want with regards to > > > protecting from mprotect calls. > > > > > > How about simply changing check from WRITE to MAYWRITE? > > > > It would be fine and based on the code in do_mmap(), it seems we even don't > > need extra check here. The condition (vma->vm_flags & VM_SHARED) will be enough. > > Am I missing something? > > VM_MAYWRITE is unset if the file hasn't been open for writing (in which > case the mapping can't be mprotect()ed to writable map), so checking it > is a bit more efficient. > > Anyway I'd like to obsolete the writeback fid uses now that fids have a > refcount (this usecase can be a simple refcount increase), in which case > efficiency is less of a problem, but we're not there yet... > > Please resend with MAYWRITE if you want authorship and I'll try to take > some time to test incl. the mprotect usecase. > Thanks for the review, I'll send revised version later. Thanks, Chengguang ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-07 13:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20201205130904.518104-1-cgxu519@mykernel.net>
2020-12-06 9:16 ` [RFC PATCH] 9p: create writeback fid on shared mmap Dominique Martinet
2020-12-06 20:53 ` [V9fs-developer] " Dominique Martinet
2020-12-07 6:02 ` Chengguang Xu
2020-12-07 11:24 ` Dominique Martinet
2020-12-07 13:13 ` Chengguang Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).