Re: mm: shm: hang in shmem_fallocate
From: Vlastimil Babka
Date: Thu Jun 26 2014 - 11:19:33 EST
On 06/26/2014 11:14 AM, Vlastimil Babka wrote:
On 06/26/2014 12:36 AM, Hugh Dickins wrote:
On Tue, 24 Jun 2014, Vlastimil Babka wrote:
On 06/16/2014 04:29 AM, Hugh Dickins wrote:
On Thu, 12 Jun 2014, Sasha Levin wrote:
On 02/09/2014 08:41 PM, Sasha Levin wrote:
On 02/08/2014 10:25 PM, Hugh Dickins wrote:
Would trinity be likely to have a thread or process repeatedly faulting
in pages from the hole while it is being punched?
I can see how trinity would do that, but just to be certain - Cc davej.
On 02/08/2014 10:25 PM, Hugh Dickins wrote:
Does this happen with other holepunch filesystems? If it does not,
I'd suppose it's because the tmpfs fault-in-newly-created-page path
is lighter than a consistent disk-based filesystem's has to be.
But we don't want to make the tmpfs path heavier to match them.
No, this is strictly limited to tmpfs, and AFAIK trinity tests hole
punching in other filesystems and I make sure to get a bunch of those
mounted before starting testing.
Just pinging this one again. I still see hangs in -next where the hang
location looks same as before:
Please give this patch a try. It fixes what I can reproduce, but given
your unexplained page_mapped() BUG in this area, we know there's more
yet to be understood, so perhaps this patch won't do enough for you.
Hi,
Sorry for the slow response: I have got confused, learnt more, and
changed my mind, several times in the course of replying to you.
I think this reply will be stable... though not final.
Thanks a lot for looking into it!
since this got a CVE,
Oh. CVE-2014-4171. Couldn't locate that yesterday but see it now.
Sorry, I should have mentioned it explicitly.
Looks overrated to me
I'd bet it would pass unnoticed if you didn't use the sentence "but
whether it's a serious matter in the scale of denials of service, I'm
not so sure" in your first reply to Sasha's report :) I wouldn't be
surprised if people grep for this.
(and amusing to see my pompous words about a
"range notification mechanism" taken too seriously), but of course
we do need to address it.
I've been looking at backport to an older kernel where
Thanks a lot for looking into it. I didn't think it was worth a
Cc: stable@xxxxxxxxxxxxxxx myself, but admit to being both naive
and inconsistent about that.
fallocate(FALLOC_FL_PUNCH_HOLE) is not yet supported, and there's also no
range notification mechanism yet. There's just madvise(MADV_REMOVE) and since
Yes, that mechanism could be ported back pre-v3.5,
but I agree with your preference not to.
it doesn't guarantee anything, it seems simpler just to give up retrying to
Right, I don't think we have formally documented the instant of "full hole"
that I strove for there, and it's probably not externally verifiable, nor
guaranteed by other filesystems. I just thought it a good QoS aim, but
it has given us this problem.
truncate really everything. Then I realized that maybe it would work for
current kernel as well, without having to add any checks in the page fault
path. The semantics of fallocate(FALLOC_FL_PUNCH_HOLE) might look different
from madvise(MADV_REMOVE), but it seems to me that as long as it does discard
the old data from the range, it's fine from any information leak point of view.
If someone races page faulting, it IMHO doesn't matter if he gets a new zeroed
page before the parallel truncate has ended, or right after it has ended.
Yes. I disagree with your actual patch, for more than one reason,
but it's in the right area; and I found myself growing to agree with
you, that's it's better to have one kind of fix for all these releases,
than one for v3.5..v3.15 and another for v3.1..v3.4. (The CVE cites
v3.0 too, I'm sceptical about that, but haven't tried it as yet.)
I was looking at our 3.0 based kernel, but it could be due to backported
patches on top.
OK, seems I cannot reproduce this on 3.0.101 vanilla.
If I'd realized that we were going to have to backport, I'd have spent
longer looking for a patch like yours originally. So my inclination
now is to go your route, make a new patch for v3.16 and backports,
and revert the f00cdc6df7d7 that has already gone in.
So I'm posting it here as a RFC. I haven't thought about the
i915_gem_object_truncate caller yet. I think that this path wouldn't satisfy
My understanding is that i915_gem_object_truncate() is not a problem,
that i915's dev->struct_mutex serializes all its relevant transitions,
plus the object woudn't even be interestingly accessible to the user.
the new "lstart < inode->i_size" condition, but I don't know if it's "vulnerable"
to the problem.
I don't think i915 is vulnerable, but if it is, that condition would
be fine for it, as would be the patch I'm now thinking of.
-----8<-----
From: Vlastimil Babka <vbabka@xxxxxxx>
Subject: [RFC PATCH] shmem: prevent livelock between page fault and hole punching
---
mm/shmem.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/mm/shmem.c b/mm/shmem.c
index f484c27..6d6005c 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -476,6 +476,25 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
if (!pvec.nr) {
if (index == start || unfalloc)
break;
+ /*
+ * When this condition is true, it means we were
+ * called from fallocate(FALLOC_FL_PUNCH_HOLE).
+ * To prevent a livelock when someone else is faulting
+ * pages back, we are content with single pass and do
+ * not retry with index = start. It's important that
+ * previous page content has been discarded, and
+ * faulter(s) got new zeroed pages.
+ *
+ * The other callsites are shmem_setattr (for
+ * truncation) and shmem_evict_inode, which set i_size
+ * to truncated size or 0, respectively, and then call
+ * us with lstart == inode->i_size. There we do want to
+ * retry, and livelock cannot happen for other reasons.
+ *
+ * XXX what about i915_gem_object_truncate?
+ */
I doubt you have ever faced such a criticism before, but I'm going
to speak my mind and say that comment is too long! A comment of that
length is okay above or just inside or at a natural break in a function,
but here it distracts too much from what the code is actually doing.
Fair enough. The reasoning should have gone into commit log, not comment.
In particular, the words "this condition" are so much closer to the
condition above than the condition below, that it's rather confusing.
/* Single pass when hole-punching to not livelock on racing faults */
would have been enough (yes, I've cheated, that would be 2 or 4 lines).
+ if (lstart < inode->i_size)
For a long time I was going to suggest that you leave i_size out of it,
and use "lend > 0" instead. Then suddenly I realized that this is the
wrong place for the test.
Well my first idea was to just add a flag about how persistent it should
be. And set it false for the punch hole case. Then I wondered if there's
already some bit that distinguishes it. But it makes it more subtle.
And then that it's not your fault, it's mine,
in v3.1's d0823576bf4b "mm: pincer in truncate_inode_pages_range".
Wow, that really pessimized the hole-punch case!
When is pvec.nr 0? When we've reached the end of the file. Why should
we go to the end of the file, when punching a hole at the start? Ughh!
Ah, I see (I think). But I managed to reproduce this problem when there
was only an extra page between lend and the end of file, so I doubt this
is the only problem. AFAIU it's enough to try punching a large enough
hole, then the loop can only do a single pagevec worth of pages per
iteration, which gives enough time for somebody faulting pages back?
+ break;
index = start;
continue;
}
--
1.8.4.5
But there is another problem. We cannot break out after one pass on
shmem, because there's a possiblilty that a swap entry in the radix_tree
got swizzled into a page just as it was about to be removed - your patch
might then leave that data behind in the hole.
Thanks, I didn't notice that. Do I understand correctly that this could
mean info leak for the punch hole call, but wouldn't be a problem for
madvise? (In any case, that means the solution is not general enough for
all kernels, so I'm asking just to be sure).
As it happens, Konstantin Khlebnikov suggested a patch for that a few
weeks ago, before noticing that it's already handled by the endless loop.
If we make that loop no longer endless, we need to add in Konstantin's
"if (shmem_free_swap) goto retry" patch.
Right now I'm thinking that my idiocy in d0823576bf4b may actually
be the whole of Trinity's problem: patch below. If we waste time
traversing the radix_tree to end-of-file, no wonder that concurrent
faults have time to put something in the hole every time.
Sasha, may I trespass on your time, and ask you to revert the previous
patch from your tree, and give this patch below a try? I am very
interested to learn if in fact it fixes it for you (as it did for me).
I will try this, but as I explained above, I doubt that alone will help.
Yep, it didn't help here.
However, I am wasting your time, in that I think we shall decide that
it's too unsafe to rely solely upon the patch below (what happens if
1024 cpus are all faulting on it while we try to punch a 4MB hole at
My reproducer is 4MB file, where the puncher tries punching everything
except first and last page. And there are 8 other threads (as I have 8
logical CPU's) that just repeatedly sweep the same range, reading only
the first byte of each page.
end of file? if we care). I think we shall end up with the optimization
below (or some such: it can be written in various ways), plus reverting
d0823576bf4b's "index == start && " pincer, plus Konstantin's
shmem_free_swap handling, rolled into a single patch; and a similar
So that means no retry in any case (except the swap thing)? All callers
can handle that? I guess shmem_evict_inode would be ok, as nobody else
can be accessing that inode. But what about shmem_setattr? (i.e.
straight truncation) As you said earlier, faulters will get a SIGBUS
(which AFAIU is due to i_size being updated before we enter
shmem_undo_range). But could possibly a faulter already pass the i_size
test, and proceed with the fault only when we are already in
shmem_undo_range and have passed the page in question?
patch (without the swap part) for several functions in truncate.c.
Hugh
--- 3.16-rc2/mm/shmem.c 2014-06-16 00:28:55.124076531 -0700
+++ linux/mm/shmem.c 2014-06-25 10:28:47.063967052 -0700
@@ -470,6 +470,7 @@ static void shmem_undo_range(struct inod
for ( ; ; ) {
cond_resched();
+ index = min(index, end);
pvec.nr = find_get_entries(mapping, index,
min(end - index, (pgoff_t)PAGEVEC_SIZE),
pvec.pages, indices);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/