| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 Sep 2016 14:54:47 +0200
From: Michal Hocko <mhocko@...nel.org>
To: Xiao Guangrong <guangrong.xiao@...ux.intel.com>
Cc: pbonzini@...hat.com, akpm@...ux-foundation.org,
dan.j.williams@...el.com, dave.hansen@...el.com, gleb@...nel.org,
mtosatti@...hat.com, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, stefanha@...hat.com,
yuhuang@...hat.com, linux-mm@...ck.org,
ross.zwisler@...ux.intel.com, Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH v2] mm, proc: Fix region lost in /proc/self/smaps
[Cc Oleg - he seemed to touch it the last. Keeping the whole email for
reference with a comment inline]
On Mon 12-09-16 11:12:44, Xiao Guangrong wrote:
> Recently, Redhat reported that nvml test suite failed on QEMU/KVM,
> more detailed info please refer to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1365721
>
> Actually, this bug is not only for NVDIMM/DAX but also for any other file
> systems. This simple test case abstracted from nvml can easily reproduce
> this bug in common environment:
>
> -------------------------- testcase.c -----------------------------
> #define PROCMAXLEN 4096
>
> int
> is_pmem_proc(const void *addr, size_t len)
> {
> const char *caddr = addr;
>
> FILE *fp;
> if ((fp = fopen("/proc/self/smaps", "r")) == NULL) {
> printf("!/proc/self/smaps");
> return 0;
> }
>
> int retval = 0; /* assume false until proven otherwise */
> char line[PROCMAXLEN]; /* for fgets() */
> char *lo = NULL; /* beginning of current range in smaps file */
> char *hi = NULL; /* end of current range in smaps file */
> int needmm = 0; /* looking for mm flag for current range */
> while (fgets(line, PROCMAXLEN, fp) != NULL) {
> static const char vmflags[] = "VmFlags:";
> static const char mm[] = " wr";
>
> /* check for range line */
> if (sscanf(line, "%p-%p", &lo, &hi) == 2) {
> if (needmm) {
> /* last range matched, but no mm flag found */
> printf("never found mm flag.\n");
> break;
> } else if (caddr < lo) {
> /* never found the range for caddr */
> printf("#######no match for addr %p.\n", caddr);
> break;
> } else if (caddr < hi) {
> /* start address is in this range */
> size_t rangelen = (size_t)(hi - caddr);
>
> /* remember that matching has started */
> needmm = 1;
>
> /* calculate remaining range to search for */
> if (len > rangelen) {
> len -= rangelen;
> caddr += rangelen;
> printf("matched %zu bytes in range "
> "%p-%p, %zu left over.\n",
> rangelen, lo, hi, len);
> } else {
> len = 0;
> printf("matched all bytes in range "
> "%p-%p.\n", lo, hi);
> }
> }
> } else if (needmm && strncmp(line, vmflags,
> sizeof(vmflags) - 1) == 0) {
> if (strstr(&line[sizeof(vmflags) - 1], mm) != NULL) {
> printf("mm flag found.\n");
> if (len == 0) {
> /* entire range matched */
> retval = 1;
> break;
> }
> needmm = 0; /* saw what was needed */
> } else {
> /* mm flag not set for some or all of range */
> printf("range has no mm flag.\n");
> break;
> }
> }
> }
>
> fclose(fp);
>
> printf("returning %d.\n", retval);
> return retval;
> }
>
> #define NTHREAD 16
>
> void *Addr;
> size_t Size;
>
> /*
> * worker -- the work each thread performs
> */
> static void *
> worker(void *arg)
> {
> int *ret = (int *)arg;
> *ret = is_pmem_proc(Addr, Size);
> return NULL;
> }
>
> int main(int argc, char *argv[])
> {
> if (argc < 2 || argc > 3) {
> printf("usage: %s file [env].\n", argv[0]);
> return -1;
> }
>
> int fd = open(argv[1], O_RDWR);
>
> struct stat stbuf;
> fstat(fd, &stbuf);
>
> Size = stbuf.st_size;
> Addr = mmap(0, stbuf.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
>
> close(fd);
>
> pthread_t threads[NTHREAD];
> int ret[NTHREAD];
>
> /* kick off NTHREAD threads */
> for (int i = 0; i < NTHREAD; i++)
> pthread_create(&threads[i], NULL, worker, &ret[i]);
>
> /* wait for all the threads to complete */
> for (int i = 0; i < NTHREAD; i++)
> pthread_join(threads[i], NULL);
>
> /* verify that all the threads return the same value */
> for (int i = 1; i < NTHREAD; i++) {
> if (ret[0] != ret[i]) {
> printf("Error i %d ret[0] = %d ret[i] = %d.\n", i,
> ret[0], ret[i]);
> }
> }
>
> printf("%d", ret[0]);
> return 0;
> }
>
> # dd if=/dev/zero of=~/out bs=2M count=1
> # ./testcase ~/out
>
> It failed as some threads can not find the memory region in
> "/proc/self/smaps" which is allocated in the main process
>
> It is caused by proc fs which uses 'file->version' to indicate the VMA that
> is the last one has already been handled by read() system call. When the
> next read() issues, it uses the 'version' to find the VMA, then the next
> VMA is what we want to handle, the related code is as follows:
>
> if (last_addr) {
> vma = find_vma(mm, last_addr);
> if (vma && (vma = m_next_vma(priv, vma)))
> return vma;
> }
>
> However, VMA will be lost if the last VMA is gone, e.g:
>
> The process VMA list is A->B->C->D
>
> CPU 0 CPU 1
> read() system call
> handle VMA B
> version = B
> return to userspace
>
> unmap VMA B
>
> issue read() again to continue to get
> the region info
> find_vma(version) will get VMA C
> m_next_vma(C) will get VMA D
> handle D
> !!! VMA C is lost !!!
>
> In order to fix this bug, we make 'file->version' indicate the end address
> of current VMA
Doesn't this open doors to another weird cases. Say B would be partially
unmapped (tail of the VMA would get unmapped and reused for a new VMA.
I am not sure we provide any guarantee when there are more read
syscalls. Hmm, even with a single read() we can get inconsistent results
from different threads without any user space synchronization.
So in other words isn't this fixing a bug by introducing a slightly
different one while we are not really guaranteeing anything strong here?
> Changelog:
> Thanks to Dave Hansen's comments, this version fixes the issue in v1 that
> same virtual address range may be outputted twice, e.g:
>
> Take two example VMAs:
>
> vma-A: (0x1000 -> 0x2000)
> vma-B: (0x2000 -> 0x3000)
>
> read() #1: prints vma-A, sets m->version=0x2000
>
> Now, merge A/B to make C:
>
> vma-C: (0x1000 -> 0x3000)
>
> read() #2: find_vma(m->version=0x2000), returns vma-C, prints vma-C
>
> The user will see two VMAs in their output:
>
> A: 0x1000->0x2000
> C: 0x1000->0x3000
>
> Acked-by: Dave Hansen <dave.hansen@...el.com>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@...ux.intel.com>
> ---
> fs/proc/task_mmu.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 187d84e..10ca648 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -147,7 +147,7 @@ m_next_vma(struct proc_maps_private *priv, struct vm_area_struct *vma)
> static void m_cache_vma(struct seq_file *m, struct vm_area_struct *vma)
> {
> if (m->count < m->size) /* vma is copied successfully */
> - m->version = m_next_vma(m->private, vma) ? vma->vm_start : -1UL;
> + m->version = m_next_vma(m->private, vma) ? vma->vm_end : -1UL;
> }
>
> static void *m_start(struct seq_file *m, loff_t *ppos)
> @@ -176,14 +176,14 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
>
> if (last_addr) {
> vma = find_vma(mm, last_addr);
> - if (vma && (vma = m_next_vma(priv, vma)))
> + if (vma)
> return vma;
> }
>
> m->version = 0;
> if (pos < mm->map_count) {
> for (vma = mm->mmap; pos; pos--) {
> - m->version = vma->vm_start;
> + m->version = vma->vm_end;
> vma = vma->vm_next;
> }
> return vma;
> @@ -293,7 +293,7 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
> vm_flags_t flags = vma->vm_flags;
> unsigned long ino = 0;
> unsigned long long pgoff = 0;
> - unsigned long start, end;
> + unsigned long end, start = m->version;
> dev_t dev = 0;
> const char *name = NULL;
>
> @@ -304,8 +304,13 @@ show_map_vma(struct seq_file *m, struct vm_area_struct *vma, int is_pid)
> pgoff = ((loff_t)vma->vm_pgoff) << PAGE_SHIFT;
> }
>
> + /*
> + * the region [0, m->version) has already been handled, do not
> + * handle it doubly.
> + */
> + start = max(vma->vm_start, start);
> +
> /* We don't show the stack guard page in /proc/maps */
> - start = vma->vm_start;
> if (stack_guard_page_start(vma, start))
> start += PAGE_SIZE;
> end = vma->vm_end;
> --
> 1.8.3.1
>
--
Michal Hocko
SUSE Labs
Powered by blists - more mailing lists