| Message ID | fbce01cf521414fa5b27d7e053c343f538695799.1485547404.git.joseph.salisbury@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 01/27/2017 05:08 PM, Joseph Salisbury wrote: > From: Keno Fischer <keno@juliacomputing.com> > > BugLink: http://bugs.launchpad.net/bugs/1658270 > > In 19be0eaff ("mm: remove gup_flags FOLL_WRITE games from __get_user_pages()"), > the mm code was changed from unsetting FOLL_WRITE after a COW was resolved to > setting the (newly introduced) FOLL_COW instead. Simultaneously, the check in > gup.c was updated to still allow writes with FOLL_FORCE set if FOLL_COW had > also been set. However, a similar check in huge_memory.c was forgotten. As a > result, remote memory writes to ro regions of memory backed by transparent huge > pages cause an infinite loop in the kernel (handle_mm_fault sets FOLL_COW and > returns 0 causing a retry, but follow_trans_huge_pmd bails out immidiately > because `(flags & FOLL_WRITE) && !pmd_write(*pmd)` is true. While in this > state the process is stil SIGKILLable, but little else works (e.g. no ptrace > attach, no other signals). This is easily reproduced with the following > code (assuming thp are set to always): > > #include <assert.h> > #include <fcntl.h> > #include <stdint.h> > #include <stdio.h> > #include <string.h> > #include <sys/mman.h> > #include <sys/stat.h> > #include <sys/types.h> > #include <sys/wait.h> > #include <unistd.h> > > #define TEST_SIZE 5 * 1024 * 1024 > > int main(void) { > int status; > pid_t child; > int fd = open("/proc/self/mem", O_RDWR); > void *addr = mmap(NULL, TEST_SIZE, PROT_READ, > MAP_ANONYMOUS | MAP_PRIVATE, 0, 0); > assert(addr != MAP_FAILED); > pid_t parent_pid = getpid(); > if ((child = fork()) == 0) { > void *addr2 = mmap(NULL, TEST_SIZE, PROT_READ | PROT_WRITE, > MAP_ANONYMOUS | MAP_PRIVATE, 0, 0); > assert(addr2 != MAP_FAILED); > memset(addr2, 'a', TEST_SIZE); > pwrite(fd, addr2, TEST_SIZE, (uintptr_t)addr); > return 0; > } > assert(child == waitpid(child, &status, 0)); > assert(WIFEXITED(status) && WEXITSTATUS(status) == 0); > return 0; > } > > Fix this by updating follow_trans_huge_pmd in huge_memory.c analogously to > the update in gup.c in the original commit. The same pattern exists in > follow_devmap_pmd. However, we should not be able to reach that check > with FOLL_COW set, so add WARN_ONCE to make sure we notice if we ever > do. > > Signed-off-by: Keno Fischer <keno@juliacomputing.com> > Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Acked-by: Michal Hocko <mhocko@suse.com> > Tested-by: David Rientjes <rientjes@google.com> > Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com> > --- > mm/huge_memory.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 65aa131..4333213 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -1230,6 +1230,16 @@ out_unlock: > return ret; > } > > +/* > + * FOLL_FORCE can write to even unwritable pmd's, but only > + * after we've gone through a COW cycle and they are dirty. > + */ > +static inline bool can_follow_write_pmd(pmd_t pmd, unsigned int flags) > +{ > + return pmd_write(pmd) || > + ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pmd_dirty(pmd)); > +} > + > struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, > unsigned long addr, > pmd_t *pmd, > @@ -1240,7 +1250,7 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, > > assert_spin_locked(pmd_lockptr(mm, pmd)); > > - if (flags & FOLL_WRITE && !pmd_write(*pmd)) > + if (flags & FOLL_WRITE && !can_follow_write_pmd(*pmd, flags)) > goto out; > > /* Avoid dumping huge zero page */ It looks like this patch will also be needed in Yakkety and Zesty, since both will also have the mainline commit that introduced the bug: 19be0eaffa3a mm: remove gup_flags FOLL_WRITE games from __get_user_pages()
This patch is following up on one that was a CVE and seems to cause infinite looping in some cases at least on Xenial[1]. So ACK for Trusty, Xenial, Yakkety, and Zesty. Maybe Precise, too? -Stefan [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1660518
On 31.01.2017 09:49, Stefan Bader wrote: > This patch is following up on one that was a CVE and seems to cause infinite > looping in some cases at least on Xenial[1]. So ACK for Trusty, Xenial, Yakkety, > and Zesty. Maybe Precise, too? I would say Precise also needs this since some variant of the "mm: remove gup_flags FOLL_WRITE games from __get_user_pages()" went there as well. -Stefan > [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1660518
Applied to xenial master-next branch. Regards. Cascardo.
Applied to yakkety master-next branch. Thanks. Cascardo.
ACK for Vivid, as it applies and builds cleanly, and the corresponding
gup commit has been applied.
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 65aa131..4333213 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -1230,6 +1230,16 @@ out_unlock: return ret; } +/* + * FOLL_FORCE can write to even unwritable pmd's, but only + * after we've gone through a COW cycle and they are dirty. + */ +static inline bool can_follow_write_pmd(pmd_t pmd, unsigned int flags) +{ + return pmd_write(pmd) || + ((flags & FOLL_FORCE) && (flags & FOLL_COW) && pmd_dirty(pmd)); +} + struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmd, @@ -1240,7 +1250,7 @@ struct page *follow_trans_huge_pmd(struct vm_area_struct *vma, assert_spin_locked(pmd_lockptr(mm, pmd)); - if (flags & FOLL_WRITE && !pmd_write(*pmd)) + if (flags & FOLL_WRITE && !can_follow_write_pmd(*pmd, flags)) goto out; /* Avoid dumping huge zero page */