I've reverted the 'let's try to just remove the code' part in my tree.
But I didn't apply the two other patches yet. Any final comments
before that should happen?
I mentionned when sending the updated version that just checking file
permission might not be enough, e.g. a git tree is full of read-only
objects that someone might want to preload and think we might really
want to check both despite the overhead in the denied case.
Josh agreed and I meant to send a new version since nothing was
happening but work priorities got the better of me, and I was kind of
waiting for the ltp testcases[1] as well because aside from the few
tests I ran by hand I'm not sure the few hours of ltp/xfstests Jiri ran
did much but this is probably going to be a chicken-or-egg problem..
[1] https://github.com/linux-test-project/ltp/issues/461
Jiri Kosina wrote on Thu, Jan 24, 2019:
On Thu, 24 Jan 2019, Linus Torvalds wrote:
quoted
Side note: the inode_permission() addition to can_do_mincore() in that
patch 0002, seems to be questionable. We do
+static inline bool can_do_mincore(struct vm_area_struct *vma)
+{
+ return vma_is_anonymous(vma)
+ || (vma->vm_file && (vma->vm_file->f_mode & FMODE_WRITE))
+ || inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
+}
note how it tests whether vma->vm_file is NULL for the FMODE_WRITE
test, but not for the inode_permission() test.
So either we test unnecessarily in the second line, or we don't
properly test it in the third one.
I think the "test vm_file" thing may be unnecessary, because a
non-anonymous mapping should always have a file pointer and an inode.
But I could imagine some odd case (vdso mapping, anyone?) that
doesn't have a vm_file, but also isn't anonymous.
Hmm, good point.
So dropping the 'vma->vm_file' test and checking whether given vma is
special mapping should hopefully provide the desired semantics, shouldn't
it?
I think it's probably better to keep this simple, if we're going to
check something before accessing vm_file we might as well directly check
it.
I was thinking of something along the lines of:
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));
I dropped the first f_mode check because none of the known mincore users
open the files read-write, and the check is redundant with
inode_permission() so while it would probably be an optimisation in some
cases I do not think it is useful in practice.
On the other hand, I have no idea how expensive the inode_permission and
owner checks really are - do they try to refresh attributes on a
networked filesystem or would it trust the cache or is it fs dependant?
Honestly this is more a case of "the people who's be interested in
seeing this have no idea what they're doing" than lack of interest.. I
wouldn't mind if there were tests doing mincore on a bunch of special
files/mappings but I just tried on a few regular files by hand, this
isn't proper coverage; I'll try to take more time to test various
mappings today (JST).
Thanks,
--
Dominique
I was thinking of something along the lines of:
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));
I dropped the first f_mode check because none of the known mincore users
open the files read-write, and the check is redundant with
inode_permission() so while it would probably be an optimisation in some
cases I do not think it is useful in practice.
On the other hand, I have no idea how expensive the inode_permission and
owner checks really are - do they try to refresh attributes on a
networked filesystem or would it trust the cache or is it fs dependant?
Honestly this is more a case of "the people who's be interested in
seeing this have no idea what they're doing" than lack of interest.. I
wouldn't mind if there were tests doing mincore on a bunch of special
files/mappings but I just tried on a few regular files by hand, this
isn't proper coverage; I'll try to take more time to test various
mappings today (JST).
I've done some tests with this, it appears OK.
Obviously the tests I previously had done still work:
- user's own files are ok, even if read-only now.
- non-user writable files are ok.
- non-user non-writable files (e.g. system libs) aren't.
- root can still do anything.
On new tests:
- there are vmas with no file that aren't anonymous and come all the
way there (vvar and vdso), so factoring vma->vm_file check is definitely
needed.
- vsyscall doesn't reach can_do_mincore()
- [heap] [stack] and other fileless regular maps are anonymous
- I tried a char device (/dev/zero) and it was marked anonymous despite
mapping with MAP_SHARED, which is somewhat expected I guess?
- I couldn't map /proc or /sys files (no such device), so no mincore
there.
I'd post my test program but I actually added pr_info messages in
can_do_mincore to check what it returned because madvise dontneed isn't
guaranteed to evict pages so we can't rely on madvise dontneed + mincore
to return 0; not sure what to do for ltp... If anyone has a good idea of
how to check if mincore actually got granted permissions without
drop_caches I'll post to the ltp github.
Anything else to try?
Jiri, you've offered resubmitting the last two patches properly, can you
incorporate this change or should I just send this directly? (I'd take
most of your commit message and add your name somewhere)
Thanks,
--
Dominique
Jiri, you've offered resubmitting the last two patches properly, can you
incorporate this change or should I just send this directly? (I'd take
most of your commit message and add your name somewhere)
Jiri, you've offered resubmitting the last two patches properly, can you
incorporate this change or should I just send this directly? (I'd take
most of your commit message and add your name somewhere)
I'm not sure why I'm the main recipient of that mail but answering
because I am -- let's get these patches in through the regular -mm tree
though
--
Dominique
From: Michal Hocko <mhocko@kernel.org> Date: 2019-01-30 09:09:51
On Wed 30-01-19 00:52:02, Jiri Kosina wrote:
On Mon, 28 Jan 2019, Dominique Martinet wrote:
quoted
quoted
So, any objections to aproaching it this way?
I'm not sure why I'm the main recipient of that mail but answering
because I am -- let's get these patches in through the regular -mm tree
though
*prod to mm maintainers* (at least for an opinion)
Could you repost those patches please? The thread is long and it is not
really clear what is the most up-to-date state of patches (at least to
me).
--
Michal Hocko
SUSE Labs
I'm not sure why I'm the main recipient of that mail but answering
because I am -- let's get these patches in through the regular -mm
tree though
*prod to mm maintainers* (at least for an opinion)
Could you repost those patches please? The thread is long and it is not
really clear what is the most up-to-date state of patches (at least to
me).
Vlastimil seems to have one extra patch to go on top, so we agreed that
he'll be sending that as a complete self-contained series (either as a
followup to the very first e-mail in this monsterthread, or completely
separately) shortly.
Thanks,
--
Jiri Kosina
SUSE Labs