* [PATCH 0/8] Use correctly the Xen memory terminologies in Linux
From: Julien Grall @ 2015-07-28 15:02 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, stefano.stabellini, linux-kernel, Julien Grall,
Boris Ostrovsky, David Vrabel, Dmitry Torokhov,
Greg Kroah-Hartman, H. Peter Anvin, Ingo Molnar,
James E.J. Bottomley, Jean-Christophe Plagniol-Villard,
Jiri Slaby, Juergen Gross, Konrad Rzeszutek Wilk, linux-api,
linux-arm-kernel, linux-fbdev, linux-input, linuxppc-dev,
linux-scsi, netdev, Roger Pau Monné
Hi all,
This patch series aims to use the memory terminologies described in
include/linux/mm.h [1] for Linux xen code.
Linux is using mistakenly MFN when GFN is meant, I suspect this is because the
first support of Xen was for PV. This has brought some misimplementation
of memory helpers on ARM and make the developper confused about the expected
behavior.
For instance, with pfn_to_mfn, we expect to get a MFN based on the name.
Although, if we look at the implementation on x86, it's returning a GFN.
Most of the callers are also using it this way.
The first 2 patches of this series is ARM related in order to remove
PV specific helpers which should not be used and fixing the implementation of
pfn_to_mfn.
The rest of the series is here rename most of the usage in the common code
of MFN to GFN. I also took the opportunity to replace most of the call to
pfn_to_gfn in the common code by page_to_gfn avoid construction such
as pfn_to_gfn(page_to_pfn(...).
Note the one xen-blkfront will be dropped by 64K series [2], I can include it
if necessary.
This series is based on Linux 4.2-rc4. A branch with all the patches
can be found here:
git://xenbits.xen.org/people/julieng/linux-arm.git branch page-renaming-v1
Sincerely yours,
[1] Xen tree: e758ed14f390342513405dd766e874934573e6cb
[2] https://lkml.org/lkml/2015/7/9/628
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "James E.J. Bottomley" <JBottomley@odin.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: linux-api@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-fbdev@vger.kernel.org
Cc: linux-input@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-scsi@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: x86@kernel.org
Julien Grall (8):
arm/xen: Remove helpers which are PV specific
xen: Make clear that swiotlb and biomerge are dealing with DMA address
arm/xen: implement correctly pfn_to_mfn
xen: Use the correctly the Xen memory terminologies
xen/tmem: Use page_to_gfn rather than pfn_to_gfn
video/xen-fbfront: Further s/MFN/GFN clean-up
hvc/xen: Further s/MFN/GFN clean-up
xen/privcmd: Further s/MFN/GFN/ clean-up
arch/arm/include/asm/xen/page.h | 44 +++++++++++++++---------------
arch/arm/xen/enlighten.c | 18 ++++++-------
arch/arm/xen/mm.c | 4 +--
arch/x86/include/asm/xen/page.h | 34 +++++++++++++----------
arch/x86/xen/enlighten.c | 4 +--
arch/x86/xen/mmu.c | 48 ++++++++++++++++-----------------
arch/x86/xen/p2m.c | 32 +++++++++++-----------
arch/x86/xen/setup.c | 12 ++++-----
arch/x86/xen/smp.c | 4 +--
arch/x86/xen/suspend.c | 8 +++---
drivers/block/xen-blkfront.c | 6 ++---
drivers/input/misc/xen-kbdfront.c | 4 +--
drivers/net/xen-netback/netback.c | 4 +--
drivers/net/xen-netfront.c | 8 +++---
drivers/scsi/xen-scsifront.c | 8 +++---
drivers/tty/hvc/hvc_xen.c | 18 +++++--------
drivers/video/fbdev/xen-fbfront.c | 20 +++++++-------
drivers/xen/balloon.c | 2 +-
drivers/xen/biomerge.c | 6 ++---
drivers/xen/events/events_base.c | 2 +-
drivers/xen/events/events_fifo.c | 4 +--
drivers/xen/gntalloc.c | 3 ++-
drivers/xen/manage.c | 2 +-
drivers/xen/privcmd.c | 44 +++++++++++++++---------------
drivers/xen/swiotlb-xen.c | 16 +++++------
drivers/xen/tmem.c | 21 +++++----------
drivers/xen/xenbus/xenbus_client.c | 2 +-
drivers/xen/xenbus/xenbus_dev_backend.c | 2 +-
drivers/xen/xenbus/xenbus_probe.c | 8 +++---
drivers/xen/xlate_mmu.c | 18 ++++++-------
include/uapi/xen/privcmd.h | 4 +++
include/xen/page.h | 4 +--
include/xen/xen-ops.h | 10 +++----
33 files changed, 210 insertions(+), 214 deletions(-)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 8/8] xen/privcmd: Further s/MFN/GFN/ clean-up
From: Julien Grall @ 2015-07-28 15:02 UTC (permalink / raw)
To: xen-devel
Cc: x86, Russell King, ian.campbell, linux-api, stefano.stabellini,
linux-kernel, Julien Grall, Ingo Molnar, David Vrabel,
H. Peter Anvin, Boris Ostrovsky, Thomas Gleixner,
linux-arm-kernel
In-Reply-To: <1438095769-2560-1-git-send-email-julien.grall@citrix.com>
The privcmd code is mixing the usage of GFN and MFN within the same
functions which make the code difficult to understand when you only work
with auto-translated guests.
The privcmd driver is only dealing with GFN so replace all the mention
of MFN into GFN.
The ioctl structure used to map foreign change has been left unchanged
given that the userspace is using it. Nonetheless, add a comment to
explain the expected value within the "mfn" field.
Signed-off-by: Julien Grall <julien.grall@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-api@vger.kernel.org
---
arch/arm/xen/enlighten.c | 18 +++++++++---------
arch/x86/xen/mmu.c | 32 ++++++++++++++++----------------
drivers/xen/privcmd.c | 44 ++++++++++++++++++++++----------------------
drivers/xen/xlate_mmu.c | 18 +++++++++---------
include/uapi/xen/privcmd.h | 4 ++++
include/xen/xen-ops.h | 10 +++++-----
6 files changed, 65 insertions(+), 61 deletions(-)
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 6c09cc4..a8b8806 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -56,35 +56,35 @@ static __read_mostly unsigned int xen_events_irq;
static __initdata struct device_node *xen_node;
-int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
+int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
unsigned long addr,
- xen_pfn_t *mfn, int nr,
+ xen_pfn_t *gfn, int nr,
int *err_ptr, pgprot_t prot,
unsigned domid,
struct page **pages)
{
- return xen_xlate_remap_gfn_array(vma, addr, mfn, nr, err_ptr,
+ return xen_xlate_remap_gfn_array(vma, addr, gfn, nr, err_ptr,
prot, domid, pages);
}
-EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_array);
+EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_array);
/* Not used by XENFEAT_auto_translated guests. */
-int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
+int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
unsigned long addr,
- xen_pfn_t mfn, int nr,
+ xen_pfn_t gfn, int nr,
pgprot_t prot, unsigned domid,
struct page **pages)
{
return -ENOSYS;
}
-EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
+EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
-int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
+int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
int nr, struct page **pages)
{
return xen_xlate_unmap_gfn_range(vma, nr, pages);
}
-EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
+EXPORT_SYMBOL_GPL(xen_unmap_domain_gfn_range);
static void xen_percpu_init(void)
{
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 742b8d2..ca5c71f 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -2465,9 +2465,9 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, pgtable_t token,
return 0;
}
-static int do_remap_mfn(struct vm_area_struct *vma,
+static int do_remap_gfn(struct vm_area_struct *vma,
unsigned long addr,
- xen_pfn_t *mfn, int nr,
+ xen_pfn_t *gfn, int nr,
int *err_ptr, pgprot_t prot,
unsigned domid,
struct page **pages)
@@ -2483,14 +2483,14 @@ static int do_remap_mfn(struct vm_area_struct *vma,
if (xen_feature(XENFEAT_auto_translated_physmap)) {
#ifdef CONFIG_XEN_PVH
/* We need to update the local page tables and the xen HAP */
- return xen_xlate_remap_gfn_array(vma, addr, mfn, nr, err_ptr,
+ return xen_xlate_remap_gfn_array(vma, addr, gfn, nr, err_ptr,
prot, domid, pages);
#else
return -EINVAL;
#endif
}
- rmd.mfn = mfn;
+ rmd.mfn = gfn;
rmd.prot = prot;
/* We use the err_ptr to indicate if there we are doing a contigious
* mapping or a discontigious mapping. */
@@ -2518,8 +2518,8 @@ static int do_remap_mfn(struct vm_area_struct *vma,
batch_left, &done, domid);
/*
- * @err_ptr may be the same buffer as @mfn, so
- * only clear it after each chunk of @mfn is
+ * @err_ptr may be the same buffer as @gfn, so
+ * only clear it after each chunk of @gfn is
* used.
*/
if (err_ptr) {
@@ -2549,19 +2549,19 @@ out:
return err < 0 ? err : mapped;
}
-int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
+int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
unsigned long addr,
- xen_pfn_t mfn, int nr,
+ xen_pfn_t gfn, int nr,
pgprot_t prot, unsigned domid,
struct page **pages)
{
- return do_remap_mfn(vma, addr, &mfn, nr, NULL, prot, domid, pages);
+ return do_remap_gfn(vma, addr, &gfn, nr, NULL, prot, domid, pages);
}
-EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
+EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_range);
-int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
+int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
unsigned long addr,
- xen_pfn_t *mfn, int nr,
+ xen_pfn_t *gfn, int nr,
int *err_ptr, pgprot_t prot,
unsigned domid, struct page **pages)
{
@@ -2570,13 +2570,13 @@ int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
* cause of "wrong memory was mapped in".
*/
BUG_ON(err_ptr == NULL);
- return do_remap_mfn(vma, addr, mfn, nr, err_ptr, prot, domid, pages);
+ return do_remap_gfn(vma, addr, gfn, nr, err_ptr, prot, domid, pages);
}
-EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_array);
+EXPORT_SYMBOL_GPL(xen_remap_domain_gfn_array);
/* Returns: 0 success */
-int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
+int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
int numpgs, struct page **pages)
{
if (!pages || !xen_feature(XENFEAT_auto_translated_physmap))
@@ -2588,4 +2588,4 @@ int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
return -EINVAL;
#endif
}
-EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
+EXPORT_SYMBOL_GPL(xen_unmap_domain_gfn_range);
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 5a29616..c6deb87 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -193,16 +193,16 @@ static int traverse_pages_block(unsigned nelem, size_t size,
return ret;
}
-struct mmap_mfn_state {
+struct mmap_gfn_state {
unsigned long va;
struct vm_area_struct *vma;
domid_t domain;
};
-static int mmap_mfn_range(void *data, void *state)
+static int mmap_gfn_range(void *data, void *state)
{
struct privcmd_mmap_entry *msg = data;
- struct mmap_mfn_state *st = state;
+ struct mmap_gfn_state *st = state;
struct vm_area_struct *vma = st->vma;
int rc;
@@ -216,7 +216,7 @@ static int mmap_mfn_range(void *data, void *state)
((msg->va+(msg->npages<<PAGE_SHIFT)) > vma->vm_end))
return -EINVAL;
- rc = xen_remap_domain_mfn_range(vma,
+ rc = xen_remap_domain_gfn_range(vma,
msg->va & PAGE_MASK,
msg->mfn, msg->npages,
vma->vm_page_prot,
@@ -236,7 +236,7 @@ static long privcmd_ioctl_mmap(void __user *udata)
struct vm_area_struct *vma;
int rc;
LIST_HEAD(pagelist);
- struct mmap_mfn_state state;
+ struct mmap_gfn_state state;
/* We only support privcmd_ioctl_mmap_batch for auto translated. */
if (xen_feature(XENFEAT_auto_translated_physmap))
@@ -273,7 +273,7 @@ static long privcmd_ioctl_mmap(void __user *udata)
rc = traverse_pages(mmapcmd.num, sizeof(struct privcmd_mmap_entry),
&pagelist,
- mmap_mfn_range, &state);
+ mmap_gfn_range, &state);
out_up:
@@ -299,18 +299,18 @@ struct mmap_batch_state {
int global_error;
int version;
- /* User-space mfn array to store errors in the second pass for V1. */
- xen_pfn_t __user *user_mfn;
+ /* User-space gfn array to store errors in the second pass for V1. */
+ xen_pfn_t __user *user_gfn;
/* User-space int array to store errors in the second pass for V2. */
int __user *user_err;
};
-/* auto translated dom0 note: if domU being created is PV, then mfn is
- * mfn(addr on bus). If it's auto xlated, then mfn is pfn (input to HAP).
+/* auto translated dom0 note: if domU being created is PV, then gfn is
+ * mfn(addr on bus). If it's auto xlated, then gfn is pfn (input to HAP).
*/
static int mmap_batch_fn(void *data, int nr, void *state)
{
- xen_pfn_t *mfnp = data;
+ xen_pfn_t *gfnp = data;
struct mmap_batch_state *st = state;
struct vm_area_struct *vma = st->vma;
struct page **pages = vma->vm_private_data;
@@ -321,8 +321,8 @@ static int mmap_batch_fn(void *data, int nr, void *state)
cur_pages = &pages[st->index];
BUG_ON(nr < 0);
- ret = xen_remap_domain_mfn_array(st->vma, st->va & PAGE_MASK, mfnp, nr,
- (int *)mfnp, st->vma->vm_page_prot,
+ ret = xen_remap_domain_gfn_array(st->vma, st->va & PAGE_MASK, gfnp, nr,
+ (int *)gfnp, st->vma->vm_page_prot,
st->domain, cur_pages);
/* Adjust the global_error? */
@@ -347,22 +347,22 @@ static int mmap_return_error(int err, struct mmap_batch_state *st)
if (st->version == 1) {
if (err) {
- xen_pfn_t mfn;
+ xen_pfn_t gfn;
- ret = get_user(mfn, st->user_mfn);
+ ret = get_user(gfn, st->user_gfn);
if (ret < 0)
return ret;
/*
* V1 encodes the error codes in the 32bit top
- * nibble of the mfn (with its known
+ * nibble of the gfn (with its known
* limitations vis-a-vis 64 bit callers).
*/
- mfn |= (err == -ENOENT) ?
+ gfn |= (err == -ENOENT) ?
PRIVCMD_MMAPBATCH_PAGED_ERROR :
PRIVCMD_MMAPBATCH_MFN_ERROR;
- return __put_user(mfn, st->user_mfn++);
+ return __put_user(gfn, st->user_gfn++);
} else
- st->user_mfn++;
+ st->user_gfn++;
} else { /* st->version == 2 */
if (err)
return __put_user(err, st->user_err++);
@@ -388,7 +388,7 @@ static int mmap_return_errors(void *data, int nr, void *state)
return 0;
}
-/* Allocate pfns that are then mapped with gmfns from foreign domid. Update
+/* Allocate pfns that are then mapped with gfns from foreign domid. Update
* the vma with the page info to use later.
* Returns: 0 if success, otherwise -errno
*/
@@ -526,7 +526,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
if (state.global_error) {
/* Write back errors in second pass. */
- state.user_mfn = (xen_pfn_t *)m.arr;
+ state.user_gfn = (xen_pfn_t *)m.arr;
state.user_err = m.err;
ret = traverse_pages_block(m.num, sizeof(xen_pfn_t),
&pagelist, mmap_return_errors, &state);
@@ -587,7 +587,7 @@ static void privcmd_close(struct vm_area_struct *vma)
if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)
return;
- rc = xen_unmap_domain_mfn_range(vma, numpgs, pages);
+ rc = xen_unmap_domain_gfn_range(vma, numpgs, pages);
if (rc == 0)
free_xenballooned_pages(numpgs, pages);
else
diff --git a/drivers/xen/xlate_mmu.c b/drivers/xen/xlate_mmu.c
index 58a5389..cff2387 100644
--- a/drivers/xen/xlate_mmu.c
+++ b/drivers/xen/xlate_mmu.c
@@ -38,8 +38,8 @@
#include <xen/interface/xen.h>
#include <xen/interface/memory.h>
-/* map fgmfn of domid to lpfn in the current domain */
-static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
+/* map fgfn of domid to lpfn in the current domain */
+static int map_foreign_page(unsigned long lpfn, unsigned long fgfn,
unsigned int domid)
{
int rc;
@@ -49,7 +49,7 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
.size = 1,
.space = XENMAPSPACE_gmfn_foreign,
};
- xen_ulong_t idx = fgmfn;
+ xen_ulong_t idx = fgfn;
xen_pfn_t gpfn = lpfn;
int err = 0;
@@ -62,13 +62,13 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
}
struct remap_data {
- xen_pfn_t *fgmfn; /* foreign domain's gmfn */
+ xen_pfn_t *fgfn; /* foreign domain's gfn */
pgprot_t prot;
domid_t domid;
struct vm_area_struct *vma;
int index;
struct page **pages;
- struct xen_remap_mfn_info *info;
+ struct xen_remap_gfn_info *info;
int *err_ptr;
int mapped;
};
@@ -82,20 +82,20 @@ static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
pte_t pte = pte_mkspecial(pfn_pte(pfn, info->prot));
int rc;
- rc = map_foreign_page(pfn, *info->fgmfn, info->domid);
+ rc = map_foreign_page(pfn, *info->fgfn, info->domid);
*info->err_ptr++ = rc;
if (!rc) {
set_pte_at(info->vma->vm_mm, addr, ptep, pte);
info->mapped++;
}
- info->fgmfn++;
+ info->fgfn++;
return 0;
}
int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
unsigned long addr,
- xen_pfn_t *mfn, int nr,
+ xen_pfn_t *gfn, int nr,
int *err_ptr, pgprot_t prot,
unsigned domid,
struct page **pages)
@@ -108,7 +108,7 @@ int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
x86 PVOPS */
BUG_ON(!((vma->vm_flags & (VM_PFNMAP | VM_IO)) == (VM_PFNMAP | VM_IO)));
- data.fgmfn = mfn;
+ data.fgfn = gfn;
data.prot = prot;
data.domid = domid;
data.vma = vma;
diff --git a/include/uapi/xen/privcmd.h b/include/uapi/xen/privcmd.h
index a853168..7ddeeda 100644
--- a/include/uapi/xen/privcmd.h
+++ b/include/uapi/xen/privcmd.h
@@ -44,6 +44,10 @@ struct privcmd_hypercall {
struct privcmd_mmap_entry {
__u64 va;
+ /*
+ * This should be a GFN. It's not possible to change the name because
+ * it's exposed to the user-space.
+ */
__u64 mfn;
__u64 npages;
};
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 0ce4f32..e4e214a 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -30,7 +30,7 @@ void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order);
struct vm_area_struct;
/*
- * xen_remap_domain_mfn_array() - map an array of foreign frames
+ * xen_remap_domain_gfn_array() - map an array of foreign frames
* @vma: VMA to map the pages into
* @addr: Address at which to map the pages
* @gfn: Array of GFNs to map
@@ -46,14 +46,14 @@ struct vm_area_struct;
* Returns the number of successfully mapped frames, or a -ve error
* code.
*/
-int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
+int xen_remap_domain_gfn_array(struct vm_area_struct *vma,
unsigned long addr,
xen_pfn_t *gfn, int nr,
int *err_ptr, pgprot_t prot,
unsigned domid,
struct page **pages);
-/* xen_remap_domain_mfn_range() - map a range of foreign frames
+/* xen_remap_domain_gfn_range() - map a range of foreign frames
* @vma: VMA to map the pages into
* @addr: Address at which to map the pages
* @gfn: First GFN to map.
@@ -65,12 +65,12 @@ int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
* Returns the number of successfully mapped frames, or a -ve error
* code.
*/
-int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
+int xen_remap_domain_gfn_range(struct vm_area_struct *vma,
unsigned long addr,
xen_pfn_t gfn, int nr,
pgprot_t prot, unsigned domid,
struct page **pages);
-int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
+int xen_unmap_domain_gfn_range(struct vm_area_struct *vma,
int numpgs, struct page **pages);
int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
unsigned long addr,
--
2.1.4
^ permalink raw reply related
* Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
From: Vlastimil Babka @ 2015-07-28 15:10 UTC (permalink / raw)
To: Eric B Munson, Michal Hocko
Cc: Andrew Morton, Shuah Khan, Michael Kerrisk, Jonathan Corbet,
Ralf Baechle, linux-alpha, linux-kernel, linux-mips, linux-parisc,
linuxppc-dev, sparclinux, linux-xtensa, linux-mm, linux-arch,
linux-api
In-Reply-To: <20150728134942.GB2407@akamai.com>
On 07/28/2015 03:49 PM, Eric B Munson wrote:
> On Tue, 28 Jul 2015, Michal Hocko wrote:
>
[...]
> The only
> remaining question I have is should we have 2 new mlockall flags so that
> the caller can explicitly set VM_LOCKONFAULT in the mm->def_flags vs
> locking all current VMAs on fault. I ask because if the user wants to
> lock all current VMAs the old way, but all future VMAs on fault they
> have to call mlockall() twice:
>
> mlockall(MCL_CURRENT);
> mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT);
>
> This has the side effect of converting all the current VMAs to
> VM_LOCKONFAULT, but because they were all made present and locked in the
> first call, this should not matter in most cases.
Shouldn't the user be able to do this?
mlockall(MCL_CURRENT)
mlockall(MCL_FUTURE | MCL_ONFAULT);
Note that the second call shouldn't change (i.e. munlock) existing vma's
just because MCL_CURRENT is not present. The current implementation
doesn't do that thanks to the following in do_mlockall():
if (flags == MCL_FUTURE)
goto out;
before current vma's are processed and MCL_CURRENT is checked. This is
probably so that do_mlockall() can also handle the munlockall() syscall.
So we should be careful not to break this, but otherwise there are no
limitations by not having two MCL_ONFAULT flags. Having to do invoke
syscalls instead of one is not an issue as this shouldn't be frequent
syscall.
> The catch is that,
> like mmap(MAP_LOCKED), mlockall() does not communicate if mm_populate()
> fails. This has been true of mlockall() from the beginning so I don't
> know if it needs more than an entry in the man page to clarify (which I
> will add when I add documentation for MCL_ONFAULT).
Good point.
> In a much less
> likely corner case, it is not possible in the current setup to request
> all current VMAs be VM_LOCKONFAULT and all future be VM_LOCKED.
So again this should work:
mlockall(MCL_CURRENT | MCL_ONFAULT)
mlockall(MCL_FUTURE);
But the order matters here, as current implementation of do_mlockall()
will clear VM_LOCKED from def_flags if MCL_FUTURE is not passed. So
*it's different* from how it handles MCL_CURRENT (as explained above).
And not documented in manpage. Oh crap, this API is a closet full of
skeletons. Maybe it was an unnoticed regression and we can restore some
sanity?
^ permalink raw reply
* Re: [PATCH RFC 0/1] UAPI,x86: export syscall numbers for all x86 archs
From: David Drysdale @ 2015-07-28 15:32 UTC (permalink / raw)
To: Paul Moore
Cc: X86 ML, Thomas Gleixner, Ingo Molnar, Andy Lutomirski,
H. Peter Anvin, Michael Kerrisk, Kees Cook, Eric Paris, Linux API,
linux-kernel@vger.kernel.org
In-Reply-To: <CAHC9VhRf8b7LRyG1N0t11jQdDbAa_m6-pYjnyb2EHxU0YoJmLg@mail.gmail.com>
On Tue, Jul 28, 2015 at 1:20 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jul 28, 2015 at 4:05 AM, David Drysdale <drysdale@google.com> wrote:
>> A while ago I was trying to build a seccomp-bpf filter program that would
>> survive a change of x86 architecture. This was complicated for all sorts of
>> reasons, but one of the problems was that the different syscall numbers aren't
>> all available at the same time -- hence this patch.
>
> Or just use libseccomp and let it take care of all the different ABI
> specific warts for you. The library handles the undefined syscalls
> you describe, but also multiplexed syscalls (e.g. socket related
> syscalls on x86) and proper invalid arch/ABI filtering
Ah, I hadn't realized that libseccomp handled cross-architecture
stuff and the socketcall multiplexing -- very neat. I'll look into whether
I can convert my stuff to use it.
I still think exporting all the sub-arch syscall numbers is a good idea
though (even if my need for it is potentially reduced by libseccomp)...
> (you are
> filtering x32 correctly on x86-64 right?).
Yep, I think so, but it's fiddly. If I can leave the fiddliness
to libseccomp, so much the better...
Thanks for the pointer,
David
> * https://github.com/seccomp/libseccomp
>
> --
> paul moore
> www.paul-moore.com
^ permalink raw reply
* Re: [PATCH RFC 0/1] UAPI,x86: export syscall numbers for all x86 archs
From: Paul Moore @ 2015-07-28 15:52 UTC (permalink / raw)
To: David Drysdale
Cc: X86 ML, Thomas Gleixner, Ingo Molnar, Andy Lutomirski,
H. Peter Anvin, Michael Kerrisk, Kees Cook, Eric Paris, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAHse=S8dN+e6nhSrf=hVLsfGyrR191mrxj_wc_OauqNtUXxSCg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Jul 28, 2015 at 11:32 AM, David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, Jul 28, 2015 at 1:20 PM, Paul Moore <paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org> wrote:
>> On Tue, Jul 28, 2015 at 4:05 AM, David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>> A while ago I was trying to build a seccomp-bpf filter program that would
>>> survive a change of x86 architecture. This was complicated for all sorts of
>>> reasons, but one of the problems was that the different syscall numbers aren't
>>> all available at the same time -- hence this patch.
>>
>> Or just use libseccomp and let it take care of all the different ABI
>> specific warts for you. The library handles the undefined syscalls
>> you describe, but also multiplexed syscalls (e.g. socket related
>> syscalls on x86) and proper invalid arch/ABI filtering
>
> Ah, I hadn't realized that libseccomp handled cross-architecture
> stuff and the socketcall multiplexing -- very neat. I'll look into whether
> I can convert my stuff to use it.
[Ooops, forgot to hit reply-all]
It should be pretty easy; if you've been writing BPF assembly, simply
making a few function calls should be a no-brainer.
We've got man pages for each of the libseccomp APIs that should cover
most of your questions, but there is also a collection of tests (see
the "tests/" directory) which serve as reasonable examples too. If
all else fails, you can always ask for help on our mailing list:
* https://groups.google.com/d/forum/libseccomp
> I still think exporting all the sub-arch syscall numbers is a good idea
> though (even if my need for it is potentially reduced by libseccomp)...
No real argument against it from me. I just worry that some
developers accidently get the seccomp-bpf filters wrong when they do
it by hand, e.g. ABI specific filters and not properly handling x32 on
x86-64.
>> (you are filtering x32 correctly on x86-64 right?).
>
> Yep, I think so, but it's fiddly. If I can leave the fiddliness
> to libseccomp, so much the better...
Annoyingly fiddly. If we could do it over I would much prefer to see
x32 get its own 32-bit ABI token value; sharing a value with x86-64
makes things harder than they need to be, but sadly it is too late to
change it now.
> Thanks for the pointer,
> David
>
>> * https://github.com/seccomp/libseccomp
No problem, let me know if you run into any problems.
Good luck!
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH RFC 0/1] Document how to add a new syscall
From: David Drysdale @ 2015-07-28 16:02 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Shuah Khan, Linux API, Michael Kerrisk, Andrew Morton,
Arnd Bergmann, Jonathan Corbet, Andrea Arcangeli, Thomas Gleixner,
Eric B Munson, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Andy Lutomirski, Al Viro,
Rusty Russell, Vivek Goyal, Alexei Starovoitov, David Herrmann,
Theodore Ts'o, Kees Cook, Miklos Szeredi, Milosz
In-Reply-To: <20150728141920.GC19282@twins.programming.kicks-ass.net>
On Tue, Jul 28, 2015 at 3:19 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jul 28, 2015 at 07:59:16AM -0600, Shuah Khan wrote:
>> On 07/28/2015 05:41 AM, David Drysdale wrote:
>> > Given that I've gotten some of the details wrong in the past (and I've
>> > seen others do likewise), I thought it might be helpful to collate the
>> > best practices for adding a new system call to the kernel.
>> >
>> > Apologies for the wide circulation -- I've tried to include folk who've
>> > recently added or proposed a system call, as they're most likely to
>> > have opinions on:
>> > - whether this a useful addition to Documentation/
>> > - whether the details of the advice are correct and complete.
>> >
>> > Shuah, is there anything more that should be added for the Testing
>> > section in particular?
>> >
>>
>> David,
>>
>> Looks good. You could add a one liner on in some cases, it might be
>> necessary to install headers before test compiles. Other than that,
>> the information looks good.
>
> For perf we still use the syscall() wrapper and we have hardcoded
> (fallback) syscall numbers in there in case they've not yet reached
> unistd.h.
>
> People update their kernels far more often than their userspace headers.
How about:
diff --git a/Documentation/adding-syscalls.txt
b/Documentation/adding-syscalls.txt
index 5f52edda8951..b274c3d01edb 100644
--- a/Documentation/adding-syscalls.txt
+++ b/Documentation/adding-syscalls.txt
@@ -389,6 +389,11 @@ reviewers with a demonstration of how user space
programs will use the system
call. A good way to combine these aims is to include a simple self-test
program in a new directory under tools/testing/selftests/.
+For a new system call, there will obviously be no libc wrapper function and so
+the test will need to invoke it using syscall(); also, if the system call
+involves a new userspace-visible structure, the corresponding header will need
+to be installed to compile the test.
+
Make sure the selftest runs successfully on all supported architectures. For
example, check that it works when compiled as an x86_64 (-m64), x86_32 (-m32)
and x32 (-mx32) ABI program.
^ permalink raw reply related
* Re: [PATCH RFC 0/1] Document how to add a new syscall
From: Shuah Khan @ 2015-07-28 16:41 UTC (permalink / raw)
To: David Drysdale, Peter Zijlstra
Cc: Linux API, Michael Kerrisk, Andrew Morton, Arnd Bergmann,
Jonathan Corbet, Andrea Arcangeli, Thomas Gleixner, Eric B Munson,
Ingo Molnar, H. Peter Anvin, Oleg Nesterov, Linus Torvalds,
Greg Kroah-Hartman, Andy Lutomirski, Al Viro, Rusty Russell,
Vivek Goyal, Alexei Starovoitov, David Herrmann,
Theodore Ts'o, Kees Cook, Miklos Szeredi, Milosz Tanski,
Fam Zheng <famz@
In-Reply-To: <CAHse=S_h8c7Sg56V1UR2uW-0hqAMopqvEyvtdOx0hUH2c+8y2w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 07/28/2015 10:02 AM, David Drysdale wrote:
> On Tue, Jul 28, 2015 at 3:19 PM, Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
>> On Tue, Jul 28, 2015 at 07:59:16AM -0600, Shuah Khan wrote:
>>> On 07/28/2015 05:41 AM, David Drysdale wrote:
>>>> Given that I've gotten some of the details wrong in the past (and I've
>>>> seen others do likewise), I thought it might be helpful to collate the
>>>> best practices for adding a new system call to the kernel.
>>>>
>>>> Apologies for the wide circulation -- I've tried to include folk who've
>>>> recently added or proposed a system call, as they're most likely to
>>>> have opinions on:
>>>> - whether this a useful addition to Documentation/
>>>> - whether the details of the advice are correct and complete.
>>>>
>>>> Shuah, is there anything more that should be added for the Testing
>>>> section in particular?
>>>>
>>>
>>> David,
>>>
>>> Looks good. You could add a one liner on in some cases, it might be
>>> necessary to install headers before test compiles. Other than that,
>>> the information looks good.
>>
>> For perf we still use the syscall() wrapper and we have hardcoded
>> (fallback) syscall numbers in there in case they've not yet reached
>> unistd.h.
>>
>> People update their kernels far more often than their userspace headers.
>
> How about:
>
> diff --git a/Documentation/adding-syscalls.txt
> b/Documentation/adding-syscalls.txt
> index 5f52edda8951..b274c3d01edb 100644
> --- a/Documentation/adding-syscalls.txt
> +++ b/Documentation/adding-syscalls.txt
> @@ -389,6 +389,11 @@ reviewers with a demonstration of how user space
> programs will use the system
> call. A good way to combine these aims is to include a simple self-test
> program in a new directory under tools/testing/selftests/.
>
> +For a new system call, there will obviously be no libc wrapper function and so
> +the test will need to invoke it using syscall(); also, if the system call
> +involves a new userspace-visible structure, the corresponding header will need
> +to be installed to compile the test.
> +
This looks good.
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org | (970) 217-8978
^ permalink raw reply
* Re: [PATCH RFC 1/1] Documentation: describe how to add a system call
From: Kees Cook @ 2015-07-28 16:43 UTC (permalink / raw)
To: David Drysdale
Cc: Linux API, Michael Kerrisk, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Andrea Arcangeli, Thomas Gleixner,
Eric B Munson, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Andy Lutomirski, Al Viro,
Rusty Russell, Peter Zijlstra, Vivek Goyal, Alexei Starovoitov,
David Herrmann, Theodore Ts'o, Miklos Szeredi, Milosz
In-Reply-To: <1438083663-24814-2-git-send-email-drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On Tue, Jul 28, 2015 at 4:41 AM, David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> Add a document describing the process of adding a new system call,
> including the need for a flags argument for future compatibility, and
> covering 32-bit/64-bit concerns (albeit in an x86-centric way).
>
> Signed-off-by: David Drysdale <drysdale-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
This is great!
Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
I have a few minor suggestions below...
>
> ---
> Documentation/adding-syscalls.txt | 454 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 454 insertions(+)
> create mode 100644 Documentation/adding-syscalls.txt
>
> diff --git a/Documentation/adding-syscalls.txt b/Documentation/adding-syscalls.txt
> new file mode 100644
> index 000000000000..5f52edda8951
> --- /dev/null
> +++ b/Documentation/adding-syscalls.txt
> @@ -0,0 +1,454 @@
> +Adding a New System Call
> +========================
> +
> +This document describes what's involved in adding a new system call to the
> +Linux kernel, over and above the normal submission advice in
> +Documentation/SubmittingPatches.
> +
> +
> +System Call Alternatives
> +------------------------
> +
> +The first thing to consider when adding a new system call is whether one of
> +the alternatives might be suitable instead. Although system calls are the
> +most traditional and most obvious interaction points between userspace and the
> +kernel, there are other possibilities -- choose what fits best for your
> +interface.
> +
> + - If the operations involved can be made to look like a filesystem-like
> + object, it may make more sense to create a new filesystem or device. This
> + also makes it easier to encapsulate the new functionality in a kernel module
> + rather than requiring it to be built into the main kernel.
> + - If the new functionality involves operations where the kernel notifies
> + userspace that something has happened, then returning a new file
> + descriptor for the relevant object allows userspace to use
> + poll/select/epoll to receive that notification.
> + - However, operations that don't map to read(2)/write(2)-like operations
> + have to be implemented as ioctl(2) requests, which can lead to a
> + somewhat opaque API.
> + - If you're just exposing runtime system information, a new node in sysfs
> + (see Documentation/filesystems/sysfs.txt) or the /proc filesystem may be
> + more appropriate. However, access to these mechanisms requires that the
> + relevant filesystem is mounted, which might not always be the case (e.g.
> + in a namespaced/sandboxed/chrooted environment).
I would add "Avoid adding anything to debugfs, as this is not
considered a 'production' interface to userspace."
> + - If the operation is specific to a particular file or file descriptor, then
> + an additional fcntl(2) command option may be more appropriate. However,
> + fcntl(2) is a multiplexing system call that hides a lot of complexity, so
> + this option is best for when the new function is closely analogous to
> + existing fcntl(2) functionality, or the new functionality is very simple
> + (for example, getting/setting a simple flag related to a file descriptor).
> + - If the operation is specific to a particular task or process, then an
> + additional prctl(2) command option may be more appropriate. As with
> + fcntl(2), this system call is a complicated multiplexor so is best reserved
> + for near-analogs of existing prctl() commands or getting/setting a simple
> + flag related to a process.
> +
> +
> +Designing the API
> +-----------------
> +
> +A new system call forms part of the API of the kernel, and has to be supported
> +indefinitely. As such, it's a very good idea to explicitly discuss the
> +interface on the kernel mailing list, and to plan for future extensions of the
> +interface. In particular:
> +
> + **Include a flags argument for every new system call**
> +
> +The syscall table is littered with historical examples where this wasn't done,
> +together with the corresponding follow-up system calls (eventfd/eventfd2,
> +dup2/dup3, inotify_init/inotify_init1, pipe/pipe2, renameat/renameat2), so
> +learn from the history of the kernel and include a flags argument from the
> +start.
> +
> +Also, to make sure that userspace programs can safely use flags between kernel
> +versions, check whether the flags value holds any unknown flags, and reject the
> +sycall (with EINVAL) if it does:
> +
> + if (flags & ~(THING_FLAG1 | THING_FLAG2 | THING_FLAG3))
> + return -EINVAL;
Perhaps mention, "Even if no flags will be used, make sure the flags
argument is zero."
> +
> +If your new xyzzy(2) system call returns a new file descriptor, then the flags
> +argument should include a value that is equivalent to setting O_CLOEXEC on the
> +new FD. This makes it possible for userspace to close the timing window
> +between xyzzy() and calling fcntl(fd, F_SETFD, FD_CLOEXEC), where an
> +unexpected fork() and execve() in another thread could leak a descriptor to
> +the exec'ed program. (However, resist the temptation to re-use the actual value
> +of the O_CLOEXEC constant, as it is architecture-specific and is part of a
> +numbering space of O_* flags that is fairly full.)
> +
> +If your new xyzzy(2) system call involves a filename argument:
> +
> + int sys_xyzzy(const char __user *path, ..., unsigned int flags);
> +
> +you should also consider whether an xyzzyat(2) version is more appropriate:
> +
> + int sys_xyzzyat(int dfd, const char __user *path, ..., unsigned int flags);
> +
> +This allows more flexibility for how userspace specifies the file in question;
> +in particular it allows userspace to request the functionality for an
> +already-opened file descriptor using the AT_EMPTY_PATH flag, effectively giving
> +an fxyzzy(3) operation for free:
> +
> + - xyzzyat(AT_FDCWD, path, ..., 0) is equivalent to xyzzy(path,...)
> + - xyzzyat(fd, "", ..., AT_EMPTY_PATH) is equivalent to fxyzzy(fd, ...)
> +
> +(For more details on the rationale of the *at() calls, see the openat(2) man
> +page; for an example of AT_EMPTY_PATH, see the statat(2) man page.)
> +
> +If your new xyzzy(2) system call involves a parameter describing an offset
> +within a file, make its type loff_t so that 64-bit offsets can be supported
> +even on 32-bit architectures.
> +
> +If your new xyzzy(2) system call involves administrative functionality, it
> +needs to be governed by the appropriate Linux capability bit, as described in
Maybe add:
... capability bit, and checked with a call to capable(), as described ...
> +the capabilities(7) man page.
> +
> + - If there is an existing capability that governs related functionality, then
> + use that. However, avoid combining lots of only vaguely related functions
> + together under the same bit, as this goes against capabilities' purpose of
> + splitting the power of root. In particular, avoid adding new uses of the
> + already overly-general CAP_SYS_ADMIN capability.
> + - If there is no related capability, then consider adding a new capability
> + bit -- but bear in mind that the numbering space is limited, and each new
> + bit needs to be understood and administered by sysadmins.
Perhaps mention alternative mechanisms for access control when working
on file descriptors, like avoiding security issues by looking at fd
_opener_ credentials, rather than current's credentials?
> +
> +Finally, be aware that some non-x86 architectures have an easier time if
> +system call parameters that are explicitly 64-bit fall on odd-numbered
> +arguments (i.e. parameter 1, 3, 5), to allow use of contiguous pairs of 32-bit
> +registers.
> +
> +
> +Proposing the API
> +-----------------
> +
> +To make new system calls easy to review, it's best to divide up the patchset
> +into separate chunks. These should include at least the following items as
> +distinct commits (each of which is described further below):
> +
> + - The core implementation of the system call together with prototypes, generic
> + numbering and fallback stub implementation.
> + - Wiring up of the new system call for one particular architecture, usually
> + x86 (including all of x86_64, x86_32 and x32).
> + - A demonstration of the use of the new system call in userspace via a
> + selftest.
While you call out the path later, I think it could use being explicit here too:
" ... a selftest in tools/testing/selftests/."
> + - A draft man-page for the new system call.
> +
> +Changes to the kernel's API should always be cc'ed to linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
I would strengthen this as:
New syscall proposals, like any change to the kernel's API, should
always be cc'ed...
> +
> +
> +Generic System Call Implementation
> +----------------------------------
> +
> +The main entry point for your new xyzzy(2) system call will be called
> +sys_xyzzy(), but you add this entry point with the appropriate
> +SYSCALL_DEFINEn() macro rather than explicitly. The 'n' indicates the number
> +of arguments to the system call, and the macro takes the system call name
> +followed by the (type, name) pairs for the parameters as arguments. Using
> +this macro allows metadata about the new system call to be made available for
> +other tools.
> +
> +The new entry point also needs a corresponding function prototype, in
> +include/linux/syscalls.h, marked as asmlinkage to match the way that system
> +calls are invoked:
> +
> + asmlinkage long sys_xyzzy(...);
> +
> +Some architectures (e.g. x86) have their own architecture-specific syscall
> +tables, but several other architectures share a generic syscall table. Add your
> +new system call to the generic list by adding an entry to the list in
> +include/uapi/asm-generic/unistd.h:
> +
> + #define __NR_xyzzy 292
> + __SYSCALL(__NR_xyzzy, sys_xyzzy)
> +
> +Also update the __NR_syscalls count to reflect the additional system call, and
> +note that if multiple new system calls are added in the same merge window,
> +your new syscall number may get adjusted to resolve conflicts.
> +
> +The file kernel/sys_ni.c provides a fallback stub implementation of each system
> +call, returning -ENOSYS. Add your new system call here too:
> +
> + cond_syscall(sys_xyzzy);
> +
> +To summarize, you need a commit that includes:
> +
> + - SYSCALL_DEFINEn(xyzzy, ...) for the entry point
> + - corresponding prototype in include/linux/syscalls.h
> + - generic table entry in include/uapi/asm-generic/unistd.h
> + - fallback stub in kernel/sys_ni.c
> +
> +
> +x86 System Call Implementation
> +------------------------------
> +
> +To wire up your new system call for x86 platforms, you need to update the
> +master syscall tables. Assuming your new system call isn't special in some
> +way (see below), this involves a "common" entry (for x86_64 and x32) in
> +arch/x86/entry/syscalls/syscall_64.tbl:
> +
> + 333 common xyzzy sys_xyzzy
> +
> +and an "i386" entry in arch/x86/entry/syscalls/syscall_32.tbl:
> +
> + 380 i386 xyzzy sys_xyzzy
> +
> +Again, these numbers are liable to be changed if there are conflicts in the
> +relevant merge window.
> +
> +
> +Compatibility System Calls (Generic)
> +------------------------------------
> +
> +For most system calls the same 64-bit implementation can be invoked even when
> +the userspace program is itself 32-bit; even if the system call's parameters
> +include an explicit pointer, this is handled transparently.
> +
> +However, there are a couple of situations where a compatibility layer is
> +needed to cope with size differences between 32-bit and 64-bit.
> +
> +The first is if the 64-bit kernel also supports 32-bit userspace programs, and
> +so needs to parse areas of (__user) memory that could hold either 32-bit or
> +64-bit values. In particular, this is needed whenever a system call argument
> +is:
> +
> + - a pointer to a pointer
> + - a pointer to a struct containing a pointer (e.g. struct iovec __user *)
> + - a pointer to a varying sized integral type (time_t, off_t, long, ...)
> + - a pointer to a struct containing a varying sized integral type.
> +
> +The second situation that requires a compatibility layer is if one of the
> +system call's arguments has a type that is explicitly 64-bit even on a 32-bit
> +architecture, for example loff_t or __u64. In this case, a value that arrives
> +at a 64-bit kernel from a 32-bit application will be split into two 32-bit
> +values, which then need to be re-assembled in the compatibility layer.
> +
> +(Note that a system call argument that's a pointer to an explicit 64-bit type
> +does *not* need a compatibility layer; for example, splice(2)'s arguments of
> +type loff_t __user * do not trigger the need for a compat_ system call.)
> +
> +The compatibility version of the system call is called compat_sys_xyzzy(), and
> +is added with the COMPAT_SYSCALL_DEFINEn() macro, analogously to
> +SYSCALL_DEFINEn. This version of the implementation runs as part of a 64-bit
> +kernel, but expects to receive 32-bit parameter values and does whatever is
> +needed to deal with them. (Typically, the compat_sys_ version converts the
> +values to 64-bit versions and either calls on to the sys_ version, or both of
> +them call a common inner implementation function.)
> +
> +The compat entry point also needs a corresponding function prototype, in
> +include/linux/compat.h, marked as asmlinkage to match the way that system
> +calls are invoked:
> +
> + asmlinkage long compat_sys_xyzzy(...);
> +
> +If the system call involves a structure that is laid out differently on 32-bit
> +and 64-bit systems, say struct xyzzy_args, then the include/linux/compat.h
> +header file should also include a compat version of the structure (struct
> +compat_xyzzy_args) where each variable-size field has the appropriate compat_
> +type that corresponds to the type in struct xyzzy_args. The
> +compat_sys_xyzzy() routine can then use this compat_ structure to parse the
> +arguments from a 32-bit invocation.
> +
> +For example, if there are fields:
> +
> + struct xyzzy_args {
> + const char __user *ptr;
> + __kernel_long_t varying_val;
> + u64 fixed_val;
> + /* ... */
> + };
> +
> +in struct xyzzy_args, then struct compat_xyzzy_args would have:
> +
> + struct compat_xyzzy_args {
> + compat_uptr_t ptr;
> + compat_long_t varying_val;
> + u64 fixed_val;
> + /* ... */
> + };
> +
> +The generic system call list also needs adjusting to allow for the compat
> +version; the entry in include/uapi/asm-generic/unistd.h should use
> +__SC_COMP rather than __SYSCALL:
> +
> + #define __NR_xyzzy 292
> + __SC_COMP(__NR_xyzzy, sys_xyzzy, compat_sys_xyzzy)
> +
> +To summarize, you need:
> +
> + - a COMPAT_SYSCALL_DEFINEn(xyzzy, ...) for the compat entry point
> + - corresponding prototype in include/linux/compat.h
> + - (if needed) 32-bit mapping struct in include/linux/compat.h
> + - instance of __SC_COMP not __SYSCALL in include/uapi/asm-generic/unistd.h
> +
> +
> +Compatibility System Calls (x86)
> +--------------------------------
> +
> +To wire up the x86 architecture of a system call with a compatibility version,
> +the entries in the syscall tables need to be adjusted.
> +
> +First, the entry in arch/x86/entry/syscalls/syscall_32.tbl gets an extra
> +column to indicate that a 32-bit userspace program running on a 64-bit kernel
> +should hit the compat entry point:
> +
> + 380 i386 xyzzy sys_xyzzy compat_sys_xyzzy
> +
> +Second, you need to figure out what should happen for the x32 ABI version of
> +the new system call. There's a choice here: the layout of the arguments
> +should either match the 64-bit version or the 32-bit version.
> +
> +If there's a pointer-to-a-pointer involved, the decision is easy: x32 is
> +ILP32, so the layout should match the 32-bit version, and the entry in
> +arch/x86/entry/syscalls/syscall_64.tbl is split so that x32 programs hit the
> +compatibility wrapper:
> +
> + 333 64 xyzzy sys_xyzzy
> + ...
> + 555 x32 xyzzy compat_sys_xyzzy
> +
> +If no pointers are involved, then it is preferable to re-use the 64-bit system
> +call for the x32 ABI (and consequently the entry in
> +arch/x86/entry/syscalls/syscall_64.tbl is unchanged).
> +
> +In either case, you should check that the types involved in your argument
> +layout do indeed map exactly from x32 (-mx32) to either the 32-bit (-m32) or
> +64-bit (-m64) equivalents.
> +
> +
> +System Calls Returning Elsewhere
> +--------------------------------
> +
> +For most system calls, once the system call is complete the user program
> +continues exactly where it left off -- at the next instruction, with the same
> +stack and registers as before the system call, and with the same virtual
> +memory space.
> +
> +However, a few system calls do things differently. They might return to a
> +different location (rt_sigreturn) or change the memory space (fork/vfork/clone)
> +or even architecture (execve/execveat) of the program.
> +
> +To allow for this, the kernel implementation of the system call may need to
> +save and restore additional registers to the kernel stack, allowing complete
> +control of where and how execution continues after the system call.
> +
> +This is arch-specific, but typically involves defining assembly entry points
> +that save/restore additional registers and invoke the real system call entry
> +point.
> +
> +For x86_64, this is implemented as a stub_xyzzy entry point in
> +arch/x86/entry/entry_64.S, and the entry in the syscall table
> +(arch/x86/entry/syscalls/syscall_64.tbl) is adjusted to match:
> +
> + 333 common xyzzy stub_xyzzy
> +
> +The equivalent for 32-bit programs running on a 64-bit kernel is normally
> +called stub32_xyzzy and implemented in arch/x86/entry/entry_64_compat.S,
> +with the corresponding syscall table adjustment in
> +arch/x86/entry/syscalls/syscall_32.tbl:
> +
> + 380 i386 xyzzy sys_xyzzy stub32_xyzzy
> +
> +If the system call needs a compatibility layer (as in the previous section)
> +then the stub32_ version needs to call on to the compat_sys_ version of the
> +system call rather than the native 64-bit version. Also, if the x32 ABI
> +implementation is not common with the x86_64 version, then its syscall
> +table will also need to invoke a stub that calls on to the compat_sys_
> +version.
> +
> +For completeness, it's also nice to set up a mapping so that user-mode Linux
> +still works -- its syscall table will reference stub_xyzzy, but the UML build
> +doesn't include arch/x86/entry/entry_64.S implementation (because UML
> +simulates registers etc). Fixing this is as simple as adding a #define to
> +arch/x86/um/sys_call_table_64.c:
> +
> + #define stub_xyzzy sys_xyzzy
> +
> +
> +Other Details
> +-------------
> +
> +Most of the kernel treats system calls in a generic way, but there is the
> +occasional exception that may need updating for your particular system call.
> +
> +The audit subsystem is one such special case; it includes (arch-specific)
> +functions that classify some special types of system call -- specifically
> +file open (open/openat), program execution (execve/exeveat) or socket
> +multiplexor (socketcall) operations. If your new system call is analogous to
> +one of these, then the audit system should be updated.
> +
> +More generally, if there is an existing system call that is analogous to your
> +new system call, it's worth doing a kernel-wide grep for the existing system
> +call to check there are no other special cases.
> +
> +
> +Testing
> +-------
> +
> +A new system call should obviously be tested; it is also useful to provide
> +reviewers with a demonstration of how user space programs will use the system
> +call. A good way to combine these aims is to include a simple self-test
> +program in a new directory under tools/testing/selftests/.
> +
> +Make sure the selftest runs successfully on all supported architectures. For
> +example, check that it works when compiled as an x86_64 (-m64), x86_32 (-m32)
> +and x32 (-mx32) ABI program.
> +
> +
> +Man Page
> +--------
> +
> +All new system calls should come with a complete man page, ideally using groff
> +markup, but plain text will do. If groff is used, it's helpful to include a
> +pre-rendered ASCII version of the man page in the cover email for the
> +patchset, for the convenience of reviewers.
How about adding:
"For more details, see https://www.kernel.org/doc/man-pages/patches.html"
> +
> +The man page should be cc'ed to linux-man-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +
> +
> +References and Sources
> +----------------------
> +
> + - LWN article from Michael Kerrisk on use of flags argument in system calls:
> + https://lwn.net/Articles/585415/
> + - LWN article from Michael Kerrisk on how to handle unknown flags in a system
> + call: https://lwn.net/Articles/588444/
> + - LWN article from Jake Edge describing constraints on 64-bit system call
> + arguments: https://lwn.net/Articles/311630/
> + - Pair of LWN articles from David Drysdale that describe the system call
> + implementation paths in detail for v3.14:
> + - https://lwn.net/Articles/604287/
> + - https://lwn.net/Articles/604515/
> + - Architecture-specific requirements for system calls are discussed in the
> + syscall(2) man-page:
> + http://man7.org/linux/man-pages/man2/syscall.2.html#NOTES
> + - Collated emails from Linus Torvalds discussing the problems with ioctl():
> + http://yarchive.net/comp/linux/ioctl.html
> + - "How to not invent kernel interfaces", Arnd Bergmann,
> + http://www.ukuug.org/events/linux2007/2007/papers/Bergmann.pdf
> + - LWN article from Michael Kerrisk on avoiding new uses of CAP_SYS_ADMIN:
> + https://lwn.net/Articles/486306/
> +
> + - Recommendation from Andrew Morton that all related information for a new
> + system call should come in the same email thread:
> + https://lkml.org/lkml/2014/7/24/641
> + - Recommendation from Michael Kerrisk that a new system call should come with
> + a man page: https://lkml.org/lkml/2014/6/13/309
> + - Suggestion from Thomas Gleixner that x86 wire-up should be in a separate
> + commit: https://lkml.org/lkml/2014/11/19/254
> + - Suggestion from Greg Kroah-Hartman that it's good for new system calls to
> + come with a man-page & selftest: https://lkml.org/lkml/2014/3/19/710
> + - Discussion from Michael Kerrisk of new system call vs. prctl(2) extension:
> + https://lkml.org/lkml/2014/6/3/411
> + - Numbering oddities arising from (re-)use of O_* numbering space flags:
> + - commit 75069f2b5bfb ("vfs: renumber FMODE_NONOTIFY and add to uniqueness
> + check")
> + - commit 12ed2e36c98a ("fanotify: FMODE_NONOTIFY and __O_SYNC in sparc
> + conflict")
> + - commit bb458c644a59 ("Safer ABI for O_TMPFILE")
> + - Discussion from Matthew Wilcox about restrictions on 64-bit arguments:
> + https://lkml.org/lkml/2008/12/12/187
> + - Recommendation from Greg Kroah-Hartman that unknown flags should be
> + policed: https://lkml.org/lkml/2014/7/17/577
> + - Recommendation from Linus Torvalds that x32 system calls should prefer
> + compatibility with 64-bit versions rather than 32-bit versions:
> + https://lkml.org/lkml/2011/8/31/244
> --
> 2.4.3.573.g4eafbef
Thanks for writing this up!
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH RFC 1/1] Documentation: describe how to add a system call
From: David Drysdale @ 2015-07-28 17:13 UTC (permalink / raw)
To: Kees Cook
Cc: Linux API, Michael Kerrisk, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Andrea Arcangeli, Thomas Gleixner,
Eric B Munson, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Andy Lutomirski, Al Viro,
Rusty Russell, Peter Zijlstra, Vivek Goyal, Alexei Starovoitov,
David Herrmann, Theodore Ts'o, Milosz Tanski, Fam
In-Reply-To: <CAGXu5j+biSzHgY95TgMOpFzhzysBHr2MPTaU_iNXjhesr=Mafw@mail.gmail.com>
On Tue, Jul 28, 2015 at 5:43 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jul 28, 2015 at 4:41 AM, David Drysdale <drysdale@google.com> wrote:
>> Add a document describing the process of adding a new system call,
>> including the need for a flags argument for future compatibility, and
>> covering 32-bit/64-bit concerns (albeit in an x86-centric way).
>>
>> Signed-off-by: David Drysdale <drysdale@google.com>
>> Reviewed-by: Michael Kerrisk <mtk.manpages@gmail.com>
>
> This is great!
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> I have a few minor suggestions below...
Thanks, I've applied all bar one -- a query below.
>> ---
>> Documentation/adding-syscalls.txt | 454 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 454 insertions(+)
>> create mode 100644 Documentation/adding-syscalls.txt
>>
>> diff --git a/Documentation/adding-syscalls.txt b/Documentation/adding-syscalls.txt
>> new file mode 100644
>> index 000000000000..5f52edda8951
>> --- /dev/null
>> +++ b/Documentation/adding-syscalls.txt
[snip]
>> + - If there is an existing capability that governs related functionality, then
>> + use that. However, avoid combining lots of only vaguely related functions
>> + together under the same bit, as this goes against capabilities' purpose of
>> + splitting the power of root. In particular, avoid adding new uses of the
>> + already overly-general CAP_SYS_ADMIN capability.
>> + - If there is no related capability, then consider adding a new capability
>> + bit -- but bear in mind that the numbering space is limited, and each new
>> + bit needs to be understood and administered by sysadmins.
>
> Perhaps mention alternative mechanisms for access control when working
> on file descriptors, like avoiding security issues by looking at fd
> _opener_ credentials, rather than current's credentials?
I'm struggling to cope up with text about this that doesn't feel either
too vague or much too detailed / internal, so maybe I'm misunderstanding
what you're after. Could you clarify or maybe suggest a sentence or two?
Thanks,
David
^ permalink raw reply
* Re: [PATCH RFC 1/1] Documentation: describe how to add a system call
From: Kees Cook @ 2015-07-28 17:17 UTC (permalink / raw)
To: David Drysdale
Cc: Linux API, Michael Kerrisk, Andrew Morton, Arnd Bergmann,
Shuah Khan, Jonathan Corbet, Andrea Arcangeli, Thomas Gleixner,
Eric B Munson, Ingo Molnar, H. Peter Anvin, Oleg Nesterov,
Linus Torvalds, Greg Kroah-Hartman, Andy Lutomirski, Al Viro,
Rusty Russell, Peter Zijlstra, Vivek Goyal, Alexei Starovoitov,
David Herrmann, Theodore Ts'o, Milosz Tanski, Fam
In-Reply-To: <CAHse=S-_w+3WR0H0qPGWs5fer-3ncA6LX0is4UeJ48KCtK5Vmg@mail.gmail.com>
On Tue, Jul 28, 2015 at 10:13 AM, David Drysdale <drysdale@google.com> wrote:
> On Tue, Jul 28, 2015 at 5:43 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, Jul 28, 2015 at 4:41 AM, David Drysdale <drysdale@google.com> wrote:
>>> Add a document describing the process of adding a new system call,
>>> including the need for a flags argument for future compatibility, and
>>> covering 32-bit/64-bit concerns (albeit in an x86-centric way).
>>>
>>> Signed-off-by: David Drysdale <drysdale@google.com>
>>> Reviewed-by: Michael Kerrisk <mtk.manpages@gmail.com>
>>
>> This is great!
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>
>> I have a few minor suggestions below...
>
> Thanks, I've applied all bar one -- a query below.
>
>>> ---
>>> Documentation/adding-syscalls.txt | 454 ++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 454 insertions(+)
>>> create mode 100644 Documentation/adding-syscalls.txt
>>>
>>> diff --git a/Documentation/adding-syscalls.txt b/Documentation/adding-syscalls.txt
>>> new file mode 100644
>>> index 000000000000..5f52edda8951
>>> --- /dev/null
>>> +++ b/Documentation/adding-syscalls.txt
>
> [snip]
>
>>> + - If there is an existing capability that governs related functionality, then
>>> + use that. However, avoid combining lots of only vaguely related functions
>>> + together under the same bit, as this goes against capabilities' purpose of
>>> + splitting the power of root. In particular, avoid adding new uses of the
>>> + already overly-general CAP_SYS_ADMIN capability.
>>> + - If there is no related capability, then consider adding a new capability
>>> + bit -- but bear in mind that the numbering space is limited, and each new
>>> + bit needs to be understood and administered by sysadmins.
>>
>> Perhaps mention alternative mechanisms for access control when working
>> on file descriptors, like avoiding security issues by looking at fd
>> _opener_ credentials, rather than current's credentials?
>
> I'm struggling to cope up with text about this that doesn't feel either
> too vague or much too detailed / internal, so maybe I'm misunderstanding
> what you're after. Could you clarify or maybe suggest a sentence or two?
Hm, yes, I think you're right: my suggestion here was too specific.
Please ignore! :)
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [RFC PATCH 03/14] kthread: Add drain_kthread_worker()
From: Tejun Heo @ 2015-07-28 17:18 UTC (permalink / raw)
To: Petr Mladek
Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
Steven Rostedt, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Vlastimil Babka,
live-patching-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1438094371-8326-4-git-send-email-pmladek-IBi9RG/b67k@public.gmane.org>
Hello,
On Tue, Jul 28, 2015 at 04:39:20PM +0200, Petr Mladek wrote:
> +/*
> + * Test whether @work is being queued from another work
> + * executing on the same kthread.
> + */
> +static bool is_chained_work(struct kthread_worker *worker)
> +{
> + struct kthread_worker *current_worker;
> +
> + current_worker = current_kthread_worker();
> + /*
> + * Return %true if I'm a kthread worker executing a work item on
> + * the given @worker.
> + */
> + return current_worker && current_worker == worker;
> +}
I'm not sure full-on chained work detection is necessary here.
kthread worker's usages tend to be significantly simpler and draining
is only gonna be used for destruction.
> +void drain_kthread_worker(struct kthread_worker *worker)
> +{
> + int flush_cnt = 0;
> +
> + spin_lock_irq(&worker->lock);
> + worker->nr_drainers++;
> +
> + while (!list_empty(&worker->work_list)) {
> + /*
> + * Unlock, so we could move forward. Note that queuing
> + * is limited by @nr_drainers > 0.
> + */
> + spin_unlock_irq(&worker->lock);
> +
> + flush_kthread_worker(worker);
> +
> + if (++flush_cnt == 10 ||
> + (flush_cnt % 100 == 0 && flush_cnt <= 1000))
> + pr_warn("kthread worker %s: drain_kthread_worker() isn't complete after %u tries\n",
> + worker->task->comm, flush_cnt);
> +
> + spin_lock_irq(&worker->lock);
> + }
I'd just do something like WARN_ONCE(flush_cnt++ > 10, "kthread worker: ...").
Thanks.
--
tejun
^ permalink raw reply
* Re: [Xen-devel] [PATCH 8/8] xen/privcmd: Further s/MFN/GFN/ clean-up
From: David Vrabel @ 2015-07-28 17:19 UTC (permalink / raw)
To: Julien Grall, xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b
Cc: x86-DgEjT+Ai2ygdnm+yROfE0A, Russell King,
ian.campbell-Sxgqhf6Nn4DQT0dZR+AlfA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
stefano.stabellini-mvvWK6WmYclDPfheJLI6IQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar, David Vrabel,
H. Peter Anvin, Boris Ostrovsky, Thomas Gleixner,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1438095769-2560-9-git-send-email-julien.grall-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
On 28/07/15 16:02, Julien Grall wrote:
> The privcmd code is mixing the usage of GFN and MFN within the same
> functions which make the code difficult to understand when you only work
> with auto-translated guests.
>
> The privcmd driver is only dealing with GFN so replace all the mention
> of MFN into GFN.
>
> The ioctl structure used to map foreign change has been left unchanged
> given that the userspace is using it. Nonetheless, add a comment to
> explain the expected value within the "mfn" field.
Reviewed-by: David Vrabel <david.vrabel-Sxgqhf6Nn4DQT0dZR+AlfA@public.gmane.org>
David
^ permalink raw reply
* Re: [RFC PATCH 05/14] kthread: Add wakeup_and_destroy_kthread_worker()
From: Tejun Heo @ 2015-07-28 17:23 UTC (permalink / raw)
To: Petr Mladek
Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
Steven Rostedt, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Vlastimil Babka,
live-patching-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1438094371-8326-6-git-send-email-pmladek-IBi9RG/b67k@public.gmane.org>
Hello,
On Tue, Jul 28, 2015 at 04:39:22PM +0200, Petr Mladek wrote:
...
> +void wakeup_and_destroy_kthread_worker(struct kthread_worker *worker)
> +{
> + struct task_struct *task = worker->task;
> +
> + if (WARN_ON(!task))
> + return;
> +
> + spin_lock_irq(&worker->lock);
> + if (worker->current_work)
> + wake_up_process(worker->task);
> + spin_unlock_irq(&worker->lock);
> +
> + destroy_kthread_worker(worker);
> +}
I don't know. Wouldn't it make far more sense to convert those wake
up events with queueings? It seems backwards to be converting things
to work item based interface and then insert work items which wait for
external events. More on this later.
Thanks.
--
tejun
^ permalink raw reply
* Re: [RFC PATCH 06/14] kthread: Add kthread_worker_created()
From: Tejun Heo @ 2015-07-28 17:26 UTC (permalink / raw)
To: Petr Mladek
Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
Steven Rostedt, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Vlastimil Babka,
live-patching-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1438094371-8326-7-git-send-email-pmladek-IBi9RG/b67k@public.gmane.org>
Hello,
On Tue, Jul 28, 2015 at 04:39:23PM +0200, Petr Mladek wrote:
> I would like to make cleaner kthread worker API and hide the definition
> of struct kthread_worker. It will prevent any custom hacks and make
> the API more secure.
>
> This patch provides an API to check if the worker has been created
> and hides the implementation details.
Maybe it'd be a better idea to make create_kthread_worker() allocate
and return pointer to struct kthread_worker? You're adding
create/destroy interface anyway, it won't need a separate created
query function and the synchronization rules would be self-evident.
Thanks.
--
tejun
^ permalink raw reply
* Re: [RFC PATCH 07/14] mm/huge_page: Convert khugepaged() into kthread worker API
From: Tejun Heo @ 2015-07-28 17:36 UTC (permalink / raw)
To: Petr Mladek
Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
Steven Rostedt, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm, Vlastimil Babka, live-patching, linux-api, linux-kernel
In-Reply-To: <1438094371-8326-8-git-send-email-pmladek@suse.com>
Hello,
On Tue, Jul 28, 2015 at 04:39:24PM +0200, Petr Mladek wrote:
> -static void khugepaged_wait_work(void)
> +static void khugepaged_wait_func(struct kthread_work *dummy)
> {
> if (khugepaged_has_work()) {
> if (!khugepaged_scan_sleep_millisecs)
> - return;
> + goto out;
>
> wait_event_freezable_timeout(khugepaged_wait,
> - kthread_should_stop(),
> + !khugepaged_enabled(),
> msecs_to_jiffies(khugepaged_scan_sleep_millisecs));
> - return;
> + goto out;
> }
>
> if (khugepaged_enabled())
> wait_event_freezable(khugepaged_wait, khugepaged_wait_event());
> +
> +out:
> + if (khugepaged_enabled())
> + queue_kthread_work(&khugepaged_worker,
> + &khugepaged_do_scan_work);
> }
There gotta be a better way to do this. It's outright weird to
convert it over to work item based interface and then handle idle
periods by injecting wait work items. If there's an external event
which wakes up the worker, convert that to a queueing event. If it's
a timed event, implement a delayed work and queue that with delay.
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH 08/14] rcu: Convert RCU gp kthreads into kthread worker API
From: Tejun Heo @ 2015-07-28 17:37 UTC (permalink / raw)
To: Petr Mladek
Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
Steven Rostedt, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm, Vlastimil Babka, live-patching, linux-api, linux-kernel
In-Reply-To: <1438094371-8326-9-git-send-email-pmladek@suse.com>
On Tue, Jul 28, 2015 at 04:39:25PM +0200, Petr Mladek wrote:
...
> -static int __noreturn rcu_gp_kthread(void *arg)
> +static void rcu_gp_kthread_func(struct kthread_work *work)
> {
> int fqs_state;
> int gf;
> unsigned long j;
> int ret;
> - struct rcu_state *rsp = arg;
> + struct rcu_state *rsp = container_of(work, struct rcu_state, gp_work);
> struct rcu_node *rnp = rcu_get_root(rsp);
>
> - rcu_bind_gp_kthread();
> + /* Handle grace-period start. */
> for (;;) {
> + trace_rcu_grace_period(rsp->name,
> + READ_ONCE(rsp->gpnum),
> + TPS("reqwait"));
> + rsp->gp_state = RCU_GP_WAIT_GPS;
> + wait_event_interruptible(rsp->gp_wq,
> + READ_ONCE(rsp->gp_flags) &
> + RCU_GP_FLAG_INIT);
Same here. Why not convert the waker into a queueing event?
> + /* Locking provides needed memory barrier. */
> + if (rcu_gp_init(rsp))
> + break;
> + cond_resched_rcu_qs();
> + WRITE_ONCE(rsp->gp_activity, jiffies);
> + WARN_ON(signal_pending(current));
> + trace_rcu_grace_period(rsp->name,
> + READ_ONCE(rsp->gpnum),
> + TPS("reqwaitsig"));
> + }
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC PATCH 13/14] kthread_worker: Add set_kthread_worker_user_nice()
From: Tejun Heo @ 2015-07-28 17:40 UTC (permalink / raw)
To: Petr Mladek
Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
Steven Rostedt, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Vlastimil Babka,
live-patching-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1438094371-8326-14-git-send-email-pmladek-IBi9RG/b67k@public.gmane.org>
On Tue, Jul 28, 2015 at 04:39:30PM +0200, Petr Mladek wrote:
...
> +/*
> + * set_kthread_worker_user_nice - set scheduling priority for the kthread worker
> + * @worker: target kthread_worker
> + * @nice: niceness value
> + */
> +void set_kthread_worker_user_nice(struct kthread_worker *worker, long nice)
> +{
> + struct task_struct *task = worker->task;
> +
> + WARN_ON(!task);
> + set_user_nice(task, nice);
> +}
> +EXPORT_SYMBOL(set_kthread_worker_user_nice);
kthread_worker is explcitly associated with a single kthread. Why do
we want to create explicit wrappers for kthread operations? This is
encapsulation for encapsulation's sake. It doesn't buy us anything at
all. Just let the user access the associated kthread and operate on
it.
Thanks.
--
tejun
^ permalink raw reply
* Re: [RFC PATCH 14/14] kthread_worker: Add set_kthread_worker_scheduler*()
From: Tejun Heo @ 2015-07-28 17:41 UTC (permalink / raw)
To: Petr Mladek
Cc: Andrew Morton, Oleg Nesterov, Ingo Molnar, Peter Zijlstra,
Steven Rostedt, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm, Vlastimil Babka, live-patching, linux-api, linux-kernel
In-Reply-To: <1438094371-8326-15-git-send-email-pmladek@suse.com>
On Tue, Jul 28, 2015 at 04:39:31PM +0200, Petr Mladek wrote:
> +/**
> + * set_kthread_worker_scheduler - change the scheduling policy and/or RT
> + * priority of a kthread worker.
> + * @worker: target kthread_worker
> + * @policy: new policy
> + * @sched_priority: new RT priority
> + *
> + * Return: 0 on success. An error code otherwise.
> + */
> +int set_kthread_worker_scheduler(struct kthread_worker *worker,
> + int policy, int sched_priority)
> +{
> + return __set_kthread_worker_scheduler(worker, policy, sched_priority,
> + true);
> +}
Ditto. I don't get why we would want these thin wrappers.
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: Revised futex(2) man page for review
From: Davidlohr Bueso @ 2015-07-28 17:52 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Peter Zijlstra, Thomas Gleixner, Darren Hart, Carlos O'Donell,
Ingo Molnar, Jakub Jelinek,
linux-man-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, lkml,
Arnd Bergmann, Steven Rostedt, Linux API, Torvald Riegel,
Roland McGrath, Darren Hart, Anton Blanchard, Eric Dumazet,
bill o gallmeister, Jan Kiszka, Daniel Wagner, Rich Felker,
Andy Lutomirski, bert hubert, Rusty Russell, Heinrich
In-Reply-To: <55B732C6.9080905-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Tue, 2015-07-28 at 09:44 +0200, Michael Kerrisk (man-pages) wrote:
> Maybe you still have some further improvements for the paragraph?
Nah, this is fine enough. Looks good.
Thanks,
Davidlohr
^ permalink raw reply
* Re: [PATCH V5 0/7] Allow user to request memory to be locked on page fault
From: Eric B Munson @ 2015-07-28 18:06 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Michal Hocko, Andrew Morton, Shuah Khan, Michael Kerrisk,
Jonathan Corbet, Ralf Baechle, linux-alpha, linux-kernel,
linux-mips, linux-parisc, linuxppc-dev, sparclinux, linux-xtensa,
linux-mm, linux-arch, linux-api
In-Reply-To: <55B79B7F.9010604@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]
On Tue, 28 Jul 2015, Vlastimil Babka wrote:
> On 07/28/2015 03:49 PM, Eric B Munson wrote:
> >On Tue, 28 Jul 2015, Michal Hocko wrote:
> >
>
> [...]
>
> >The only
> >remaining question I have is should we have 2 new mlockall flags so that
> >the caller can explicitly set VM_LOCKONFAULT in the mm->def_flags vs
> >locking all current VMAs on fault. I ask because if the user wants to
> >lock all current VMAs the old way, but all future VMAs on fault they
> >have to call mlockall() twice:
> >
> > mlockall(MCL_CURRENT);
> > mlockall(MCL_CURRENT | MCL_FUTURE | MCL_ONFAULT);
> >
> >This has the side effect of converting all the current VMAs to
> >VM_LOCKONFAULT, but because they were all made present and locked in the
> >first call, this should not matter in most cases.
>
> Shouldn't the user be able to do this?
>
> mlockall(MCL_CURRENT)
> mlockall(MCL_FUTURE | MCL_ONFAULT);
>
> Note that the second call shouldn't change (i.e. munlock) existing
> vma's just because MCL_CURRENT is not present. The current
> implementation doesn't do that thanks to the following in
> do_mlockall():
>
> if (flags == MCL_FUTURE)
> goto out;
>
> before current vma's are processed and MCL_CURRENT is checked. This
> is probably so that do_mlockall() can also handle the munlockall()
> syscall.
> So we should be careful not to break this, but otherwise there are
> no limitations by not having two MCL_ONFAULT flags. Having to do
> invoke syscalls instead of one is not an issue as this shouldn't be
> frequent syscall.
Good catch, my current implementation did break this and is now fixed.
>
> >The catch is that,
> >like mmap(MAP_LOCKED), mlockall() does not communicate if mm_populate()
> >fails. This has been true of mlockall() from the beginning so I don't
> >know if it needs more than an entry in the man page to clarify (which I
> >will add when I add documentation for MCL_ONFAULT).
>
> Good point.
>
> >In a much less
> >likely corner case, it is not possible in the current setup to request
> >all current VMAs be VM_LOCKONFAULT and all future be VM_LOCKED.
>
> So again this should work:
>
> mlockall(MCL_CURRENT | MCL_ONFAULT)
> mlockall(MCL_FUTURE);
>
> But the order matters here, as current implementation of
> do_mlockall() will clear VM_LOCKED from def_flags if MCL_FUTURE is
> not passed. So *it's different* from how it handles MCL_CURRENT (as
> explained above). And not documented in manpage. Oh crap, this API
> is a closet full of skeletons. Maybe it was an unnoticed regression
> and we can restore some sanity?
I will add a note about the ordering problem to the manpage as well.
Unfortunately, the basic idea of clearing VM_LOCKED from mm->def_flags
if MCL_FUTURE is not specified but not doing the same for MCL_CURRENT
predates the move to git, so I am not sure if it was ever different.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: Revised futex(2) man page for review
From: Michael Kerrisk (man-pages) @ 2015-07-28 18:09 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, Peter Zijlstra,
Thomas Gleixner, Darren Hart, Carlos O'Donell, Ingo Molnar,
Jakub Jelinek, linux-man-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
lkml, Arnd Bergmann, Steven Rostedt, Linux API, Torvald Riegel,
Roland McGrath, Darren Hart, Anton Blanchard, Eric Dumazet,
bill o gallmeister, Jan Kiszka, Daniel Wagner, Rich Felker,
Andy Lutomirski, bert hubert, Rusty Russell
In-Reply-To: <1438105974.2249.67.camel-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org>
On 07/28/2015 07:52 PM, Davidlohr Bueso wrote:
> On Tue, 2015-07-28 at 09:44 +0200, Michael Kerrisk (man-pages) wrote:
>> Maybe you still have some further improvements for the paragraph?
>
> Nah, this is fine enough. Looks good.
Okay. Thanks. I added a Reviewed-by: for you.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [RFC PATCH 14/14] kthread_worker: Add set_kthread_worker_scheduler*()
From: Peter Zijlstra @ 2015-07-28 19:48 UTC (permalink / raw)
To: Tejun Heo
Cc: Petr Mladek, Andrew Morton, Oleg Nesterov, Ingo Molnar,
Steven Rostedt, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Vlastimil Babka,
live-patching-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150728174154.GG5322-qYNAdHglDFBN0TnZuCh8vA@public.gmane.org>
On Tue, Jul 28, 2015 at 01:41:54PM -0400, Tejun Heo wrote:
> On Tue, Jul 28, 2015 at 04:39:31PM +0200, Petr Mladek wrote:
> > +/**
> > + * set_kthread_worker_scheduler - change the scheduling policy and/or RT
> > + * priority of a kthread worker.
> > + * @worker: target kthread_worker
> > + * @policy: new policy
> > + * @sched_priority: new RT priority
> > + *
> > + * Return: 0 on success. An error code otherwise.
> > + */
> > +int set_kthread_worker_scheduler(struct kthread_worker *worker,
> > + int policy, int sched_priority)
> > +{
> > + return __set_kthread_worker_scheduler(worker, policy, sched_priority,
> > + true);
> > +}
>
> Ditto. I don't get why we would want these thin wrappers.
On top of which this is an obsolete interface :-)
^ permalink raw reply
* [PATCH v5 0/6] support "cpu_isolated" mode for nohz_full
From: Chris Metcalf @ 2015-07-28 19:49 UTC (permalink / raw)
To: Gilad Ben Yossef, Steven Rostedt, Ingo Molnar, Peter Zijlstra,
Andrew Morton, Rik van Riel, Tejun Heo, Frederic Weisbecker,
Thomas Gleixner, Paul E. McKenney, Christoph Lameter,
Viresh Kumar, Catalin Marinas, Will Deacon,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Chris Metcalf
In-Reply-To: <1436817481-8732-1-git-send-email-cmetcalf-d5a29ZRxExrQT0dZR+AlfA@public.gmane.org>
This version of the patch series incorporates Christoph Lameter's
change to add a quiet_vmstat() call, and restructures cpu_isolated as
a "hard" isolation mode in contrast to nohz_full's "soft" isolation,
breaking it out as a separate CONFIG_CPU_ISOLATED with its own
include/linux/cpu_isolated.h and kernel/time/cpu_isolated.c.
It is rebased to 4.2-rc3.
Thomas: as I mentioned in v4, I haven't heard from you whether my
removal of the cpu_idle calls sufficiently addresses your concerns
about that aspect.
Andy: as I said in email, I've left in the support where cpu_isolated
relies on the context_tracking stuff currently in 4.2-rc3. I'm not
sure what the cleanest way is for me to pick up the new
context_tracking stuff; if that's all that ends up standing between
this patch series and having it be pulled, perhaps I can rebase it
onto whatever branch it is that has the new context_tracking?
Original patch series cover letter follows:
The existing nohz_full mode does a nice job of suppressing extraneous
kernel interrupts for cores that desire it. However, there is a need
for a more deterministic mode that rigorously disallows kernel
interrupts, even at a higher cost in user/kernel transition time:
for example, high-speed networking applications running userspace
drivers that will drop packets if they are ever interrupted.
These changes attempt to provide an initial draft of such a framework;
the changes do not add any overhead to the usual non-nohz_full mode,
and only very small overhead to the typical nohz_full mode. The
kernel must be built with CONFIG_CPU_ISOLATED to take advantage of
this new mode. A prctl() option (PR_SET_CPU_ISOLATED) is added to
control whether processes have requested this stricter semantics, and
within that prctl() option we provide a number of different bits for
more precise control. Additionally, we add a new command-line boot
argument to facilitate debugging where unexpected interrupts are being
delivered from.
Code that is conceptually similar has been in use in Tilera's
Multicore Development Environment since 2008, known as Zero-Overhead
Linux, and has seen wide adoption by a range of customers. This patch
series represents the first serious attempt to upstream that
functionality. Although the current state of the kernel isn't quite
ready to run with absolutely no kernel interrupts (for example,
workqueues on cpu_isolated cores still remain to be dealt with), this
patch series provides a way to make dynamic tradeoffs between avoiding
kernel interrupts on the one hand, and making voluntary calls in and
out of the kernel more expensive, for tasks that want it.
The series (based currently on v4.2-rc3) is available at:
git://git.kernel.org/pub/scm/linux/kernel/git/cmetcalf/linux-tile.git dataplane
v5:
rebased on kernel v4.2-rc3
converted to use CONFIG_CPU_ISOLATED and separate .c and .h files
incorporates Christoph Lameter's quiet_vmstat() call
v4:
rebased on kernel v4.2-rc1
added support for detecting CPU_ISOLATED_STRICT syscalls on arm64
v3:
remove dependency on cpu_idle subsystem (Thomas Gleixner)
use READ_ONCE instead of ACCESS_ONCE in tick_nohz_cpu_isolated_enter
use seconds for console messages instead of jiffies (Thomas Gleixner)
updated commit description for patch 5/5
v2:
rename "dataplane" to "cpu_isolated"
drop ksoftirqd suppression changes (believed no longer needed)
merge previous "QUIESCE" functionality into baseline functionality
explicitly track syscalls and exceptions for "STRICT" functionality
allow configuring a signal to be delivered for STRICT mode failures
move debug tracking to irq_enter(), not irq_exit()
Note: I have not removed the commit to disable the 1Hz timer tick
fallback that was nack'ed by PeterZ, pending a decision on that thread
as to what to do (https://lkml.org/lkml/2015/5/8/555); also since if
we remove the 1Hz tick, cpu_isolated threads will never re-enter
userspace since a tick will always be pending.
Chris Metcalf (5):
cpu_isolated: add initial support
cpu_isolated: support PR_CPU_ISOLATED_STRICT mode
cpu_isolated: provide strict mode configurable signal
cpu_isolated: add debug boot flag
nohz: cpu_isolated: allow tick to be fully disabled
Christoph Lameter (1):
vmstat: provide a function to quiet down the diff processing
Documentation/kernel-parameters.txt | 7 +++
arch/arm64/kernel/ptrace.c | 5 ++
arch/tile/kernel/process.c | 9 +++
arch/tile/kernel/ptrace.c | 5 +-
arch/tile/mm/homecache.c | 5 +-
arch/x86/kernel/ptrace.c | 2 +
include/linux/context_tracking.h | 11 +++-
include/linux/cpu_isolated.h | 42 +++++++++++++
include/linux/sched.h | 3 +
include/linux/vmstat.h | 2 +
include/uapi/linux/prctl.h | 8 +++
kernel/context_tracking.c | 12 +++-
kernel/irq_work.c | 5 +-
kernel/sched/core.c | 21 +++++++
kernel/signal.c | 5 ++
kernel/smp.c | 4 ++
kernel/softirq.c | 7 +++
kernel/sys.c | 8 +++
kernel/time/Kconfig | 20 +++++++
kernel/time/Makefile | 1 +
kernel/time/cpu_isolated.c | 116 ++++++++++++++++++++++++++++++++++++
kernel/time/tick-sched.c | 3 +-
mm/vmstat.c | 14 +++++
23 files changed, 305 insertions(+), 10 deletions(-)
create mode 100644 include/linux/cpu_isolated.h
create mode 100644 kernel/time/cpu_isolated.c
--
2.1.2
^ permalink raw reply
* [PATCH v5 2/6] cpu_isolated: add initial support
From: Chris Metcalf @ 2015-07-28 19:49 UTC (permalink / raw)
To: Gilad Ben Yossef, Steven Rostedt, Ingo Molnar, Peter Zijlstra,
Andrew Morton, Rik van Riel, Tejun Heo, Frederic Weisbecker,
Thomas Gleixner, Paul E. McKenney, Christoph Lameter,
Viresh Kumar, Catalin Marinas, Will Deacon, linux-doc, linux-api,
linux-kernel
Cc: Chris Metcalf
In-Reply-To: <1438112980-9981-1-git-send-email-cmetcalf@ezchip.com>
The existing nohz_full mode is designed as a "soft" isolation mode
that makes tradeoffs to minimize userspace interruptions while
still attempting to avoid overheads in the kernel entry/exit path,
to provide 100% kernel semantics, etc.
However, some applications require a "hard" commitment from the
kernel to avoid interruptions, in particular userspace device
driver style applications, such as high-speed networking code.
This change introduces a framework to allow applications
to elect to have the "hard" semantics as needed, specifying
prctl(PR_SET_CPU_ISOLATED, PR_CPU_ISOLATED_ENABLE) to do so.
Subsequent commits will add additional flags and additional
semantics.
The kernel must be built with the new CPU_ISOLATED Kconfig flag
to enable this mode, and the kernel booted with an appropriate
nohz_full=CPULIST boot argument. The "cpu_isolated" state is then
indicated by setting a new task struct field, cpu_isolated_flags,
to the value passed by prctl(). When the _ENABLE bit is set for a
task, and it is returning to userspace on a nohz_full core, it calls
the new cpu_isolated_enter() routine to take additional actions
to help the task avoid being interrupted in the future.
Initially, there are only three actions taken. First, the
task calls lru_add_drain() to prevent being interrupted by a
subsequent lru_add_drain_all() call on another core. Then, it calls
quiet_vmstat() to quieten the vmstat worker to avoid a follow-on
interrupt. Finally, the code checks for pending timer interrupts
and quiesces until they are no longer pending. As a result, sys
calls (and page faults, etc.) can be inordinately slow. However,
this quiescing guarantees that no unexpected interrupts will occur,
even if the application intentionally calls into the kernel.
Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
arch/tile/kernel/process.c | 9 ++++++
include/linux/cpu_isolated.h | 24 +++++++++++++++
include/linux/sched.h | 3 ++
include/uapi/linux/prctl.h | 5 ++++
kernel/context_tracking.c | 3 ++
kernel/sys.c | 8 +++++
kernel/time/Kconfig | 20 +++++++++++++
kernel/time/Makefile | 1 +
kernel/time/cpu_isolated.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
9 files changed, 144 insertions(+)
create mode 100644 include/linux/cpu_isolated.h
create mode 100644 kernel/time/cpu_isolated.c
diff --git a/arch/tile/kernel/process.c b/arch/tile/kernel/process.c
index e036c0aa9792..7db6f8386417 100644
--- a/arch/tile/kernel/process.c
+++ b/arch/tile/kernel/process.c
@@ -70,6 +70,15 @@ void arch_cpu_idle(void)
_cpu_idle();
}
+#ifdef CONFIG_CPU_ISOLATED
+void cpu_isolated_wait(void)
+{
+ set_current_state(TASK_INTERRUPTIBLE);
+ _cpu_idle();
+ set_current_state(TASK_RUNNING);
+}
+#endif
+
/*
* Release a thread_info structure
*/
diff --git a/include/linux/cpu_isolated.h b/include/linux/cpu_isolated.h
new file mode 100644
index 000000000000..a3d17360f7ae
--- /dev/null
+++ b/include/linux/cpu_isolated.h
@@ -0,0 +1,24 @@
+/*
+ * CPU isolation related global functions
+ */
+#ifndef _LINUX_CPU_ISOLATED_H
+#define _LINUX_CPU_ISOLATED_H
+
+#include <linux/tick.h>
+#include <linux/prctl.h>
+
+#ifdef CONFIG_CPU_ISOLATED
+static inline bool is_cpu_isolated(void)
+{
+ return tick_nohz_full_cpu(smp_processor_id()) &&
+ (current->cpu_isolated_flags & PR_CPU_ISOLATED_ENABLE);
+}
+
+extern void cpu_isolated_enter(void);
+extern void cpu_isolated_wait(void);
+#else
+static inline bool is_cpu_isolated(void) { return false; }
+static inline void cpu_isolated_enter(void) { }
+#endif
+
+#endif
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 04b5ada460b4..0bb248385d88 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1776,6 +1776,9 @@ struct task_struct {
unsigned long task_state_change;
#endif
int pagefault_disabled;
+#ifdef CONFIG_CPU_ISOLATED
+ unsigned int cpu_isolated_flags;
+#endif
/* CPU-specific state of this task */
struct thread_struct thread;
/*
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 31891d9535e2..edb40b6b84db 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -190,4 +190,9 @@ struct prctl_mm_map {
# define PR_FP_MODE_FR (1 << 0) /* 64b FP registers */
# define PR_FP_MODE_FRE (1 << 1) /* 32b compatibility */
+/* Enable/disable or query cpu_isolated mode for NO_HZ_FULL kernels. */
+#define PR_SET_CPU_ISOLATED 47
+#define PR_GET_CPU_ISOLATED 48
+# define PR_CPU_ISOLATED_ENABLE (1 << 0)
+
#endif /* _LINUX_PRCTL_H */
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 0a495ab35bc7..36b6509c3e2a 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -20,6 +20,7 @@
#include <linux/hardirq.h>
#include <linux/export.h>
#include <linux/kprobes.h>
+#include <linux/cpu_isolated.h>
#define CREATE_TRACE_POINTS
#include <trace/events/context_tracking.h>
@@ -99,6 +100,8 @@ void context_tracking_enter(enum ctx_state state)
* on the tick.
*/
if (state == CONTEXT_USER) {
+ if (is_cpu_isolated())
+ cpu_isolated_enter();
trace_user_enter(0);
vtime_user_enter(current);
}
diff --git a/kernel/sys.c b/kernel/sys.c
index 259fda25eb6b..c68417ff4800 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2267,6 +2267,14 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
case PR_GET_FP_MODE:
error = GET_FP_MODE(me);
break;
+#ifdef CONFIG_CPU_ISOLATED
+ case PR_SET_CPU_ISOLATED:
+ me->cpu_isolated_flags = arg2;
+ break;
+ case PR_GET_CPU_ISOLATED:
+ error = me->cpu_isolated_flags;
+ break;
+#endif
default:
error = -EINVAL;
break;
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 579ce1b929af..141969149994 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -195,5 +195,25 @@ config HIGH_RES_TIMERS
hardware is not capable then this option only increases
the size of the kernel image.
+config CPU_ISOLATED
+ bool "Provide hard CPU isolation from the kernel on demand"
+ depends on NO_HZ_FULL
+ help
+ Allow userspace processes to place themselves on nohz_full
+ cores and run prctl(PR_SET_CPU_ISOLATED) to "isolate"
+ themselves from the kernel. On return to userspace,
+ cpu-isolated tasks will first arrange that no future kernel
+ activity will interrupt the task while the task is running
+ in userspace. This "hard" isolation from the kernel is
+ required for userspace tasks that are running hard real-time
+ tasks in userspace, such as a 10 Gbit network driver in userspace.
+
+ Without this option, but with NO_HZ_FULL enabled, the kernel
+ will make a best-faith, "soft" effort to shield a single userspace
+ process from interrupts, but makes no guarantees.
+
+ You should say "N" unless you are intending to run a
+ high-performance userspace driver or similar task.
+
endmenu
endif
diff --git a/kernel/time/Makefile b/kernel/time/Makefile
index 49eca0beed32..984081cce974 100644
--- a/kernel/time/Makefile
+++ b/kernel/time/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_TICK_ONESHOT) += tick-oneshot.o tick-sched.o
obj-$(CONFIG_TIMER_STATS) += timer_stats.o
obj-$(CONFIG_DEBUG_FS) += timekeeping_debug.o
obj-$(CONFIG_TEST_UDELAY) += test_udelay.o
+obj-$(CONFIG_CPU_ISOLATED) += cpu_isolated.o
diff --git a/kernel/time/cpu_isolated.c b/kernel/time/cpu_isolated.c
new file mode 100644
index 000000000000..e27259f30caf
--- /dev/null
+++ b/kernel/time/cpu_isolated.c
@@ -0,0 +1,71 @@
+/*
+ * linux/kernel/time/cpu_isolated.c
+ *
+ * Implementation for cpu isolation.
+ *
+ * Distributed under GPLv2.
+ */
+
+#include <linux/mm.h>
+#include <linux/swap.h>
+#include <linux/vmstat.h>
+#include <linux/cpu_isolated.h>
+#include "tick-sched.h"
+
+/*
+ * Rather than continuously polling for the next_event in the
+ * tick_cpu_device, architectures can provide a method to save power
+ * by sleeping until an interrupt arrives.
+ */
+void __weak cpu_isolated_wait(void)
+{
+ cpu_relax();
+}
+
+/*
+ * We normally return immediately to userspace.
+ *
+ * In cpu_isolated mode we wait until no more interrupts are
+ * pending. Otherwise we nap with interrupts enabled and wait for the
+ * next interrupt to fire, then loop back and retry.
+ *
+ * Note that if you schedule two cpu_isolated processes on the same
+ * core, neither will ever leave the kernel, and one will have to be
+ * killed manually. Otherwise in situations where another process is
+ * in the runqueue on this cpu, this task will just wait for that
+ * other task to go idle before returning to user space.
+ */
+void cpu_isolated_enter(void)
+{
+ struct clock_event_device *dev =
+ __this_cpu_read(tick_cpu_device.evtdev);
+ struct task_struct *task = current;
+ unsigned long start = jiffies;
+ bool warned = false;
+
+ /* Drain the pagevecs to avoid unnecessary IPI flushes later. */
+ lru_add_drain();
+
+ /* Quieten the vmstat worker so it won't interrupt us. */
+ quiet_vmstat();
+
+ while (READ_ONCE(dev->next_event.tv64) != KTIME_MAX) {
+ if (!warned && (jiffies - start) >= (5 * HZ)) {
+ pr_warn("%s/%d: cpu %d: cpu_isolated task blocked for %ld seconds\n",
+ task->comm, task->pid, smp_processor_id(),
+ (jiffies - start) / HZ);
+ warned = true;
+ }
+ if (should_resched())
+ schedule();
+ if (test_thread_flag(TIF_SIGPENDING))
+ break;
+ cpu_isolated_wait();
+ }
+ if (warned) {
+ pr_warn("%s/%d: cpu %d: cpu_isolated task unblocked after %ld seconds\n",
+ task->comm, task->pid, smp_processor_id(),
+ (jiffies - start) / HZ);
+ dump_stack();
+ }
+}
--
2.1.2
^ permalink raw reply related
* [PATCH v5 3/6] cpu_isolated: support PR_CPU_ISOLATED_STRICT mode
From: Chris Metcalf @ 2015-07-28 19:49 UTC (permalink / raw)
To: Gilad Ben Yossef, Steven Rostedt, Ingo Molnar, Peter Zijlstra,
Andrew Morton, Rik van Riel, Tejun Heo, Frederic Weisbecker,
Thomas Gleixner, Paul E. McKenney, Christoph Lameter,
Viresh Kumar, Catalin Marinas, Will Deacon, linux-doc, linux-api,
linux-kernel
Cc: Chris Metcalf
In-Reply-To: <1438112980-9981-1-git-send-email-cmetcalf@ezchip.com>
With cpu_isolated mode, the task is in principle guaranteed not to be
interrupted by the kernel, but only if it behaves. In particular,
if it enters the kernel via system call, page fault, or any of a
number of other synchronous traps, it may be unexpectedly exposed
to long latencies. Add a simple flag that puts the process into
a state where any such kernel entry is fatal.
To allow the state to be entered and exited, we ignore the prctl()
syscall so that we can clear the bit again later, and we ignore
exit/exit_group to allow exiting the task without a pointless signal
killing you as you try to do so.
This change adds the syscall-detection hooks only for x86, arm64,
and tile.
The signature of context_tracking_exit() changes to report whether
we, in fact, are exiting back to user space, so that we can track
user exceptions properly separately from other kernel entries.
Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
---
Note: Andy Lutomirski points out that improvements are coming
to the context_tracking code to make it more robust, which may
mean that some of the code suggested here for context_tracking
may not be necessary. I am keeping it in the series for now since
it is required for it to work based on 4.2-rc3.
arch/arm64/kernel/ptrace.c | 5 +++++
arch/tile/kernel/ptrace.c | 5 ++++-
arch/x86/kernel/ptrace.c | 2 ++
include/linux/context_tracking.h | 11 ++++++++---
include/linux/cpu_isolated.h | 16 ++++++++++++++++
include/uapi/linux/prctl.h | 1 +
kernel/context_tracking.c | 9 ++++++---
kernel/time/cpu_isolated.c | 38 ++++++++++++++++++++++++++++++++++++++
8 files changed, 80 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index d882b833dbdb..ff83968ab4d4 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -37,6 +37,7 @@
#include <linux/regset.h>
#include <linux/tracehook.h>
#include <linux/elf.h>
+#include <linux/cpu_isolated.h>
#include <asm/compat.h>
#include <asm/debug-monitors.h>
@@ -1150,6 +1151,10 @@ static void tracehook_report_syscall(struct pt_regs *regs,
asmlinkage int syscall_trace_enter(struct pt_regs *regs)
{
+ /* Ensure we report cpu_isolated violations in all circumstances. */
+ if (test_thread_flag(TIF_NOHZ) && cpu_isolated_strict())
+ cpu_isolated_syscall(regs->syscallno);
+
/* Do the secure computing check first; failures should be fast. */
if (secure_computing() == -1)
return -1;
diff --git a/arch/tile/kernel/ptrace.c b/arch/tile/kernel/ptrace.c
index f84eed8243da..e54256c54311 100644
--- a/arch/tile/kernel/ptrace.c
+++ b/arch/tile/kernel/ptrace.c
@@ -259,8 +259,11 @@ int do_syscall_trace_enter(struct pt_regs *regs)
* If TIF_NOHZ is set, we are required to call user_exit() before
* doing anything that could touch RCU.
*/
- if (work & _TIF_NOHZ)
+ if (work & _TIF_NOHZ) {
user_exit();
+ if (cpu_isolated_strict())
+ cpu_isolated_syscall(regs->regs[TREG_SYSCALL_NR]);
+ }
if (work & _TIF_SYSCALL_TRACE) {
if (tracehook_report_syscall_entry(regs))
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 9be72bc3613f..e5aec57e8e25 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1479,6 +1479,8 @@ unsigned long syscall_trace_enter_phase1(struct pt_regs *regs, u32 arch)
if (work & _TIF_NOHZ) {
user_exit();
work &= ~_TIF_NOHZ;
+ if (cpu_isolated_strict())
+ cpu_isolated_syscall(regs->orig_ax);
}
#ifdef CONFIG_SECCOMP
diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index b96bd299966f..590414ef2bf1 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -3,6 +3,7 @@
#include <linux/sched.h>
#include <linux/vtime.h>
+#include <linux/cpu_isolated.h>
#include <linux/context_tracking_state.h>
#include <asm/ptrace.h>
@@ -11,7 +12,7 @@
extern void context_tracking_cpu_set(int cpu);
extern void context_tracking_enter(enum ctx_state state);
-extern void context_tracking_exit(enum ctx_state state);
+extern bool context_tracking_exit(enum ctx_state state);
extern void context_tracking_user_enter(void);
extern void context_tracking_user_exit(void);
@@ -35,8 +36,12 @@ static inline enum ctx_state exception_enter(void)
return 0;
prev_ctx = this_cpu_read(context_tracking.state);
- if (prev_ctx != CONTEXT_KERNEL)
- context_tracking_exit(prev_ctx);
+ if (prev_ctx != CONTEXT_KERNEL) {
+ if (context_tracking_exit(prev_ctx)) {
+ if (cpu_isolated_strict())
+ cpu_isolated_exception();
+ }
+ }
return prev_ctx;
}
diff --git a/include/linux/cpu_isolated.h b/include/linux/cpu_isolated.h
index a3d17360f7ae..b0f1c2669b2f 100644
--- a/include/linux/cpu_isolated.h
+++ b/include/linux/cpu_isolated.h
@@ -15,10 +15,26 @@ static inline bool is_cpu_isolated(void)
}
extern void cpu_isolated_enter(void);
+extern void cpu_isolated_syscall(int nr);
+extern void cpu_isolated_exception(void);
extern void cpu_isolated_wait(void);
#else
static inline bool is_cpu_isolated(void) { return false; }
static inline void cpu_isolated_enter(void) { }
+static inline void cpu_isolated_syscall(int nr) { }
+static inline void cpu_isolated_exception(void) { }
#endif
+static inline bool cpu_isolated_strict(void)
+{
+#ifdef CONFIG_CPU_ISOLATED
+ if (tick_nohz_full_cpu(smp_processor_id()) &&
+ (current->cpu_isolated_flags &
+ (PR_CPU_ISOLATED_ENABLE | PR_CPU_ISOLATED_STRICT)) ==
+ (PR_CPU_ISOLATED_ENABLE | PR_CPU_ISOLATED_STRICT))
+ return true;
+#endif
+ return false;
+}
+
#endif
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index edb40b6b84db..0c11238a84fb 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -194,5 +194,6 @@ struct prctl_mm_map {
#define PR_SET_CPU_ISOLATED 47
#define PR_GET_CPU_ISOLATED 48
# define PR_CPU_ISOLATED_ENABLE (1 << 0)
+# define PR_CPU_ISOLATED_STRICT (1 << 1)
#endif /* _LINUX_PRCTL_H */
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 36b6509c3e2a..c740850eea11 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -147,15 +147,16 @@ NOKPROBE_SYMBOL(context_tracking_user_enter);
* This call supports re-entrancy. This way it can be called from any exception
* handler without needing to know if we came from userspace or not.
*/
-void context_tracking_exit(enum ctx_state state)
+bool context_tracking_exit(enum ctx_state state)
{
unsigned long flags;
+ bool from_user = false;
if (!context_tracking_is_enabled())
- return;
+ return false;
if (in_interrupt())
- return;
+ return false;
local_irq_save(flags);
if (!context_tracking_recursion_enter())
@@ -169,6 +170,7 @@ void context_tracking_exit(enum ctx_state state)
*/
rcu_user_exit();
if (state == CONTEXT_USER) {
+ from_user = true;
vtime_user_exit(current);
trace_user_exit(0);
}
@@ -178,6 +180,7 @@ void context_tracking_exit(enum ctx_state state)
context_tracking_recursion_exit();
out_irq_restore:
local_irq_restore(flags);
+ return from_user;
}
NOKPROBE_SYMBOL(context_tracking_exit);
EXPORT_SYMBOL_GPL(context_tracking_exit);
diff --git a/kernel/time/cpu_isolated.c b/kernel/time/cpu_isolated.c
index e27259f30caf..d30bf3852897 100644
--- a/kernel/time/cpu_isolated.c
+++ b/kernel/time/cpu_isolated.c
@@ -10,6 +10,7 @@
#include <linux/swap.h>
#include <linux/vmstat.h>
#include <linux/cpu_isolated.h>
+#include <asm/unistd.h>
#include "tick-sched.h"
/*
@@ -69,3 +70,40 @@ void cpu_isolated_enter(void)
dump_stack();
}
}
+
+static void kill_cpu_isolated_strict_task(void)
+{
+ dump_stack();
+ current->cpu_isolated_flags &= ~PR_CPU_ISOLATED_ENABLE;
+ send_sig(SIGKILL, current, 1);
+}
+
+/*
+ * This routine is called from syscall entry (with the syscall number
+ * passed in) if the _STRICT flag is set.
+ */
+void cpu_isolated_syscall(int syscall)
+{
+ /* Ignore prctl() syscalls or any task exit. */
+ switch (syscall) {
+ case __NR_prctl:
+ case __NR_exit:
+ case __NR_exit_group:
+ return;
+ }
+
+ pr_warn("%s/%d: cpu_isolated strict mode violated by syscall %d\n",
+ current->comm, current->pid, syscall);
+ kill_cpu_isolated_strict_task();
+}
+
+/*
+ * This routine is called from any userspace exception if the _STRICT
+ * flag is set.
+ */
+void cpu_isolated_exception(void)
+{
+ pr_warn("%s/%d: cpu_isolated strict mode violated by exception\n",
+ current->comm, current->pid);
+ kill_cpu_isolated_strict_task();
+}
--
2.1.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox