Re: [PATCH 1/3] mm/mincore: make mincore() more conservative
From: Michal Hocko <mhocko@kernel.org>
Date: 2019-01-31 09:44:04
Also in:
linux-mm, lkml
On Wed 30-01-19 13:44:18, Vlastimil Babka wrote:
From: Jiri Kosina <redacted> The semantics of what mincore() considers to be resident is not completely clear, but Linux has always (since 2.3.52, which is when mincore() was initially done) treated it as "page is available in page cache". That's potentially a problem, as that [in]directly exposes meta-information about pagecache / memory mapping state even about memory not strictly belonging to the process executing the syscall, opening possibilities for sidechannel attacks. Change the semantics of mincore() so that it only reveals pagecache information for non-anonymous mappings that belog to files that the calling process could (if it tried to) successfully open for writing.
I agree that this is a better way than the original 574823bfab82
("Change mincore() to count "mapped" pages rather than "cached" pages").
One thing is still not clear to me though. Is the new owner/writeable
check OK for the Netflix-like usecases? I mean does happycache have
appropriate access to the cache data? I have tried to re-read the
original thread but couldn't find any confirmation.
I nit below
Originally-by: Linus Torvalds [off-list ref] Originally-by: Dominique Martinet [off-list ref] Cc: Dominique Martinet <asmadeus@codewreck.org> Cc: Andy Lutomirski <luto@amacapital.net> Cc: Dave Chinner <david@fromorbit.com> Cc: Kevin Easton <redacted> Cc: Matthew Wilcox <willy@infradead.org> Cc: Cyril Hrubis <chrubis@suse.cz> Cc: Tejun Heo <tj@kernel.org> Cc: Kirill A. Shutemov <redacted> Cc: Daniel Gruss <redacted> Signed-off-by: Jiri Kosina <redacted> Signed-off-by: Vlastimil Babka <redacted>
other than that looks good to me. Acked-by: Michal Hocko <mhocko@suse.com> If this still doesn't help happycache kind of workloads then we should add a capability check IMO but this looks like a decent foundation to me.
quoted hunk ↗ jump to hunk
--- mm/mincore.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)diff --git a/mm/mincore.c b/mm/mincore.c index 218099b5ed31..747a4907a3ac 100644 --- a/mm/mincore.c +++ b/mm/mincore.c@@ -169,6 +169,14 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end, return 0; } +static inline bool can_do_mincore(struct vm_area_struct *vma) +{ + return vma_is_anonymous(vma) || + (vma->vm_file && + (inode_owner_or_capable(file_inode(vma->vm_file)) + || inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0)); +}
This is hard to read. Can we do if (vma_is_anonymous(vma)) return true; if (!vma->vm_file) return false; return inode_owner_or_capable(file_inode(vma->vm_file)) || inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0; -- Michal Hocko SUSE Labs