| 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
| ||
|
Message-ID: <6A3DF150A5B70D4F9B66A25E3F7C888D07183CBB@039-SN2MPN1-011.039d.mgd.msft.net>
Date: Thu, 26 Sep 2013 03:57:36 +0000
From: Bhushan Bharat-R65777 <R65777@...escale.com>
To: Alex Williamson <alex.williamson@...hat.com>
CC: "joro@...tes.org" <joro@...tes.org>,
"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
"galak@...nel.crashing.org" <galak@...nel.crashing.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"agraf@...e.de" <agraf@...e.de>,
Wood Scott-B07421 <B07421@...escale.com>,
"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>
Subject: RE: [PATCH 6/7] vfio: moving some functions in common file
> -----Original Message-----
> From: linux-pci-owner@...r.kernel.org [mailto:linux-pci-owner@...r.kernel.org]
> On Behalf Of Alex Williamson
> Sent: Wednesday, September 25, 2013 10:33 PM
> To: Bhushan Bharat-R65777
> Cc: joro@...tes.org; benh@...nel.crashing.org; galak@...nel.crashing.org; linux-
> kernel@...r.kernel.org; linuxppc-dev@...ts.ozlabs.org; linux-
> pci@...r.kernel.org; agraf@...e.de; Wood Scott-B07421; iommu@...ts.linux-
> foundation.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 6/7] vfio: moving some functions in common file
>
> On Thu, 2013-09-19 at 12:59 +0530, Bharat Bhushan wrote:
> > Some function defined in vfio_iommu_type1.c were common and we want to
> > use these for FSL IOMMU (PAMU) and iommu-none driver.
> > So some of them are moved to vfio_iommu_common.c
> >
> > I think we can do more of that but we will take this step by step.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@...escale.com>
> > ---
> > drivers/vfio/Makefile | 4 +-
> > drivers/vfio/vfio_iommu_common.c | 235
> ++++++++++++++++++++++++++++++++++++++
> > drivers/vfio/vfio_iommu_common.h | 30 +++++
> > drivers/vfio/vfio_iommu_type1.c | 206
> > +---------------------------------
> > 4 files changed, 268 insertions(+), 207 deletions(-) create mode
> > 100644 drivers/vfio/vfio_iommu_common.c create mode 100644
> > drivers/vfio/vfio_iommu_common.h
> >
> > diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile index
> > 72bfabc..c5792ec 100644
> > --- a/drivers/vfio/Makefile
> > +++ b/drivers/vfio/Makefile
> > @@ -1,4 +1,4 @@
> > obj-$(CONFIG_VFIO) += vfio.o
> > -obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> > -obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> > +obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_common.o
> > +vfio_iommu_type1.o
> > +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_common.o
> > +vfio_iommu_spapr_tce.o
> > obj-$(CONFIG_VFIO_PCI) += pci/
> > diff --git a/drivers/vfio/vfio_iommu_common.c
> > b/drivers/vfio/vfio_iommu_common.c
> > new file mode 100644
> > index 0000000..8bdc0ea
> > --- /dev/null
> > +++ b/drivers/vfio/vfio_iommu_common.c
> > @@ -0,0 +1,235 @@
> > +/*
> > + * VFIO: Common code for vfio IOMMU support
> > + *
> > + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> > + * Author: Alex Williamson <alex.williamson@...hat.com>
> > + * Author: Bharat Bhushan <bharat.bhushan@...escale.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * Derived from original vfio:
> > + * Copyright 2010 Cisco Systems, Inc. All rights reserved.
> > + * Author: Tom Lyon, pugs@...co.com
> > + */
> > +
> > +#include <linux/compat.h>
> > +#include <linux/device.h>
> > +#include <linux/fs.h>
> > +#include <linux/iommu.h>
> > +#include <linux/module.h>
> > +#include <linux/mm.h>
> > +#include <linux/pci.h> /* pci_bus_type */
> > +#include <linux/rbtree.h>
> > +#include <linux/sched.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/vfio.h>
> > +#include <linux/workqueue.h>
>
> Please cleanup includes on both the source and target files. You obviously
> don't need linux/pci.h here for one.
Will do.
>
> > +
> > +static bool disable_hugepages;
> > +module_param_named(disable_hugepages,
> > + disable_hugepages, bool, S_IRUGO | S_IWUSR);
> > +MODULE_PARM_DESC(disable_hugepages,
> > + "Disable VFIO IOMMU support for IOMMU hugepages.");
> > +
> > +struct vwork {
> > + struct mm_struct *mm;
> > + long npage;
> > + struct work_struct work;
> > +};
> > +
> > +/* delayed decrement/increment for locked_vm */ void
> > +vfio_lock_acct_bg(struct work_struct *work) {
> > + struct vwork *vwork = container_of(work, struct vwork, work);
> > + struct mm_struct *mm;
> > +
> > + mm = vwork->mm;
> > + down_write(&mm->mmap_sem);
> > + mm->locked_vm += vwork->npage;
> > + up_write(&mm->mmap_sem);
> > + mmput(mm);
> > + kfree(vwork);
> > +}
> > +
> > +void vfio_lock_acct(long npage)
> > +{
> > + struct vwork *vwork;
> > + struct mm_struct *mm;
> > +
> > + if (!current->mm || !npage)
> > + return; /* process exited or nothing to do */
> > +
> > + if (down_write_trylock(¤t->mm->mmap_sem)) {
> > + current->mm->locked_vm += npage;
> > + up_write(¤t->mm->mmap_sem);
> > + return;
> > + }
> > +
> > + /*
> > + * Couldn't get mmap_sem lock, so must setup to update
> > + * mm->locked_vm later. If locked_vm were atomic, we
> > + * wouldn't need this silliness
> > + */
> > + vwork = kmalloc(sizeof(struct vwork), GFP_KERNEL);
> > + if (!vwork)
> > + return;
> > + mm = get_task_mm(current);
> > + if (!mm) {
> > + kfree(vwork);
> > + return;
> > + }
> > + INIT_WORK(&vwork->work, vfio_lock_acct_bg);
> > + vwork->mm = mm;
> > + vwork->npage = npage;
> > + schedule_work(&vwork->work);
> > +}
> > +
> > +/*
> > + * Some mappings aren't backed by a struct page, for example an
> > +mmap'd
> > + * MMIO range for our own or another device. These use a different
> > + * pfn conversion and shouldn't be tracked as locked pages.
> > + */
> > +bool is_invalid_reserved_pfn(unsigned long pfn) {
> > + if (pfn_valid(pfn)) {
> > + bool reserved;
> > + struct page *tail = pfn_to_page(pfn);
> > + struct page *head = compound_trans_head(tail);
> > + reserved = !!(PageReserved(head));
> > + if (head != tail) {
> > + /*
> > + * "head" is not a dangling pointer
> > + * (compound_trans_head takes care of that)
> > + * but the hugepage may have been split
> > + * from under us (and we may not hold a
> > + * reference count on the head page so it can
> > + * be reused before we run PageReferenced), so
> > + * we've to check PageTail before returning
> > + * what we just read.
> > + */
> > + smp_rmb();
> > + if (PageTail(tail))
> > + return reserved;
> > + }
> > + return PageReserved(tail);
> > + }
> > +
> > + return true;
> > +}
> > +
> > +int put_pfn(unsigned long pfn, int prot) {
> > + if (!is_invalid_reserved_pfn(pfn)) {
> > + struct page *page = pfn_to_page(pfn);
> > + if (prot & IOMMU_WRITE)
> > + SetPageDirty(page);
> > + put_page(page);
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > +static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long
> > +*pfn) {
> > + struct page *page[1];
> > + struct vm_area_struct *vma;
> > + int ret = -EFAULT;
> > +
> > + if (get_user_pages_fast(vaddr, 1, !!(prot & IOMMU_WRITE), page) == 1) {
> > + *pfn = page_to_pfn(page[0]);
> > + return 0;
> > + }
> > +
> > + printk("via vma\n");
> > + down_read(¤t->mm->mmap_sem);
> > +
> > + vma = find_vma_intersection(current->mm, vaddr, vaddr + 1);
> > +
> > + if (vma && vma->vm_flags & VM_PFNMAP) {
> > + *pfn = ((vaddr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> > + if (is_invalid_reserved_pfn(*pfn))
> > + ret = 0;
> > + }
> > +
> > + up_read(¤t->mm->mmap_sem);
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * Attempt to pin pages. We really don't want to track all the pfns
> > +and
> > + * the iommu can only map chunks of consecutive pfns anyway, so get
> > +the
> > + * first page and all consecutive pages with the same locking.
> > + */
> > +long vfio_pin_pages(unsigned long vaddr, long npage,
> > + int prot, unsigned long *pfn_base) {
> > + unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> > + bool lock_cap = capable(CAP_IPC_LOCK);
> > + long ret, i;
> > +
> > + if (!current->mm)
> > + return -ENODEV;
> > +
> > + ret = vaddr_get_pfn(vaddr, prot, pfn_base);
> > + if (ret)
> > + return ret;
> > +
> > + if (is_invalid_reserved_pfn(*pfn_base))
> > + return 1;
> > +
> > + if (!lock_cap && current->mm->locked_vm + 1 > limit) {
> > + put_pfn(*pfn_base, prot);
> > + pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__,
> > + limit << PAGE_SHIFT);
> > + return -ENOMEM;
> > + }
> > +
> > + if (unlikely(disable_hugepages)) {
> > + vfio_lock_acct(1);
> > + return 1;
> > + }
> > +
> > + /* Lock all the consecutive pages from pfn_base */
> > + for (i = 1, vaddr += PAGE_SIZE; i < npage; i++, vaddr += PAGE_SIZE) {
> > + unsigned long pfn = 0;
> > +
> > + ret = vaddr_get_pfn(vaddr, prot, &pfn);
> > + if (ret)
> > + break;
> > +
> > + if (pfn != *pfn_base + i || is_invalid_reserved_pfn(pfn)) {
> > + put_pfn(pfn, prot);
> > + break;
> > + }
> > +
> > + if (!lock_cap && current->mm->locked_vm + i + 1 > limit) {
> > + put_pfn(pfn, prot);
> > + pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n",
> > + __func__, limit << PAGE_SHIFT);
> > + break;
> > + }
> > + }
> > +
> > + vfio_lock_acct(i);
> > +
> > + return i;
> > +}
> > +
> > +long vfio_unpin_pages(unsigned long pfn, long npage,
> > + int prot, bool do_accounting) {
> > + unsigned long unlocked = 0;
> > + long i;
> > +
> > + for (i = 0; i < npage; i++)
> > + unlocked += put_pfn(pfn++, prot);
> > +
> > + if (do_accounting)
> > + vfio_lock_acct(-unlocked);
> > +
> > + return unlocked;
> > +}
> > diff --git a/drivers/vfio/vfio_iommu_common.h
> > b/drivers/vfio/vfio_iommu_common.h
> > new file mode 100644
> > index 0000000..4738391
> > --- /dev/null
> > +++ b/drivers/vfio/vfio_iommu_common.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> > + * Copyright (C) 2013 Freescale Semiconductor, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify it
> > + * under the terms of the GNU General Public License version 2 as
> > +published
> > + * by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> > +02111-1307 USA */
> > +
> > +#ifndef _VFIO_IOMMU_COMMON_H
> > +#define _VFIO_IOMMU_COMMON_H
> > +
> > +void vfio_lock_acct_bg(struct work_struct *work);
>
> Does this need to be exposed?
No, will make this and other which does not need to be exposed as static to this file
Thanks
-Bharat
>
> > +void vfio_lock_acct(long npage);
> > +bool is_invalid_reserved_pfn(unsigned long pfn); int put_pfn(unsigned
> > +long pfn, int prot);
>
> Why are these exposed, they only seem to be used by functions in the new common
> file.
>
> > +long vfio_pin_pages(unsigned long vaddr, long npage, int prot,
> > + unsigned long *pfn_base);
> > +long vfio_unpin_pages(unsigned long pfn, long npage,
> > + int prot, bool do_accounting);
>
> Can we get by with just these two and vfio_lock_acct()?
>
> > +#endif
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index a9807de..e9a58fa 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -37,6 +37,7 @@
> > #include <linux/uaccess.h>
> > #include <linux/vfio.h>
> > #include <linux/workqueue.h>
> > +#include "vfio_iommu_common.h"
> >
> > #define DRIVER_VERSION "0.2"
> > #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@...hat.com>"
> > @@ -48,12 +49,6 @@ module_param_named(allow_unsafe_interrupts,
> > MODULE_PARM_DESC(allow_unsafe_interrupts,
> > "Enable VFIO IOMMU support for on platforms without interrupt
> > remapping support.");
> >
> > -static bool disable_hugepages;
> > -module_param_named(disable_hugepages,
> > - disable_hugepages, bool, S_IRUGO | S_IWUSR);
> > -MODULE_PARM_DESC(disable_hugepages,
> > - "Disable VFIO IOMMU support for IOMMU hugepages.");
> > -
> > struct vfio_iommu {
> > struct iommu_domain *domain;
> > struct mutex lock;
> > @@ -123,205 +118,6 @@ static void vfio_remove_dma(struct vfio_iommu *iommu,
> struct vfio_dma *old)
> > rb_erase(&old->node, &iommu->dma_list); }
> >
> > -struct vwork {
> > - struct mm_struct *mm;
> > - long npage;
> > - struct work_struct work;
> > -};
> > -
> > -/* delayed decrement/increment for locked_vm */ -static void
> > vfio_lock_acct_bg(struct work_struct *work) -{
> > - struct vwork *vwork = container_of(work, struct vwork, work);
> > - struct mm_struct *mm;
> > -
> > - mm = vwork->mm;
> > - down_write(&mm->mmap_sem);
> > - mm->locked_vm += vwork->npage;
> > - up_write(&mm->mmap_sem);
> > - mmput(mm);
> > - kfree(vwork);
> > -}
> > -
> > -static void vfio_lock_acct(long npage) -{
> > - struct vwork *vwork;
> > - struct mm_struct *mm;
> > -
> > - if (!current->mm || !npage)
> > - return; /* process exited or nothing to do */
> > -
> > - if (down_write_trylock(¤t->mm->mmap_sem)) {
> > - current->mm->locked_vm += npage;
> > - up_write(¤t->mm->mmap_sem);
> > - return;
> > - }
> > -
> > - /*
> > - * Couldn't get mmap_sem lock, so must setup to update
> > - * mm->locked_vm later. If locked_vm were atomic, we
> > - * wouldn't need this silliness
> > - */
> > - vwork = kmalloc(sizeof(struct vwork), GFP_KERNEL);
> > - if (!vwork)
> > - return;
> > - mm = get_task_mm(current);
> > - if (!mm) {
> > - kfree(vwork);
> > - return;
> > - }
> > - INIT_WORK(&vwork->work, vfio_lock_acct_bg);
> > - vwork->mm = mm;
> > - vwork->npage = npage;
> > - schedule_work(&vwork->work);
> > -}
> > -
> > -/*
> > - * Some mappings aren't backed by a struct page, for example an
> > mmap'd
> > - * MMIO range for our own or another device. These use a different
> > - * pfn conversion and shouldn't be tracked as locked pages.
> > - */
> > -static bool is_invalid_reserved_pfn(unsigned long pfn) -{
> > - if (pfn_valid(pfn)) {
> > - bool reserved;
> > - struct page *tail = pfn_to_page(pfn);
> > - struct page *head = compound_trans_head(tail);
> > - reserved = !!(PageReserved(head));
> > - if (head != tail) {
> > - /*
> > - * "head" is not a dangling pointer
> > - * (compound_trans_head takes care of that)
> > - * but the hugepage may have been split
> > - * from under us (and we may not hold a
> > - * reference count on the head page so it can
> > - * be reused before we run PageReferenced), so
> > - * we've to check PageTail before returning
> > - * what we just read.
> > - */
> > - smp_rmb();
> > - if (PageTail(tail))
> > - return reserved;
> > - }
> > - return PageReserved(tail);
> > - }
> > -
> > - return true;
> > -}
> > -
> > -static int put_pfn(unsigned long pfn, int prot) -{
> > - if (!is_invalid_reserved_pfn(pfn)) {
> > - struct page *page = pfn_to_page(pfn);
> > - if (prot & IOMMU_WRITE)
> > - SetPageDirty(page);
> > - put_page(page);
> > - return 1;
> > - }
> > - return 0;
> > -}
> > -
> > -static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long
> > *pfn) -{
> > - struct page *page[1];
> > - struct vm_area_struct *vma;
> > - int ret = -EFAULT;
> > -
> > - if (get_user_pages_fast(vaddr, 1, !!(prot & IOMMU_WRITE), page) == 1) {
> > - *pfn = page_to_pfn(page[0]);
> > - return 0;
> > - }
> > -
> > - down_read(¤t->mm->mmap_sem);
> > -
> > - vma = find_vma_intersection(current->mm, vaddr, vaddr + 1);
> > -
> > - if (vma && vma->vm_flags & VM_PFNMAP) {
> > - *pfn = ((vaddr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> > - if (is_invalid_reserved_pfn(*pfn))
> > - ret = 0;
> > - }
> > -
> > - up_read(¤t->mm->mmap_sem);
> > -
> > - return ret;
> > -}
> > -
> > -/*
> > - * Attempt to pin pages. We really don't want to track all the pfns
> > and
> > - * the iommu can only map chunks of consecutive pfns anyway, so get
> > the
> > - * first page and all consecutive pages with the same locking.
> > - */
> > -static long vfio_pin_pages(unsigned long vaddr, long npage,
> > - int prot, unsigned long *pfn_base)
> > -{
> > - unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> > - bool lock_cap = capable(CAP_IPC_LOCK);
> > - long ret, i;
> > -
> > - if (!current->mm)
> > - return -ENODEV;
> > -
> > - ret = vaddr_get_pfn(vaddr, prot, pfn_base);
> > - if (ret)
> > - return ret;
> > -
> > - if (is_invalid_reserved_pfn(*pfn_base))
> > - return 1;
> > -
> > - if (!lock_cap && current->mm->locked_vm + 1 > limit) {
> > - put_pfn(*pfn_base, prot);
> > - pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__,
> > - limit << PAGE_SHIFT);
> > - return -ENOMEM;
> > - }
> > -
> > - if (unlikely(disable_hugepages)) {
> > - vfio_lock_acct(1);
> > - return 1;
> > - }
> > -
> > - /* Lock all the consecutive pages from pfn_base */
> > - for (i = 1, vaddr += PAGE_SIZE; i < npage; i++, vaddr += PAGE_SIZE) {
> > - unsigned long pfn = 0;
> > -
> > - ret = vaddr_get_pfn(vaddr, prot, &pfn);
> > - if (ret)
> > - break;
> > -
> > - if (pfn != *pfn_base + i || is_invalid_reserved_pfn(pfn)) {
> > - put_pfn(pfn, prot);
> > - break;
> > - }
> > -
> > - if (!lock_cap && current->mm->locked_vm + i + 1 > limit) {
> > - put_pfn(pfn, prot);
> > - pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n",
> > - __func__, limit << PAGE_SHIFT);
> > - break;
> > - }
> > - }
> > -
> > - vfio_lock_acct(i);
> > -
> > - return i;
> > -}
> > -
> > -static long vfio_unpin_pages(unsigned long pfn, long npage,
> > - int prot, bool do_accounting)
> > -{
> > - unsigned long unlocked = 0;
> > - long i;
> > -
> > - for (i = 0; i < npage; i++)
> > - unlocked += put_pfn(pfn++, prot);
> > -
> > - if (do_accounting)
> > - vfio_lock_acct(-unlocked);
> > -
> > - return unlocked;
> > -}
> > -
> > static int vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
> > dma_addr_t iova, size_t *size) {
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body
> of a message to majordomo@...r.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists