lkml.org 
[lkml]   [2012]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] uprobes: write_opcode()->__replace_page() can race with try_to_unmap()

    * Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

    > > ---
    > > Subject: [PATCH] uprobes: write_opcode()->__replace_page() can race with try_to_unmap()
    > >
    > > write_opcode() gets old_page via get_user_pages() and then calls
    > > __replace_page() which assumes that this old_page is still mapped
    > > after pte_offset_map_lock().
    > >
    > > This is not true if this old_page was already try_to_unmap()'ed,
    > > and in this case everything __replace_page() does with old_page
    > > is wrong. Just for example, put_page() is not balanced.
    > >
    > > I think it is possible to teach __replace_page() to handle this
    > > unlikely case correctly, but this patch simply changes it to use
    > > page_check_address() and return -EAGAIN if it fails. The caller
    > > should notice this error code and retry.
    > >
    > > Note: write_opcode() asks for the cleanups, I'll try to do this
    > > in a separate patch.
    > >
    > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
    > > ---
    > > kernel/events/uprobes.c | 41 +++++++++++++----------------------------
    > > 1 files changed, 13 insertions(+), 28 deletions(-)
    > >
    > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
    > > index 9c53bc2..54c8780 100644
    > > --- a/kernel/events/uprobes.c
    > > +++ b/kernel/events/uprobes.c
    > > @@ -135,33 +135,17 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
    > > static int __replace_page(struct vm_area_struct *vma, struct page *page, struct page *kpage)
    > > {
    > > struct mm_struct *mm = vma->vm_mm;
    > > - pgd_t *pgd;
    > > - pud_t *pud;
    > > - pmd_t *pmd;
    > > - pte_t *ptep;
    > > - spinlock_t *ptl;
    > > unsigned long addr;
    > > - int err = -EFAULT;
    > > + spinlock_t *ptl;
    > > + pte_t *ptep;
    > >
    > > addr = page_address_in_vma(page, vma);
    > > if (addr == -EFAULT)
    > > - goto out;
    > > -
    > > - pgd = pgd_offset(mm, addr);
    > > - if (!pgd_present(*pgd))
    > > - goto out;
    > > -
    > > - pud = pud_offset(pgd, addr);
    > > - if (!pud_present(*pud))
    > > - goto out;
    > > + return -EFAULT;
    > >
    > > - pmd = pmd_offset(pud, addr);
    > > - if (!pmd_present(*pmd))
    > > - goto out;
    > > -
    > > - ptep = pte_offset_map_lock(mm, pmd, addr, &ptl);
    > > + ptep = page_check_address(page, mm, addr, &ptl, 0);
    > > if (!ptep)
    > > - goto out;
    > > + return -EAGAIN;
    > >
    > > get_page(kpage);
    > > page_add_new_anon_rmap(kpage, vma, addr);
    > > @@ -180,10 +164,8 @@ static int __replace_page(struct vm_area_struct *vma, struct page *page, struct
    > > try_to_free_swap(page);
    > > put_page(page);
    > > pte_unmap_unlock(ptep, ptl);
    > > - err = 0;
    > >
    > > -out:
    > > - return err;
    > > + return 0;
    > > }
    > >
    > > /**
    > > @@ -228,9 +210,10 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
    > > void *vaddr_old, *vaddr_new;
    > > struct vm_area_struct *vma;
    > > struct uprobe *uprobe;
    > > + unsigned long pgoff;
    > > loff_t addr;
    > > int ret;
    > > -
    > > +retry:
    >
    > Just a check on coding style: Shouldnt we have a preceeding blank
    > line before the goto label.

    Yeah, that's most likely helpful to readability.

    Thanks,

    Ingo


    \
     
     \ /
      Last update: 2012-06-15 15:01    [W:0.027 / U:30.860 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site