On Mon, Jul 30, 2018 at 8:26 PM, Hugh Dickins <hughd@xxxxxxxxxx> wrote: > On Mon, 30 Jul 2018, Linus Torvalds wrote: >> On Mon, Jul 30, 2018 at 2:53 PM Hugh Dickins <hughd@xxxxxxxxxx> wrote: >> > >> > I have no problem with reverting -rc7's vma_is_anonymous() series. >> >> I don't think we need to revert the whole series: I think the rest are >> all fairly obvious cleanups, and shouldn't really have any semantic >> changes. > > Okay. > >> >> It's literally only that last patch in the series that then changes >> that meaning of "vm_ops". And I don't really _mind_ that last step >> either, but since we don't know exactly what it was that it broke, and >> we're past rc7, I don't think we really have any option but the revert >> it. > > It took me a long time to grasp what was happening, that that last > patch bfd40eaff5ab was fixing. Not quite explained in the commit. > > I think it was that by mistakenly passing the vma_is_anonymous() test, > create_huge_pmd() gave the MAP_PRIVATE kcov mapping a THP (instead of > COWing pages from kcov); which the truncate then had to split, and in > going to do so, again hit the mistaken vma_is_anonymous() test, BUG. > >> >> And if we revert it, I think we need to just remove the >> VM_BUG_ON_VMA() that it was supposed to fix. Because I do think that >> it is quite likely that the real bug is that overzealous BUG_ON(), >> since I can't see any reason why anonymous mappings should be special >> there. > > Yes, that probably has to go: but it's not clear what state it leaves > us in, with an anon THP being split by a truncate, without the expected > locking; I don't remember offhand, probably a subtler bug than that BUG, > which you may or may not consider an improvement. > > I fear that Kirill has not missed inserting a vma_set_anonymous() from > somewhere that it should be, but rather that zygote is working with some > special mapping which used to satisfy vma_is_anonymous(), faults supplying > backing pages, but now comes out as !vma_is_anonymous(), so do_fault() > finds !dummy_vm_ops.fault hence SIGBUS. I've been only casually following this thread (mostly just glad Amit caught it and I could avoid having to bisect the issue in my own Android testing), but this bit starting to shake a few old cobwebs loose in my brain. I'm wondering if Zygote is utilizing ashmem here, and we're somehow traversing ashmem purged memory, or due to some setup issue the initial traverse isn't being zero-filled as expected? ashmem ranges are created using: shmem_file_setup() and shmem_zero_setup() https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/android/ashmem.c#n377 If we purge pages, it punches it out with: vfs_fallocate(range->asma->file, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, start, end - start); here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/android/ashmem.c#n447 But in ashmem_pin(), we don't do anything other then returning if we purged any page in the range. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/android/ashmem.c#n577 And I believe the future assumption is the if we traverse those pages they will be zero filled (if purged or even during the initial traversal after mmap) Its been a long time, and I've not looked at the code in question but it sounds from Hugh's comments above that we might instead get a SIGBUS here. Looking more at the problematic patch.. Amit: Does adding something like (whitespace damaged, apologies): index a1a0025..1af6915 100644 --- a/drivers/staging/android/ashmem.c +++ b/drivers/staging/android/ashmem.c @@ -402,7 +402,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma) fput(asma->file); goto out; } - } + } else + vma_set_anonymous(vma); if (vma->vm_file) fput(vma->vm_file); Seem to resolve it? (Sorry, I'd test it myself, but I'm away from my desk for the night). thanks -john
