* [PATCH v7 8/9] nvmem: sunxi: Move the SID driver to the nvmem framework
From: Srinivas Kandagatla @ 2015-07-10 9:45 UTC (permalink / raw)
To: linux-arm-kernel, Greg Kroah-Hartman
Cc: Rob Herring, Kumar Gala, Mark Brown, s.hauer, linux-api,
linux-kernel, devicetree, linux-arm-msm, arnd, sboyd,
pantelis.antoniou, mporter, stefan.wahren, wxt,
Srinivas Kandagatla, Maxime Ripard
In-Reply-To: <1436521427-10568-1-git-send-email-srinivas.kandagatla@linaro.org>
From: Maxime Ripard <maxime.ripard@free-electrons.com>
Now that we have the nvmem framework, we can consolidate the common
driver code. Move the driver to the framework, and hopefully, it will
fix the sysfs file creation race.
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
[srinivas.kandagatla: Moved to regmap based EEPROM framework]
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Documentation/ABI/testing/sysfs-driver-sunxi-sid | 22 ---
.../bindings/misc/allwinner,sunxi-sid.txt | 17 ---
.../bindings/nvmem/allwinner,sunxi-sid.txt | 21 +++
drivers/misc/eeprom/Kconfig | 13 --
drivers/misc/eeprom/Makefile | 1 -
drivers/misc/eeprom/sunxi_sid.c | 156 --------------------
drivers/nvmem/Kconfig | 11 ++
drivers/nvmem/Makefile | 2 +
drivers/nvmem/sunxi_sid.c | 160 +++++++++++++++++++++
9 files changed, 194 insertions(+), 209 deletions(-)
delete mode 100644 Documentation/ABI/testing/sysfs-driver-sunxi-sid
delete mode 100644 Documentation/devicetree/bindings/misc/allwinner,sunxi-sid.txt
create mode 100644 Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
delete mode 100644 drivers/misc/eeprom/sunxi_sid.c
create mode 100644 drivers/nvmem/sunxi_sid.c
diff --git a/Documentation/ABI/testing/sysfs-driver-sunxi-sid b/Documentation/ABI/testing/sysfs-driver-sunxi-sid
deleted file mode 100644
index ffb9536..0000000
--- a/Documentation/ABI/testing/sysfs-driver-sunxi-sid
+++ /dev/null
@@ -1,22 +0,0 @@
-What: /sys/devices/*/<our-device>/eeprom
-Date: August 2013
-Contact: Oliver Schinagl <oliver@schinagl.nl>
-Description: read-only access to the SID (Security-ID) on current
- A-series SoC's from Allwinner. Currently supports A10, A10s, A13
- and A20 CPU's. The earlier A1x series of SoCs exports 16 bytes,
- whereas the newer A20 SoC exposes 512 bytes split into sections.
- Besides the 16 bytes of SID, there's also an SJTAG area,
- HDMI-HDCP key and some custom keys. Below a quick overview, for
- details see the user manual:
- 0x000 128 bit root-key (sun[457]i)
- 0x010 128 bit boot-key (sun7i)
- 0x020 64 bit security-jtag-key (sun7i)
- 0x028 16 bit key configuration (sun7i)
- 0x02b 16 bit custom-vendor-key (sun7i)
- 0x02c 320 bit low general key (sun7i)
- 0x040 32 bit read-control access (sun7i)
- 0x064 224 bit low general key (sun7i)
- 0x080 2304 bit HDCP-key (sun7i)
- 0x1a0 768 bit high general key (sun7i)
-Users: any user space application which wants to read the SID on
- Allwinner's A-series of CPU's.
diff --git a/Documentation/devicetree/bindings/misc/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/misc/allwinner,sunxi-sid.txt
deleted file mode 100644
index fabdf64..0000000
--- a/Documentation/devicetree/bindings/misc/allwinner,sunxi-sid.txt
+++ /dev/null
@@ -1,17 +0,0 @@
-Allwinner sunxi-sid
-
-Required properties:
-- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
-- reg: Should contain registers location and length
-
-Example for sun4i:
- sid@01c23800 {
- compatible = "allwinner,sun4i-a10-sid";
- reg = <0x01c23800 0x10>
- };
-
-Example for sun7i:
- sid@01c23800 {
- compatible = "allwinner,sun7i-a20-sid";
- reg = <0x01c23800 0x200>
- };
diff --git a/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
new file mode 100644
index 0000000..d543ed3
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/allwinner,sunxi-sid.txt
@@ -0,0 +1,21 @@
+Allwinner sunxi-sid
+
+Required properties:
+- compatible: "allwinner,sun4i-a10-sid" or "allwinner,sun7i-a20-sid"
+- reg: Should contain registers location and length
+
+= Data cells =
+Are child nodes of qfprom, bindings of which as described in
+bindings/nvmem/nvmem.txt
+
+Example for sun4i:
+ sid@01c23800 {
+ compatible = "allwinner,sun4i-a10-sid";
+ reg = <0x01c23800 0x10>
+ };
+
+Example for sun7i:
+ sid@01c23800 {
+ compatible = "allwinner,sun7i-a20-sid";
+ reg = <0x01c23800 0x200>
+ };
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index 9536852f..04f2e1f 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -96,17 +96,4 @@ config EEPROM_DIGSY_MTC_CFG
If unsure, say N.
-config EEPROM_SUNXI_SID
- tristate "Allwinner sunxi security ID support"
- depends on ARCH_SUNXI && SYSFS
- help
- This is a driver for the 'security ID' available on various Allwinner
- devices.
-
- Due to the potential risks involved with changing e-fuses,
- this driver is read-only.
-
- This driver can also be built as a module. If so, the module
- will be called sunxi_sid.
-
endmenu
diff --git a/drivers/misc/eeprom/Makefile b/drivers/misc/eeprom/Makefile
index 9507aec..fc1e81d 100644
--- a/drivers/misc/eeprom/Makefile
+++ b/drivers/misc/eeprom/Makefile
@@ -4,5 +4,4 @@ obj-$(CONFIG_EEPROM_LEGACY) += eeprom.o
obj-$(CONFIG_EEPROM_MAX6875) += max6875.o
obj-$(CONFIG_EEPROM_93CX6) += eeprom_93cx6.o
obj-$(CONFIG_EEPROM_93XX46) += eeprom_93xx46.o
-obj-$(CONFIG_EEPROM_SUNXI_SID) += sunxi_sid.o
obj-$(CONFIG_EEPROM_DIGSY_MTC_CFG) += digsy_mtc_eeprom.o
diff --git a/drivers/misc/eeprom/sunxi_sid.c b/drivers/misc/eeprom/sunxi_sid.c
deleted file mode 100644
index 8385177..0000000
--- a/drivers/misc/eeprom/sunxi_sid.c
+++ /dev/null
@@ -1,156 +0,0 @@
-/*
- * Copyright (c) 2013 Oliver Schinagl <oliver@schinagl.nl>
- * http://www.linux-sunxi.org
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * This driver exposes the Allwinner security ID, efuses exported in byte-
- * sized chunks.
- */
-
-#include <linux/compiler.h>
-#include <linux/device.h>
-#include <linux/err.h>
-#include <linux/export.h>
-#include <linux/fs.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/kobject.h>
-#include <linux/module.h>
-#include <linux/of_device.h>
-#include <linux/platform_device.h>
-#include <linux/random.h>
-#include <linux/slab.h>
-#include <linux/stat.h>
-#include <linux/sysfs.h>
-#include <linux/types.h>
-
-#define DRV_NAME "sunxi-sid"
-
-struct sunxi_sid_data {
- void __iomem *reg_base;
- unsigned int keysize;
-};
-
-/* We read the entire key, due to a 32 bit read alignment requirement. Since we
- * want to return the requested byte, this results in somewhat slower code and
- * uses 4 times more reads as needed but keeps code simpler. Since the SID is
- * only very rarely probed, this is not really an issue.
- */
-static u8 sunxi_sid_read_byte(const struct sunxi_sid_data *sid_data,
- const unsigned int offset)
-{
- u32 sid_key;
-
- if (offset >= sid_data->keysize)
- return 0;
-
- sid_key = ioread32be(sid_data->reg_base + round_down(offset, 4));
- sid_key >>= (offset % 4) * 8;
-
- return sid_key; /* Only return the last byte */
-}
-
-static ssize_t sid_read(struct file *fd, struct kobject *kobj,
- struct bin_attribute *attr, char *buf,
- loff_t pos, size_t size)
-{
- struct platform_device *pdev;
- struct sunxi_sid_data *sid_data;
- int i;
-
- pdev = to_platform_device(kobj_to_dev(kobj));
- sid_data = platform_get_drvdata(pdev);
-
- if (pos < 0 || pos >= sid_data->keysize)
- return 0;
- if (size > sid_data->keysize - pos)
- size = sid_data->keysize - pos;
-
- for (i = 0; i < size; i++)
- buf[i] = sunxi_sid_read_byte(sid_data, pos + i);
-
- return i;
-}
-
-static struct bin_attribute sid_bin_attr = {
- .attr = { .name = "eeprom", .mode = S_IRUGO, },
- .read = sid_read,
-};
-
-static int sunxi_sid_remove(struct platform_device *pdev)
-{
- device_remove_bin_file(&pdev->dev, &sid_bin_attr);
- dev_dbg(&pdev->dev, "driver unloaded\n");
-
- return 0;
-}
-
-static const struct of_device_id sunxi_sid_of_match[] = {
- { .compatible = "allwinner,sun4i-a10-sid", .data = (void *)16},
- { .compatible = "allwinner,sun7i-a20-sid", .data = (void *)512},
- {/* sentinel */},
-};
-MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
-
-static int sunxi_sid_probe(struct platform_device *pdev)
-{
- struct sunxi_sid_data *sid_data;
- struct resource *res;
- const struct of_device_id *of_dev_id;
- u8 *entropy;
- unsigned int i;
-
- sid_data = devm_kzalloc(&pdev->dev, sizeof(struct sunxi_sid_data),
- GFP_KERNEL);
- if (!sid_data)
- return -ENOMEM;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- sid_data->reg_base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(sid_data->reg_base))
- return PTR_ERR(sid_data->reg_base);
-
- of_dev_id = of_match_device(sunxi_sid_of_match, &pdev->dev);
- if (!of_dev_id)
- return -ENODEV;
- sid_data->keysize = (int)of_dev_id->data;
-
- platform_set_drvdata(pdev, sid_data);
-
- sid_bin_attr.size = sid_data->keysize;
- if (device_create_bin_file(&pdev->dev, &sid_bin_attr))
- return -ENODEV;
-
- entropy = kzalloc(sizeof(u8) * sid_data->keysize, GFP_KERNEL);
- for (i = 0; i < sid_data->keysize; i++)
- entropy[i] = sunxi_sid_read_byte(sid_data, i);
- add_device_randomness(entropy, sid_data->keysize);
- kfree(entropy);
-
- dev_dbg(&pdev->dev, "loaded\n");
-
- return 0;
-}
-
-static struct platform_driver sunxi_sid_driver = {
- .probe = sunxi_sid_probe,
- .remove = sunxi_sid_remove,
- .driver = {
- .name = DRV_NAME,
- .of_match_table = sunxi_sid_of_match,
- },
-};
-module_platform_driver(sunxi_sid_driver);
-
-MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>");
-MODULE_DESCRIPTION("Allwinner sunxi security id driver");
-MODULE_LICENSE("GPL");
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index fa85805..8db2978 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -25,4 +25,15 @@ config QCOM_QFPROM
This driver can also be built as a module. If so, the module
will be called nvmem_qfprom.
+config NVMEM_SUNXI_SID
+ tristate "Allwinner SoCs SID support"
+ depends on ARCH_SUNXI
+ select REGMAP_MMIO
+ help
+ This is a driver for the 'security ID' available on various Allwinner
+ devices.
+
+ This driver can also be built as a module. If so, the module
+ will be called nvmem_sunxi_sid.
+
endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index ff44fe9..4328b93 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -8,3 +8,5 @@ nvmem_core-y := core.o
# Devices
obj-$(CONFIG_QCOM_QFPROM) += nvmem_qfprom.o
nvmem_qfprom-y := qfprom.o
+obj-$(CONFIG_NVMEM_SUNXI_SID) += nvmem_sunxi_sid.o
+nvmem_sunxi_sid-y := sunxi_sid.o
diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
new file mode 100644
index 0000000..1c3ff2e6
--- /dev/null
+++ b/drivers/nvmem/sunxi_sid.c
@@ -0,0 +1,160 @@
+/*
+ * Allwinner sunXi SoCs Security ID support.
+ *
+ * Copyright (c) 2013 Oliver Schinagl <oliver@schinagl.nl>
+ * Copyright (C) 2014 Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+
+#include <linux/platform_device.h>
+#include <linux/nvmem-provider.h>
+#include <linux/slab.h>
+#include <linux/regmap.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+
+
+static struct nvmem_config econfig = {
+ .name = "sunxi-sid",
+ .read_only = true,
+ .owner = THIS_MODULE,
+};
+
+struct sunxi_sid {
+ void __iomem *base;
+};
+
+/* We read the entire key, due to a 32 bit read alignment requirement. Since we
+ * want to return the requested byte, this results in somewhat slower code and
+ * uses 4 times more reads as needed but keeps code simpler. Since the SID is
+ * only very rarely probed, this is not really an issue.
+ */
+static u8 sunxi_sid_read_byte(const struct sunxi_sid *sid,
+ const unsigned int offset)
+{
+ u32 sid_key;
+
+ sid_key = ioread32be(sid->base + round_down(offset, 4));
+ sid_key >>= (offset % 4) * 8;
+
+ return sid_key; /* Only return the last byte */
+}
+
+static int sunxi_sid_read(void *context,
+ const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ struct sunxi_sid *sid = context;
+ unsigned int offset = *(u32 *)reg;
+ u8 *buf = val;
+
+ while (val_size) {
+ *buf++ = sunxi_sid_read_byte(sid, offset);
+ val_size--;
+ offset++;
+ }
+
+ return 0;
+}
+
+static int sunxi_sid_write(void *context, const void *data, size_t count)
+{
+ /* Unimplemented, dummy to keep regmap core happy */
+ return 0;
+}
+
+static struct regmap_bus sunxi_sid_bus = {
+ .read = sunxi_sid_read,
+ .write = sunxi_sid_write,
+ .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+ .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+};
+
+static bool sunxi_sid_writeable_reg(struct device *dev, unsigned int reg)
+{
+ return false;
+}
+
+static struct regmap_config sunxi_sid_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .reg_stride = 1,
+ .writeable_reg = sunxi_sid_writeable_reg,
+};
+
+static int sunxi_sid_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct nvmem_device *nvmem;
+ struct regmap *regmap;
+ struct sunxi_sid *sid;
+
+ sid = devm_kzalloc(dev, sizeof(*sid), GFP_KERNEL);
+ if (!sid)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ sid->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(sid->base))
+ return PTR_ERR(sid->base);
+
+ sunxi_sid_regmap_config.max_register = resource_size(res) - 1;
+
+ regmap = devm_regmap_init(dev, &sunxi_sid_bus, sid,
+ &sunxi_sid_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(dev, "regmap init failed\n");
+ return PTR_ERR(regmap);
+ }
+ econfig.dev = dev;
+ nvmem = nvmem_register(&econfig);
+ if (IS_ERR(nvmem))
+ return PTR_ERR(nvmem);
+
+ platform_set_drvdata(pdev, nvmem);
+
+ return 0;
+}
+
+static int sunxi_sid_remove(struct platform_device *pdev)
+{
+ struct nvmem_device *nvmem = platform_get_drvdata(pdev);
+
+ return nvmem_unregister(nvmem);
+}
+
+static const struct of_device_id sunxi_sid_of_match[] = {
+ { .compatible = "allwinner,sun4i-a10-sid" },
+ { .compatible = "allwinner,sun7i-a20-sid" },
+ {/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
+
+static struct platform_driver sunxi_sid_driver = {
+ .probe = sunxi_sid_probe,
+ .remove = sunxi_sid_remove,
+ .driver = {
+ .name = "eeprom-sunxi-sid",
+ .of_match_table = sunxi_sid_of_match,
+ },
+};
+module_platform_driver(sunxi_sid_driver);
+
+MODULE_AUTHOR("Oliver Schinagl <oliver@schinagl.nl>");
+MODULE_DESCRIPTION("Allwinner sunxi security id driver");
+MODULE_LICENSE("GPL");
--
1.9.1
^ permalink raw reply related
* [PATCH v7 9/9] nvmem: Add to MAINTAINERS for nvmem framework
From: Srinivas Kandagatla @ 2015-07-10 9:46 UTC (permalink / raw)
To: linux-arm-kernel, Greg Kroah-Hartman
Cc: Rob Herring, Kumar Gala, Mark Brown, s.hauer, linux-api,
linux-kernel, devicetree, linux-arm-msm, arnd, sboyd,
pantelis.antoniou, mporter, stefan.wahren, wxt,
Srinivas Kandagatla
In-Reply-To: <1436521427-10568-1-git-send-email-srinivas.kandagatla@linaro.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 818 bytes --]
This patch adds MAINTAINERS to nvmem framework.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8133cef..b69c11f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7263,6 +7263,15 @@ S: Supported
F: drivers/block/nvme*
F: include/linux/nvme.h
+NVMEM FRAMEWORK
+M: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
+M: Maxime Ripard <maxime.ripard@free-electrons.com>
+S: Maintained
+F: drivers/nvmem/
+F: Documentation/devicetree/bindings/nvmem/
+F: include/linux/nvmem-provider.h
+F: include/linux/nvmem-consumer.h
+
NXP-NCI NFC DRIVER
M: Clément Perrochaud <clement.perrochaud@effinnov.com>
R: Charles Gorand <charles.gorand@effinnov.com>
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v7 1/9] nvmem: Add a simple NVMEM framework for nvmem providers
From: Joe Perches @ 2015-07-10 10:29 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Greg Kroah-Hartman, Rob Herring, Kumar Gala, Mark Brown,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, arnd-r2nGTMty4D4,
sboyd-sgV2jX0FEOL9JmXXK+q4OQ,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w,
mporter-OWPKS81ov/FWk0Htik3J/w, stefan.wahren-eS4NqCHxEME,
wxt-TNX95d0MmH7DzftRWevZcw, Maxime Ripard
In-Reply-To: <1436521486-10682-1-git-send-email-srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Fri, 2015-07-10 at 10:44 +0100, Srinivas Kandagatla wrote:
> This patch adds just providers part of the framework just to enable easy
> review.
Trivial notes:
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> +struct nvmem_device {
> + const char *name;
> + struct regmap *regmap;
[]
> +struct nvmem_cell {
> + const char *name;
> + int offset;
> + int bytes;
It'd be nicer to use consistent indentation for *name
in nvmem_cell
> +static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *attr,
> + char *buf, loff_t pos, size_t count)
> +{
[]
> + count = count/nvmem->word_size * nvmem->word_size;
maybe rounddown() ?
> +static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *attr,
> + char *buf, loff_t pos, size_t count)
> +{
> + count = count/nvmem->word_size * nvmem->word_size;
and rounddown() here too ?
> +static struct nvmem_device *of_nvmem_find(struct device_node *nvmem_np)
> +{
> + struct device *d;
> +
> + if (!nvmem_np)
> + return NULL;
> +
> + d = bus_find_device(&nvmem_bus_type, NULL, nvmem_np, of_nvmem_match);
> +
> + return d ? to_nvmem_device(d) : NULL;
> +}
Perhaps more common as
d = bus_find_device(&nvmem_bus_type, NULL, nvmem_np, of_nvmem_match);
if (!d)
return NULL;
return to_nvmem_device(d);
}
> +struct nvmem_device *nvmem_register(struct nvmem_config *config)
> +{
[]
> + nvmem->read_only = nvmem->dev.of_node ?
> + of_property_read_bool(nvmem->dev.of_node,
> + "read-only") :
> + config->read_only;
odd indentation.
Normally, "read-only" would be aligned with nvmem->dev.of_node,
> + dev_dbg(&nvmem->dev, "Registering nvmem device %s\n",
> + dev_name(&nvmem->dev));
Isn't this going to have duplicated dev_name prefix and postfix?
> + if (device_create_bin_file(&nvmem->dev,
> + nvmem->read_only ? &bin_attr_ro_nvmem :
> + &bin_attr_rw_nvmem))
> + dev_info(&nvmem->dev, "Failed to create sysfs binary file\n");
Is the KERN_LEVEL correct?
Maybe dev_err/notice/warn/dbg?
^ permalink raw reply
* Re: [PATCH v7 1/9] nvmem: Add a simple NVMEM framework for nvmem providers
From: Srinivas Kandagatla @ 2015-07-10 10:39 UTC (permalink / raw)
To: Joe Perches
Cc: linux-arm-kernel, Greg Kroah-Hartman, Rob Herring, Kumar Gala,
Mark Brown, s.hauer, linux-api, linux-kernel, devicetree,
linux-arm-msm, arnd, sboyd, pantelis.antoniou, mporter,
stefan.wahren, wxt, Maxime Ripard
In-Reply-To: <1436524145.24866.21.camel@perches.com>
Thanks for quick review,
On 10/07/15 11:29, Joe Perches wrote:
> On Fri, 2015-07-10 at 10:44 +0100, Srinivas Kandagatla wrote:
>> This patch adds just providers part of the framework just to enable easy
>> review.
>
> Trivial notes:
Will fix all them.
--srini
>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>
>> +struct nvmem_device {
>> + const char *name;
>> + struct regmap *regmap;
> []
>> +struct nvmem_cell {
>> + const char *name;
>> + int offset;
>> + int bytes;
>
> It'd be nicer to use consistent indentation for *name
> in nvmem_cell
>
>> +static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
>> + struct bin_attribute *attr,
>> + char *buf, loff_t pos, size_t count)
>> +{
> []
>> + count = count/nvmem->word_size * nvmem->word_size;
>
> maybe rounddown() ?
>
>> +static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
>> + struct bin_attribute *attr,
>> + char *buf, loff_t pos, size_t count)
>> +{
>
>> + count = count/nvmem->word_size * nvmem->word_size;
>
> and rounddown() here too ?
>
>> +static struct nvmem_device *of_nvmem_find(struct device_node *nvmem_np)
>> +{
>> + struct device *d;
>> +
>> + if (!nvmem_np)
>> + return NULL;
>> +
>> + d = bus_find_device(&nvmem_bus_type, NULL, nvmem_np, of_nvmem_match);
>> +
>> + return d ? to_nvmem_device(d) : NULL;
>> +}
>
> Perhaps more common as
>
> d = bus_find_device(&nvmem_bus_type, NULL, nvmem_np, of_nvmem_match);
> if (!d)
> return NULL;
>
> return to_nvmem_device(d);
> }
>
>> +struct nvmem_device *nvmem_register(struct nvmem_config *config)
>> +{
> []
>> + nvmem->read_only = nvmem->dev.of_node ?
>> + of_property_read_bool(nvmem->dev.of_node,
>> + "read-only") :
>> + config->read_only;
>
> odd indentation.
> Normally, "read-only" would be aligned with nvmem->dev.of_node,
>
>> + dev_dbg(&nvmem->dev, "Registering nvmem device %s\n",
>> + dev_name(&nvmem->dev));
>
> Isn't this going to have duplicated dev_name prefix and postfix?
>
>> + if (device_create_bin_file(&nvmem->dev,
>> + nvmem->read_only ? &bin_attr_ro_nvmem :
>> + &bin_attr_rw_nvmem))
>> + dev_info(&nvmem->dev, "Failed to create sysfs binary file\n");
>
> Is the KERN_LEVEL correct?
> Maybe dev_err/notice/warn/dbg?
>
>
^ permalink raw reply
* Re: [PATCH v7 2/9] nvmem: Add a simple NVMEM framework for consumers
From: Joe Perches @ 2015-07-10 10:42 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Greg Kroah-Hartman, Rob Herring, Kumar Gala, Mark Brown,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, arnd-r2nGTMty4D4,
sboyd-sgV2jX0FEOL9JmXXK+q4OQ,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w,
mporter-OWPKS81ov/FWk0Htik3J/w, stefan.wahren-eS4NqCHxEME,
wxt-TNX95d0MmH7DzftRWevZcw, Maxime Ripard
In-Reply-To: <1436521495-10728-1-git-send-email-srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Fri, 2015-07-10 at 10:44 +0100, Srinivas Kandagatla wrote:
> This patch adds just consumers part of the framework just to enable easy
> review.
Trivial notes:
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> @@ -334,6 +335,7 @@ struct nvmem_device *nvmem_register(struct nvmem_config *config)
> if (config->cells)
> nvmem_add_cells(nvmem, config);
>
> +
superfluous newline
> +static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
> +{
> + struct nvmem_cell *cell = NULL;
> + struct nvmem_device *nvmem;
> +
> + nvmem = __nvmem_device_get(NULL, &cell, cell_id);
> + if (IS_ERR(nvmem))
> + return ERR_CAST(nvmem);
> +
> +
extra blank line here too
> +/**
> + * nvmem_cell_read() - Read a given nvmem cell
> + *
> + * @cell: nvmem cell to be read.
> + * @len: pointer to length of cell which will be populated on successful read.
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a char * bufffer. The buffer should be freed by the consumer with a
One too many f's in buffer, it's returning a void *
> + * kfree().
> + */
> +void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len)
> +{
> + struct nvmem_device *nvmem = cell->nvmem;
> + u8 *buf;
[]
> + return buf;
> +}
> +EXPORT_SYMBOL_GPL(nvmem_cell_read);
[]
> +/**
> + * nvmem_cell_write() - Write to a given nvmem cell
> + *
> + * @cell: nvmem cell to be written.
> + * @buf: Buffer to be written.
> + * @len: length of buffer to be written to nvmem cell.
> + *
> + * The return value will be an length of bytes written or non zero on failure.
less than zero or maybe negative on failure?
> + */
> +int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len)
> +{
> + struct nvmem_device *nvmem = cell->nvmem;
> + int rc;
> + void *wbuf = buf;
This variable and assignment seems unnecessary as buf could be reused.
> +
> + if (!nvmem || !nvmem->regmap || nvmem->read_only ||
> + (cell->bit_offset == 0 && len != cell->bytes))
> + return -EINVAL;
> +
> + if (cell->bit_offset || cell->nbits) {
> + wbuf = nvmem_cell_prepare_write_buffer(cell, buf, len);
> + if (IS_ERR(wbuf))
> + return PTR_ERR(wbuf);
> + }
> +
> + rc = regmap_raw_write(nvmem->regmap, cell->offset, wbuf, cell->bytes);
> +
> + /* free the tmp buffer */
> + if (cell->bit_offset)
> + kfree(wbuf);
> +
> + if (IS_ERR_VALUE(rc))
> + return rc;
> +
> + return len;
> +}
> +EXPORT_SYMBOL_GPL(nvmem_cell_write);
^ permalink raw reply
* Re: [PATCH v7 3/9] nvmem: Add nvmem_device based consumer apis.
From: Joe Perches @ 2015-07-10 10:49 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Greg Kroah-Hartman, Rob Herring, Kumar Gala, Mark Brown,
s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, arnd-r2nGTMty4D4,
sboyd-sgV2jX0FEOL9JmXXK+q4OQ,
pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w,
mporter-OWPKS81ov/FWk0Htik3J/w, stefan.wahren-eS4NqCHxEME,
wxt-TNX95d0MmH7DzftRWevZcw
In-Reply-To: <1436521505-10779-1-git-send-email-srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Fri, 2015-07-10 at 10:45 +0100, Srinivas Kandagatla wrote:
> This patch adds read/write apis which are based on nvmem_device.
More trivia:
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> +static struct nvmem_device *nvmem_find(const char *name)
> +{
> + struct device *d;
> +
> + d = bus_find_device(&nvmem_bus_type, NULL, (void *)name, nvmem_match);
> +
> + return d ? to_nvmem_device(d) : NULL;
Maybe:
d = bus_find_device(...);
if (!d)
return NULL;
return to_nvmem_device(d);
> +struct nvmem_device *of_nvmem_device_get(struct device_node *np, const char *id)
> +{
[]
> + return __nvmem_device_get(nvmem_np, NULL, NULL);
> +
odd blank line
> +}
> +EXPORT_SYMBOL_GPL(of_nvmem_device_get);
> +struct nvmem_device *nvmem_device_get(struct device *dev, const char *dev_name)
> +{
[]
> + return nvmem_find(dev_name);
> +
here too
> +}
> +EXPORT_SYMBOL_GPL(nvmem_device_get);
[]
> +void devm_nvmem_device_put(struct device *dev, struct nvmem_device *nvmem)
> +{
> + int ret;
> +
> + ret = devres_release(dev, devm_nvmem_device_release,
> + devm_nvmem_device_match, nvmem);
> +
> + WARN_ON(ret);
> +
Last unnecessary blank line I'll mention
(it seems to be the last one anyway)
> +}
> +EXPORT_SYMBOL_GPL(devm_nvmem_device_put);
^ permalink raw reply
* Re: [RFCv3 2/5] mm/compaction: enable mobile-page migration
From: Konstantin Khlebnikov @ 2015-07-10 13:07 UTC (permalink / raw)
To: Gioh Kim
Cc: Jeff Layton, Bruce Fields, Vlastimil Babka, Joonsoo Kim, Al Viro,
Michael S. Tsirkin, Minchan Kim, Rafael Aquini, linux-fsdevel,
virtualization, Linux Kernel Mailing List, Linux API,
linux-mm@kvack.org, gunho.lee, Andrew Morton, Gioh Kim
In-Reply-To: <1436243785-24105-3-git-send-email-gioh.kim@lge.com>
On Tue, Jul 7, 2015 at 7:36 AM, Gioh Kim <gioh.kim@lge.com> wrote:
> From: Gioh Kim <gurugio@hanmail.net>
>
> Add framework to register callback functions and check page mobility.
> There are some modes for page isolation so that isolate interface
> has arguments of page address and isolation mode while putback
> interface has only page address as argument.
>
> Signed-off-by: Gioh Kim <gioh.kim@lge.com>
> ---
> fs/proc/page.c | 3 ++
> include/linux/compaction.h | 76 ++++++++++++++++++++++++++++++++++
> include/linux/fs.h | 2 +
> include/linux/page-flags.h | 19 +++++++++
> include/uapi/linux/kernel-page-flags.h | 1 +
> 5 files changed, 101 insertions(+)
>
> diff --git a/fs/proc/page.c b/fs/proc/page.c
> index 7eee2d8..a4f5a00 100644
> --- a/fs/proc/page.c
> +++ b/fs/proc/page.c
> @@ -146,6 +146,9 @@ u64 stable_page_flags(struct page *page)
> if (PageBalloon(page))
> u |= 1 << KPF_BALLOON;
>
> + if (PageMobile(page))
> + u |= 1 << KPF_MOBILE;
> +
> u |= kpf_copy_bit(k, KPF_LOCKED, PG_locked);
>
> u |= kpf_copy_bit(k, KPF_SLAB, PG_slab);
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index aa8f61c..c375a89 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -1,6 +1,9 @@
> #ifndef _LINUX_COMPACTION_H
> #define _LINUX_COMPACTION_H
>
> +#include <linux/page-flags.h>
> +#include <linux/pagemap.h>
> +
> /* Return values for compact_zone() and try_to_compact_pages() */
> /* compaction didn't start as it was deferred due to past failures */
> #define COMPACT_DEFERRED 0
> @@ -51,6 +54,66 @@ extern void compaction_defer_reset(struct zone *zone, int order,
> bool alloc_success);
> extern bool compaction_restarting(struct zone *zone, int order);
>
> +static inline bool mobile_page(struct page *page)
> +{
> + return page->mapping && page->mapping->a_ops &&
Dereferncing mapping->a_ops isn't safe without page-lock and isn't required:
all mappings always have ->a_ops.
> + (PageMobile(page) || PageBalloon(page));
> +}
> +
> +static inline bool isolate_mobilepage(struct page *page, isolate_mode_t mode)
> +{
> + bool ret;
> +
> + /*
> + * Avoid burning cycles with pages that are yet under __free_pages(),
> + * or just got freed under us.
> + *
> + * In case we 'win' a race for a mobile page being freed under us and
> + * raise its refcount preventing __free_pages() from doing its job
> + * the put_page() at the end of this block will take care of
> + * release this page, thus avoiding a nasty leakage.
> + */
> + if (likely(get_page_unless_zero(page))) {
> + /*
> + * As mobile pages are not isolated from LRU lists, concurrent
> + * compaction threads can race against page migration functions
> + * as well as race against the releasing a page.
> + *
> + * In order to avoid having an already isolated mobile page
> + * being (wrongly) re-isolated while it is under migration,
> + * or to avoid attempting to isolate pages being released,
> + * lets be sure we have the page lock
> + * before proceeding with the mobile page isolation steps.
> + */
> + if (likely(trylock_page(page))) {
> + if (mobile_page(page) &&
> + page->mapping->a_ops->isolatepage) {
> + ret = page->mapping->a_ops->isolatepage(page,
> + mode);
> + unlock_page(page);
> + return ret;
> + }
> + unlock_page(page);
> + }
> + put_page(page);
> + }
> + return false;
> +}
> +
> +static inline void putback_mobilepage(struct page *page)
> +{
> + /*
> + * 'lock_page()' stabilizes the page and prevents races against
> + * concurrent isolation threads attempting to re-isolate it.
> + */
> + lock_page(page);
> + if (mobile_page(page) && page->mapping->a_ops->putbackpage) {
It seems "if (page->mapping && page->mapping->a_ops->putbackpage)"
should be enough: we already seen that page as mobile.
> + page->mapping->a_ops->putbackpage(page);
> + /* drop the extra ref count taken for mobile page isolation */
> + put_page(page);
> + }
> + unlock_page(page);
call put_page() after unlock and do that always -- putback must drop
page reference from caller.
lock_page(page);
if (page->mapping && page->mapping->a_ops->putbackpage)
page->mapping->a_ops->putbackpage(page);
unlock_page();
put_page(page);
> +}
> #else
> static inline unsigned long try_to_compact_pages(gfp_t gfp_mask,
> unsigned int order, int alloc_flags,
> @@ -83,6 +146,19 @@ static inline bool compaction_deferred(struct zone *zone, int order)
> return true;
> }
>
> +static inline bool mobile_page(struct page *page)
> +{
> + return false;
> +}
> +
> +static inline bool isolate_mobilepage(struct page *page, isolate_mode_t mode)
> +{
> + return false;
> +}
> +
> +static inline void putback_mobilepage(struct page *page)
> +{
> +}
> #endif /* CONFIG_COMPACTION */
>
> #if defined(CONFIG_COMPACTION) && defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 35ec87e..33c9aa5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -395,6 +395,8 @@ struct address_space_operations {
> */
> int (*migratepage) (struct address_space *,
> struct page *, struct page *, enum migrate_mode);
> + bool (*isolatepage) (struct page *, isolate_mode_t);
> + void (*putbackpage) (struct page *);
> int (*launder_page) (struct page *);
> int (*is_partially_uptodate) (struct page *, unsigned long,
> unsigned long);
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index f34e040..abef145 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -582,6 +582,25 @@ static inline void __ClearPageBalloon(struct page *page)
> atomic_set(&page->_mapcount, -1);
> }
>
> +#define PAGE_MOBILE_MAPCOUNT_VALUE (-255)
> +
> +static inline int PageMobile(struct page *page)
> +{
> + return atomic_read(&page->_mapcount) == PAGE_MOBILE_MAPCOUNT_VALUE;
> +}
> +
> +static inline void __SetPageMobile(struct page *page)
> +{
> + VM_BUG_ON_PAGE(atomic_read(&page->_mapcount) != -1, page);
> + atomic_set(&page->_mapcount, PAGE_MOBILE_MAPCOUNT_VALUE);
> +}
> +
> +static inline void __ClearPageMobile(struct page *page)
> +{
> + VM_BUG_ON_PAGE(!PageMobile(page), page);
> + atomic_set(&page->_mapcount, -1);
> +}
> +
> /*
> * If network-based swap is enabled, sl*b must keep track of whether pages
> * were allocated from pfmemalloc reserves.
> diff --git a/include/uapi/linux/kernel-page-flags.h b/include/uapi/linux/kernel-page-flags.h
> index a6c4962..d50d9e8 100644
> --- a/include/uapi/linux/kernel-page-flags.h
> +++ b/include/uapi/linux/kernel-page-flags.h
> @@ -33,6 +33,7 @@
> #define KPF_THP 22
> #define KPF_BALLOON 23
> #define KPF_ZERO_PAGE 24
> +#define KPF_MOBILE 25
>
>
> #endif /* _UAPILINUX_KERNEL_PAGE_FLAGS_H */
> --
> 2.1.4
>
--
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: [RFCv3 3/5] mm/balloon: apply mobile page migratable into balloon
From: Konstantin Khlebnikov @ 2015-07-10 13:29 UTC (permalink / raw)
To: Gioh Kim
Cc: Jeff Layton, Bruce Fields, Vlastimil Babka, Joonsoo Kim, Al Viro,
Michael S. Tsirkin, Minchan Kim, Rafael Aquini, linux-fsdevel,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Linux Kernel Mailing List, Linux API,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org,
gunho.lee-Hm3cg6mZ9cc, Andrew Morton, Gioh Kim
In-Reply-To: <1436243785-24105-4-git-send-email-gioh.kim-Hm3cg6mZ9cc@public.gmane.org>
On Tue, Jul 7, 2015 at 7:36 AM, Gioh Kim <gioh.kim-Hm3cg6mZ9cc@public.gmane.org> wrote:
> From: Gioh Kim <gurugio-A7HHaXTFBl7R7s880joybQ@public.gmane.org>
>
> Apply mobile page migration into balloon driver.
> The balloong driver has an anonymous inode that manages
> address_space_operation for page migration.
>
> Signed-off-by: Gioh Kim <gioh.kim-Hm3cg6mZ9cc@public.gmane.org>
> ---
> drivers/virtio/virtio_balloon.c | 3 ++
> include/linux/balloon_compaction.h | 15 +++++++--
> mm/balloon_compaction.c | 65 +++++++++++++-------------------------
> mm/compaction.c | 2 +-
> mm/migrate.c | 2 +-
> 5 files changed, 39 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 82e80e0..ef5b9b5 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -30,6 +30,7 @@
> #include <linux/balloon_compaction.h>
> #include <linux/oom.h>
> #include <linux/wait.h>
> +#include <linux/anon_inodes.h>
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -505,6 +506,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
> balloon_devinfo_init(&vb->vb_dev_info);
> #ifdef CONFIG_BALLOON_COMPACTION
> vb->vb_dev_info.migratepage = virtballoon_migratepage;
> + vb->vb_dev_info.inode = anon_inode_new();
> + vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops;
> #endif
>
> err = init_vqs(vb);
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index 9b0a15d..a9e0bde 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -48,6 +48,7 @@
> #include <linux/migrate.h>
> #include <linux/gfp.h>
> #include <linux/err.h>
> +#include <linux/fs.h>
>
> /*
> * Balloon device information descriptor.
> @@ -62,6 +63,7 @@ struct balloon_dev_info {
> struct list_head pages; /* Pages enqueued & handled to Host */
> int (*migratepage)(struct balloon_dev_info *, struct page *newpage,
> struct page *page, enum migrate_mode mode);
> + struct inode *inode;
> };
>
> extern struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info);
> @@ -73,12 +75,16 @@ static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
> spin_lock_init(&balloon->pages_lock);
> INIT_LIST_HEAD(&balloon->pages);
> balloon->migratepage = NULL;
> + balloon->inode = NULL;
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> -extern bool balloon_page_isolate(struct page *page);
> +extern const struct address_space_operations balloon_aops;
> +extern bool balloon_page_isolate(struct page *page,
> + isolate_mode_t mode);
> extern void balloon_page_putback(struct page *page);
> -extern int balloon_page_migrate(struct page *newpage,
> +extern int balloon_page_migrate(struct address_space *mapping,
> + struct page *newpage,
> struct page *page, enum migrate_mode mode);
>
> /*
> @@ -124,6 +130,7 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon,
> struct page *page)
> {
> __SetPageBalloon(page);
> + page->mapping = balloon->inode->i_mapping;
> SetPagePrivate(page);
> set_page_private(page, (unsigned long)balloon);
> list_add(&page->lru, &balloon->pages);
> @@ -140,6 +147,7 @@ static inline void balloon_page_insert(struct balloon_dev_info *balloon,
> static inline void balloon_page_delete(struct page *page)
> {
> __ClearPageBalloon(page);
> + page->mapping = NULL;
> set_page_private(page, 0);
> if (PagePrivate(page)) {
> ClearPagePrivate(page);
> @@ -191,7 +199,8 @@ static inline bool isolated_balloon_page(struct page *page)
> return false;
> }
>
> -static inline bool balloon_page_isolate(struct page *page)
> +static inline bool balloon_page_isolate(struct page *page,
> + isolate_mode_t mode)
> {
> return false;
> }
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index fcad832..0dd0b0d 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -131,43 +131,16 @@ static inline void __putback_balloon_page(struct page *page)
> }
>
> /* __isolate_lru_page() counterpart for a ballooned page */
> -bool balloon_page_isolate(struct page *page)
> +bool balloon_page_isolate(struct page *page, isolate_mode_t mode)
> {
> /*
> - * Avoid burning cycles with pages that are yet under __free_pages(),
> - * or just got freed under us.
> - *
> - * In case we 'win' a race for a balloon page being freed under us and
> - * raise its refcount preventing __free_pages() from doing its job
> - * the put_page() at the end of this block will take care of
> - * release this page, thus avoiding a nasty leakage.
> + * A ballooned page, by default, has PagePrivate set.
> + * Prevent concurrent compaction threads from isolating
> + * an already isolated balloon page by clearing it.
> */
> - if (likely(get_page_unless_zero(page))) {
> - /*
> - * As balloon pages are not isolated from LRU lists, concurrent
> - * compaction threads can race against page migration functions
> - * as well as race against the balloon driver releasing a page.
> - *
> - * In order to avoid having an already isolated balloon page
> - * being (wrongly) re-isolated while it is under migration,
> - * or to avoid attempting to isolate pages being released by
> - * the balloon driver, lets be sure we have the page lock
> - * before proceeding with the balloon page isolation steps.
> - */
> - if (likely(trylock_page(page))) {
> - /*
> - * A ballooned page, by default, has PagePrivate set.
> - * Prevent concurrent compaction threads from isolating
> - * an already isolated balloon page by clearing it.
> - */
> - if (balloon_page_movable(page)) {
> - __isolate_balloon_page(page);
> - unlock_page(page);
> - return true;
> - }
> - unlock_page(page);
> - }
> - put_page(page);
> + if (balloon_page_movable(page)) {
> + __isolate_balloon_page(page);
> + return true;
> }
> return false;
> }
> @@ -175,30 +148,28 @@ bool balloon_page_isolate(struct page *page)
> /* putback_lru_page() counterpart for a ballooned page */
> void balloon_page_putback(struct page *page)
> {
> - /*
> - * 'lock_page()' stabilizes the page and prevents races against
> - * concurrent isolation threads attempting to re-isolate it.
> - */
> - lock_page(page);
> + if (!isolated_balloon_page(page))
> + return;
>
> if (__is_movable_balloon_page(page)) {
> __putback_balloon_page(page);
> - /* drop the extra ref count taken for page isolation */
> - put_page(page);
> } else {
> WARN_ON(1);
> dump_page(page, "not movable balloon page");
> }
> - unlock_page(page);
> }
>
> /* move_to_new_page() counterpart for a ballooned page */
> -int balloon_page_migrate(struct page *newpage,
> +int balloon_page_migrate(struct address_space *mapping,
> + struct page *newpage,
> struct page *page, enum migrate_mode mode)
> {
> struct balloon_dev_info *balloon = balloon_page_device(page);
> int rc = -EAGAIN;
>
> + if (!isolated_balloon_page(page))
> + return rc;
> +
> /*
> * Block others from accessing the 'newpage' when we get around to
> * establishing additional references. We should be the only one
> @@ -218,4 +189,12 @@ int balloon_page_migrate(struct page *newpage,
> unlock_page(newpage);
Both pages passed as arguments of ->migratepage() are locked.
So, please remove lock/unlock from this function here and add lock/unlock
newpage in __unmap_and_move() below. Right in this patch.
> return rc;
> }
> +
> +/* define the balloon_mapping->a_ops callback to allow balloon page migration */
> +const struct address_space_operations balloon_aops = {
> + .migratepage = balloon_page_migrate,
> + .isolatepage = balloon_page_isolate,
> + .putbackpage = balloon_page_putback,
> +};
> +EXPORT_SYMBOL_GPL(balloon_aops);
> #endif /* CONFIG_BALLOON_COMPACTION */
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 018f08d..81bafaf 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -719,7 +719,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> */
> if (!PageLRU(page)) {
> if (unlikely(balloon_page_movable(page))) {
> - if (balloon_page_isolate(page)) {
> + if (balloon_page_isolate(page, isolate_mode)) {
> /* Successfully isolated */
> goto isolate_success;
> }
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f53838f..c94038e 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -852,7 +852,7 @@ static int __unmap_and_move(struct page *page, struct page *newpage,
> * in order to avoid burning cycles at rmap level, and perform
> * the page migration right away (proteced by page lock).
> */
> - rc = balloon_page_migrate(newpage, page, mode);
Here:
lock_page(newpage);
> + rc = balloon_page_migrate(page->mapping, newpage, page, mode);
unlock_page(newpage);
> goto out_unlock;
> }
>
> --
> 2.1.4
>
^ permalink raw reply
* Re: [RFCv3 2/5] mm/compaction: enable mobile-page migration
From: Konstantin Khlebnikov @ 2015-07-10 13:41 UTC (permalink / raw)
To: Gioh Kim
Cc: Jeff Layton, Bruce Fields, Vlastimil Babka, Joonsoo Kim, Al Viro,
Michael S. Tsirkin, Minchan Kim, Rafael Aquini, linux-fsdevel,
virtualization, Linux Kernel Mailing List, Linux API,
linux-mm@kvack.org, gunho.lee, Andrew Morton, Gioh Kim
In-Reply-To: <1436243785-24105-3-git-send-email-gioh.kim@lge.com>
One more note below.
On Tue, Jul 7, 2015 at 7:36 AM, Gioh Kim <gioh.kim@lge.com> wrote:
> From: Gioh Kim <gurugio@hanmail.net>
>
> Add framework to register callback functions and check page mobility.
> There are some modes for page isolation so that isolate interface
> has arguments of page address and isolation mode while putback
> interface has only page address as argument.
>
> Signed-off-by: Gioh Kim <gioh.kim@lge.com>
> ---
> fs/proc/page.c | 3 ++
> include/linux/compaction.h | 76 ++++++++++++++++++++++++++++++++++
> include/linux/fs.h | 2 +
> include/linux/page-flags.h | 19 +++++++++
> include/uapi/linux/kernel-page-flags.h | 1 +
> 5 files changed, 101 insertions(+)
>
> diff --git a/fs/proc/page.c b/fs/proc/page.c
> index 7eee2d8..a4f5a00 100644
> --- a/fs/proc/page.c
> +++ b/fs/proc/page.c
> @@ -146,6 +146,9 @@ u64 stable_page_flags(struct page *page)
> if (PageBalloon(page))
> u |= 1 << KPF_BALLOON;
>
> + if (PageMobile(page))
> + u |= 1 << KPF_MOBILE;
> +
> u |= kpf_copy_bit(k, KPF_LOCKED, PG_locked);
>
> u |= kpf_copy_bit(k, KPF_SLAB, PG_slab);
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index aa8f61c..c375a89 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -1,6 +1,9 @@
> #ifndef _LINUX_COMPACTION_H
> #define _LINUX_COMPACTION_H
>
> +#include <linux/page-flags.h>
> +#include <linux/pagemap.h>
> +
> /* Return values for compact_zone() and try_to_compact_pages() */
> /* compaction didn't start as it was deferred due to past failures */
> #define COMPACT_DEFERRED 0
> @@ -51,6 +54,66 @@ extern void compaction_defer_reset(struct zone *zone, int order,
> bool alloc_success);
> extern bool compaction_restarting(struct zone *zone, int order);
>
> +static inline bool mobile_page(struct page *page)
> +{
> + return page->mapping && page->mapping->a_ops &&
> + (PageMobile(page) || PageBalloon(page));
> +}
> +
> +static inline bool isolate_mobilepage(struct page *page, isolate_mode_t mode)
> +{
> + bool ret;
> +
> + /*
> + * Avoid burning cycles with pages that are yet under __free_pages(),
> + * or just got freed under us.
> + *
> + * In case we 'win' a race for a mobile page being freed under us and
> + * raise its refcount preventing __free_pages() from doing its job
> + * the put_page() at the end of this block will take care of
> + * release this page, thus avoiding a nasty leakage.
> + */
> + if (likely(get_page_unless_zero(page))) {
> + /*
> + * As mobile pages are not isolated from LRU lists, concurrent
> + * compaction threads can race against page migration functions
> + * as well as race against the releasing a page.
> + *
> + * In order to avoid having an already isolated mobile page
> + * being (wrongly) re-isolated while it is under migration,
> + * or to avoid attempting to isolate pages being released,
> + * lets be sure we have the page lock
> + * before proceeding with the mobile page isolation steps.
> + */
> + if (likely(trylock_page(page))) {
> + if (mobile_page(page) &&
> + page->mapping->a_ops->isolatepage) {
> + ret = page->mapping->a_ops->isolatepage(page,
> + mode);
> + unlock_page(page);
Here you leak page reference if isolatepage() fails.
> + return ret;
> + }
> + unlock_page(page);
> + }
> + put_page(page);
> + }
> + return false;
> +}
> +
> +static inline void putback_mobilepage(struct page *page)
> +{
> + /*
> + * 'lock_page()' stabilizes the page and prevents races against
> + * concurrent isolation threads attempting to re-isolate it.
> + */
> + lock_page(page);
> + if (mobile_page(page) && page->mapping->a_ops->putbackpage) {
> + page->mapping->a_ops->putbackpage(page);
> + /* drop the extra ref count taken for mobile page isolation */
> + put_page(page);
> + }
> + unlock_page(page);
> +}
> #else
> static inline unsigned long try_to_compact_pages(gfp_t gfp_mask,
> unsigned int order, int alloc_flags,
> @@ -83,6 +146,19 @@ static inline bool compaction_deferred(struct zone *zone, int order)
> return true;
> }
>
> +static inline bool mobile_page(struct page *page)
> +{
> + return false;
> +}
> +
> +static inline bool isolate_mobilepage(struct page *page, isolate_mode_t mode)
> +{
> + return false;
> +}
> +
> +static inline void putback_mobilepage(struct page *page)
> +{
> +}
> #endif /* CONFIG_COMPACTION */
>
> #if defined(CONFIG_COMPACTION) && defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 35ec87e..33c9aa5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -395,6 +395,8 @@ struct address_space_operations {
> */
> int (*migratepage) (struct address_space *,
> struct page *, struct page *, enum migrate_mode);
> + bool (*isolatepage) (struct page *, isolate_mode_t);
> + void (*putbackpage) (struct page *);
> int (*launder_page) (struct page *);
> int (*is_partially_uptodate) (struct page *, unsigned long,
> unsigned long);
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index f34e040..abef145 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -582,6 +582,25 @@ static inline void __ClearPageBalloon(struct page *page)
> atomic_set(&page->_mapcount, -1);
> }
>
> +#define PAGE_MOBILE_MAPCOUNT_VALUE (-255)
> +
> +static inline int PageMobile(struct page *page)
> +{
> + return atomic_read(&page->_mapcount) == PAGE_MOBILE_MAPCOUNT_VALUE;
> +}
> +
> +static inline void __SetPageMobile(struct page *page)
> +{
> + VM_BUG_ON_PAGE(atomic_read(&page->_mapcount) != -1, page);
> + atomic_set(&page->_mapcount, PAGE_MOBILE_MAPCOUNT_VALUE);
> +}
> +
> +static inline void __ClearPageMobile(struct page *page)
> +{
> + VM_BUG_ON_PAGE(!PageMobile(page), page);
> + atomic_set(&page->_mapcount, -1);
> +}
> +
> /*
> * If network-based swap is enabled, sl*b must keep track of whether pages
> * were allocated from pfmemalloc reserves.
> diff --git a/include/uapi/linux/kernel-page-flags.h b/include/uapi/linux/kernel-page-flags.h
> index a6c4962..d50d9e8 100644
> --- a/include/uapi/linux/kernel-page-flags.h
> +++ b/include/uapi/linux/kernel-page-flags.h
> @@ -33,6 +33,7 @@
> #define KPF_THP 22
> #define KPF_BALLOON 23
> #define KPF_ZERO_PAGE 24
> +#define KPF_MOBILE 25
>
>
> #endif /* _UAPILINUX_KERNEL_PAGE_FLAGS_H */
> --
> 2.1.4
>
^ permalink raw reply
* Re: [PATCH V3 3/5] mm: mlock: Introduce VM_LOCKONFAULT and add mlock flags to enable it
From: Jonathan Corbet @ 2015-07-10 16:11 UTC (permalink / raw)
To: Eric B Munson
Cc: Andrew Morton, Michal Hocko, Vlastimil Babka, linux-alpha,
linux-kernel, linux-mips, linux-parisc, linuxppc-dev, sparclinux,
linux-xtensa, linux-mm, linux-arch, linux-api
In-Reply-To: <20150709184635.GE4669@akamai.com>
On Thu, 9 Jul 2015 14:46:35 -0400
Eric B Munson <emunson@akamai.com> wrote:
> > One other question...if I call mlock2(MLOCK_ONFAULT) on a range that
> > already has resident pages, I believe that those pages will not be locked
> > until they are reclaimed and faulted back in again, right? I suspect that
> > could be surprising to users.
>
> That is the case. I am looking into what it would take to find only the
> present pages in a range and lock them, if that is the behavior that is
> preferred I can include it in the updated series.
For whatever my $0.02 is worth, I think that should be done. Otherwise
the mlock2() interface is essentially nondeterministic; you'll never
really know if a specific page is locked or not.
Thanks,
jon
^ permalink raw reply
* [REVIEW][PATCH 0/2] noexec on proc and sysfs
From: Eric W. Biederman @ 2015-07-10 16:16 UTC (permalink / raw)
To: Linux Containers
Cc: Linus Torvalds, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux API,
Andy Lutomirski, Serge E. Hallyn, Richard Weinberger,
Kenton Varda, Michael Kerrisk-manpages, Stéphane Graber,
Eric Windisch, Greg Kroah-Hartman, Tejun Heo, Seth Forshee,
Omar Sandoval, Ivan Delalande, Al Viro
In-Reply-To: <20150704231118.GT17109-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
Given the code I have seen executables especially suid root executable
appearing on proc or sysfs will break userspace because there are
current applications that depend on nosuid and noexec on proc and sysfs
being meaningless.
This patchset addes a new flag SB_I_NOEXEC to enforce that restriction,
and to make it hard for a kernel developer to make the mistake of adding
executables to sysfs or proc.
The first patch has been updated since last time to a super block flags
instead of a file_system type flag based on Al's suggestion.
The code in fs_fully_visible to enforce nosuid and noexec when needed
has also been added.
At a practical level this code is a no-op on a slow path, to guard
against future mistakes and to make auditing the kernel for this class
of problem trivial.
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-testing
Eric W. Biederman (2):
vfs: Commit to never having exectuables on proc and sysfs.
mnt: fs_fully_visible enforce noexec and nosuid if !SB_I_NOEXEC
fs/exec.c | 10 ++++++++--
fs/namespace.c | 33 +++++++++++++++++++++++++--------
fs/open.c | 2 +-
fs/proc/root.c | 2 ++
fs/sysfs/mount.c | 4 ++++
include/linux/fs.h | 3 +++
kernel/sys.c | 3 +--
mm/mmap.c | 4 ++--
mm/nommu.c | 2 +-
security/security.c | 2 +-
10 files changed, 48 insertions(+), 17 deletions(-)
^ permalink raw reply
* [REVIEW][PATCH 1/2] vfs: Commit to never having exectuables on proc and sysfs.
From: Eric W. Biederman @ 2015-07-10 16:17 UTC (permalink / raw)
To: Linux Containers
Cc: Linus Torvalds, linux-fsdevel, Linux API, Andy Lutomirski,
Serge E. Hallyn, Richard Weinberger, Kenton Varda,
Michael Kerrisk-manpages, Stéphane Graber, Eric Windisch,
Greg Kroah-Hartman, Tejun Heo, Seth Forshee, Omar Sandoval,
Ivan Delalande, Al Viro
In-Reply-To: <87mvz4yomp.fsf_-_@x220.int.ebiederm.org>
Today proc and sysfs do not contain any executable files. Several
applications today mount proc or sysfs without noexec and nosuid and
then depend on there being no exectuables files on proc or sysfs.
Having any executable files show on proc or sysfs would cause
a user space visible regression, and most likely security problems.
Therefore commit to never allowing executables on proc and sysfs by
adding a new flag to mark them as filesystems without executables and
enforce that flag.
Test the flag where MNT_NOEXEC is tested today, so that the only user
visible effect will be that exectuables will be treated as if the
execute bit is cleared.
The filesystems proc and sysfs do not currently incoporate any
executable files so this does not result in any user visible effects.
This makes it unnecessary to vet changes to proc and sysfs tightly for
adding exectuable files or changes to chattr that would modify
existing files, as no matter what the individual file say they will
not be treated as exectuable files by the vfs.
Not having to vet changes to closely is important as without this we
are only one proc_create call (or another goof up in the
implementation of notify_change) from having problematic executables
on proc. Those mistakes are all too easy to make and would create
a situation where there are security issues or the assumptions of
some program having to be broken (and cause userspace regressions).
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
fs/exec.c | 10 ++++++++--
fs/open.c | 2 +-
fs/proc/root.c | 2 ++
fs/sysfs/mount.c | 4 ++++
include/linux/fs.h | 3 +++
kernel/sys.c | 3 +--
mm/mmap.c | 4 ++--
mm/nommu.c | 2 +-
security/security.c | 2 +-
9 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 1977c2a553ac..b06623a9347f 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -98,6 +98,12 @@ static inline void put_binfmt(struct linux_binfmt * fmt)
module_put(fmt->module);
}
+bool path_noexec(const struct path *path)
+{
+ return (path->mnt->mnt_flags & MNT_NOEXEC) ||
+ (path->mnt->mnt_sb->s_iflags & SB_I_NOEXEC);
+}
+
#ifdef CONFIG_USELIB
/*
* Note that a shared library must be both readable and executable due to
@@ -132,7 +138,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library)
goto exit;
error = -EACCES;
- if (file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+ if (path_noexec(&file->f_path))
goto exit;
fsnotify_open(file);
@@ -777,7 +783,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
if (!S_ISREG(file_inode(file)->i_mode))
goto exit;
- if (file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+ if (path_noexec(&file->f_path))
goto exit;
err = deny_write_access(file);
diff --git a/fs/open.c b/fs/open.c
index e33dab287fa0..b6f1e96a7c0b 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -377,7 +377,7 @@ retry:
* with the "noexec" flag.
*/
res = -EACCES;
- if (path.mnt->mnt_flags & MNT_NOEXEC)
+ if (path_noexec(&path))
goto out_path_release;
}
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 68feb0f70e63..361ab4ee42fc 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -134,6 +134,8 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
}
sb->s_flags |= MS_ACTIVE;
+ /* User space would break if executables appear on proc */
+ sb->s_iflags |= SB_I_NOEXEC;
}
return dget(sb->s_root);
diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
index 1c6ac6fcee9f..f3db82071cfb 100644
--- a/fs/sysfs/mount.c
+++ b/fs/sysfs/mount.c
@@ -40,6 +40,10 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
SYSFS_MAGIC, &new_sb, ns);
if (IS_ERR(root) || !new_sb)
kobj_ns_drop(KOBJ_NS_TYPE_NET, ns);
+ else if (new_sb)
+ /* Userspace would break if executables appear on sysfs */
+ root->d_sb->s_iflags |= SB_I_NOEXEC;
+
return root;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a0653e560c26..42912f8d286e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1244,6 +1244,7 @@ struct mm_struct;
/* sb->s_iflags */
#define SB_I_CGROUPWB 0x00000001 /* cgroup-aware writeback enabled */
+#define SB_I_NOEXEC 0x00000002 /* Ignore executables on this fs */
/* Possible states of 'frozen' field */
enum {
@@ -3030,4 +3031,6 @@ static inline bool dir_relax(struct inode *inode)
return !IS_DEADDIR(inode);
}
+extern bool path_noexec(const struct path *path);
+
#endif /* _LINUX_FS_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index 259fda25eb6b..fa2f2f671a5c 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1668,8 +1668,7 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
* overall picture.
*/
err = -EACCES;
- if (!S_ISREG(inode->i_mode) ||
- exe.file->f_path.mnt->mnt_flags & MNT_NOEXEC)
+ if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
goto exit;
err = inode_permission(inode, MAY_EXEC);
diff --git a/mm/mmap.c b/mm/mmap.c
index aa632ade2be7..f126923ce683 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1268,7 +1268,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
* mounted, in which case we dont add PROT_EXEC.)
*/
if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
- if (!(file && (file->f_path.mnt->mnt_flags & MNT_NOEXEC)))
+ if (!(file && path_noexec(&file->f_path)))
prot |= PROT_EXEC;
if (!(flags & MAP_FIXED))
@@ -1337,7 +1337,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
case MAP_PRIVATE:
if (!(file->f_mode & FMODE_READ))
return -EACCES;
- if (file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
+ if (path_noexec(&file->f_path)) {
if (vm_flags & VM_EXEC)
return -EPERM;
vm_flags &= ~VM_MAYEXEC;
diff --git a/mm/nommu.c b/mm/nommu.c
index 58ea3643b9e9..ce17abf087ff 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -1035,7 +1035,7 @@ static int validate_mmap_request(struct file *file,
/* handle executable mappings and implied executable
* mappings */
- if (file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
+ if (path_noexec(&file->f_path)) {
if (prot & PROT_EXEC)
return -EPERM;
} else if ((prot & PROT_READ) && !(prot & PROT_EXEC)) {
diff --git a/security/security.c b/security/security.c
index 595fffab48b0..062f3c997fdc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -776,7 +776,7 @@ static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
* ditto if it's not on noexec mount, except that on !MMU we need
* NOMMU_MAP_EXEC (== VM_MAYEXEC) in this case
*/
- if (!(file->f_path.mnt->mnt_flags & MNT_NOEXEC)) {
+ if (!path_noexec(&file->f_path)) {
#ifndef CONFIG_MMU
if (file->f_op->mmap_capabilities) {
unsigned caps = file->f_op->mmap_capabilities(file);
--
2.2.1
^ permalink raw reply related
* [REVIEW][PATCH 2/2] mnt: fs_fully_visible enforce noexec and nosuid if !SB_I_NOEXEC
From: Eric W. Biederman @ 2015-07-10 16:18 UTC (permalink / raw)
To: Linux Containers
Cc: Seth Forshee, Linux API, Greg Kroah-Hartman, Al Viro,
Andy Lutomirski, Kenton Varda, Michael Kerrisk-manpages,
Richard Weinberger, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
Tejun Heo, Ivan Delalande, Linus Torvalds
In-Reply-To: <87mvz4yomp.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
The filesystems proc and sysfs do not have executable files do not
have exectuable files today and portions of userspace break if we do
enforce nosuid and noexec consistency of nosuid and noexec flags
between previous mounts and new mounts of proc and sysfs.
Add the code to enforce consistency of the nosuid and noexec flags,
and use the presence of SB_I_NOEXEC to signal that there is no need to
bother.
This results in a completely userspace invisible change that makes it
clear fs_fully_visible can only skip the enforcement of noexec and
nosuid because it is known the filesystems in question do not support
executables.
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
fs/namespace.c | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index c7cb8a526c05..ce428cadd41f 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3194,6 +3194,8 @@ static bool fs_fully_visible(struct file_system_type *type, int *new_mnt_flags)
down_read(&namespace_sem);
list_for_each_entry(mnt, &ns->list, mnt_list) {
struct mount *child;
+ int mnt_flags;
+
if (mnt->mnt.mnt_sb->s_type != type)
continue;
@@ -3203,17 +3205,30 @@ static bool fs_fully_visible(struct file_system_type *type, int *new_mnt_flags)
if (mnt->mnt.mnt_root != mnt->mnt.mnt_sb->s_root)
continue;
+ /* Read the mount flags and filter out flags that
+ * may safely be ignored.
+ */
+ mnt_flags = mnt->mnt.mnt_flags;
+ if (mnt->mnt.mnt_sb->s_iflags & SB_I_NOEXEC)
+ mnt_flags &= ~(MNT_LOCK_NOSUID | MNT_LOCK_NOEXEC);
+
/* Verify the mount flags are equal to or more permissive
* than the proposed new mount.
*/
- if ((mnt->mnt.mnt_flags & MNT_LOCK_READONLY) &&
+ if ((mnt_flags & MNT_LOCK_READONLY) &&
!(new_flags & MNT_READONLY))
continue;
- if ((mnt->mnt.mnt_flags & MNT_LOCK_NODEV) &&
+ if ((mnt_flags & MNT_LOCK_NODEV) &&
!(new_flags & MNT_NODEV))
continue;
- if ((mnt->mnt.mnt_flags & MNT_LOCK_ATIME) &&
- ((mnt->mnt.mnt_flags & MNT_ATIME_MASK) != (new_flags & MNT_ATIME_MASK)))
+ if ((mnt_flags & MNT_LOCK_NOSUID) &&
+ !(new_flags & MNT_NOSUID))
+ continue;
+ if ((mnt_flags & MNT_LOCK_NOEXEC) &&
+ !(new_flags & MNT_NOEXEC))
+ continue;
+ if ((mnt_flags & MNT_LOCK_ATIME) &&
+ ((mnt_flags & MNT_ATIME_MASK) != (new_flags & MNT_ATIME_MASK)))
continue;
/* This mount is not fully visible if there are any
@@ -3223,16 +3238,18 @@ static bool fs_fully_visible(struct file_system_type *type, int *new_mnt_flags)
list_for_each_entry(child, &mnt->mnt_mounts, mnt_child) {
struct inode *inode = child->mnt_mountpoint->d_inode;
/* Only worry about locked mounts */
- if (!(mnt->mnt.mnt_flags & MNT_LOCKED))
+ if (!(mnt_flags & MNT_LOCKED))
continue;
/* Is the directory permanetly empty? */
if (!is_empty_dir_inode(inode))
goto next;
}
/* Preserve the locked attributes */
- *new_mnt_flags |= mnt->mnt.mnt_flags & (MNT_LOCK_READONLY | \
- MNT_LOCK_NODEV | \
- MNT_LOCK_ATIME);
+ *new_mnt_flags |= mnt_flags & (MNT_LOCK_READONLY | \
+ MNT_LOCK_NODEV | \
+ MNT_LOCK_NOSUID | \
+ MNT_LOCK_NOEXEC | \
+ MNT_LOCK_ATIME);
visible = true;
goto found;
next: ;
--
2.2.1
^ permalink raw reply related
* Re: [PATCH V3 3/5] mm: mlock: Introduce VM_LOCKONFAULT and add mlock flags to enable it
From: Eric B Munson @ 2015-07-10 16:19 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Andrew Morton, Michal Hocko, Vlastimil Babka,
linux-alpha-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-parisc-u79uwXL29TY76Z2rM5mHXA,
linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ,
sparclinux-u79uwXL29TY76Z2rM5mHXA,
linux-xtensa-PjhNF2WwrV/0Sa2dR60CXw,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-arch-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150710101118.5d04d627-T1hC0tSOHrs@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]
On Fri, 10 Jul 2015, Jonathan Corbet wrote:
> On Thu, 9 Jul 2015 14:46:35 -0400
> Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
>
> > > One other question...if I call mlock2(MLOCK_ONFAULT) on a range that
> > > already has resident pages, I believe that those pages will not be locked
> > > until they are reclaimed and faulted back in again, right? I suspect that
> > > could be surprising to users.
> >
> > That is the case. I am looking into what it would take to find only the
> > present pages in a range and lock them, if that is the behavior that is
> > preferred I can include it in the updated series.
>
> For whatever my $0.02 is worth, I think that should be done. Otherwise
> the mlock2() interface is essentially nondeterministic; you'll never
> really know if a specific page is locked or not.
>
> Thanks,
>
> jon
Okay, I likely won't have the new set out today then. This change is
more invasive. IIUC, I need an equivalent to __get_user_page() skips
pages which are not present instead of faulting in and the call chain to
get to it. Unless there is an easier way that I am missing.
Eric
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC 5/8] kdbus: use LSM hooks in kdbus code
From: Stephen Smalley @ 2015-07-10 16:56 UTC (permalink / raw)
To: Paul Osmialowski, Paul Moore, James Morris, Casey Schaufler,
Serge E. Hallyn, Kees Cook, Tetsuo Handa, Neil Brown, Mark Rustad,
Greg Kroah-Hartman, Daniel Mack, David Herrmann, Djalal Harouni,
Shuah Khan, Al Viro, linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Karol Lewandowski, Lukasz Skalski
In-Reply-To: <559D27AB.4010402-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
On 07/08/2015 09:37 AM, Stephen Smalley wrote:
> On 07/08/2015 06:25 AM, Paul Osmialowski wrote:
>> Originates from:
>>
>> https://github.com/lmctl/kdbus.git (branch: kdbus-lsm-v4.for-systemd-v212)
>> commit: aa0885489d19be92fa41c6f0a71df28763228a40
>>
>> Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Paul Osmialowski <p.osmialowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>> ipc/kdbus/bus.c | 12 ++++++++++-
>> ipc/kdbus/bus.h | 3 +++
>> ipc/kdbus/connection.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> ipc/kdbus/connection.h | 4 ++++
>> ipc/kdbus/domain.c | 9 ++++++++-
>> ipc/kdbus/domain.h | 2 ++
>> ipc/kdbus/endpoint.c | 11 ++++++++++
>> ipc/kdbus/names.c | 11 ++++++++++
>> ipc/kdbus/queue.c | 30 ++++++++++++++++++----------
>> 9 files changed, 124 insertions(+), 12 deletions(-)
>>
>>
>
>> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
>> index 9993753..b85cdc7 100644
>> --- a/ipc/kdbus/connection.c
>> +++ b/ipc/kdbus/connection.c
>> @@ -31,6 +31,7 @@
>> #include <linux/slab.h>
>> #include <linux/syscalls.h>
>> #include <linux/uio.h>
>> +#include <linux/security.h>
>>
>> #include "bus.h"
>> #include "connection.h"
>> @@ -73,6 +74,8 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep, bool privileged,
>> bool is_activator;
>> bool is_monitor;
>> struct kvec kvec;
>> + u32 sid, len;
>> + char *label;
>> int ret;
>>
>> struct {
>> @@ -222,6 +225,14 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep, bool privileged,
>> }
>> }
>>
>> + security_task_getsecid(current, &sid);
>> + security_secid_to_secctx(sid, &label, &len);
>> + ret = security_kdbus_connect(conn, label, len);
>> + if (ret) {
>> + ret = -EPERM;
>> + goto exit_unref;
>> + }
>
> This seems convoluted and expensive. If you always want the label of
> the current task here, then why not just have security_kdbus_connect()
> internally extract the label of the current task?
>
>> @@ -1107,6 +1119,12 @@ static int kdbus_conn_reply(struct kdbus_conn *src, struct kdbus_kmsg *kmsg)
>> if (ret < 0)
>> goto exit;
>>
>> + ret = security_kdbus_talk(src, dst);
>> + if (ret) {
>> + ret = -EPERM;
>> + goto exit;
>> + }
>
> Where does kdbus apply its uid-based or other restrictions on
> connections? Why do we need to insert separate hooks into each of these
> functions? Is there no central chokepoint already for permission
> checking that we can hook?
For example, why wouldn't you insert a single hook into
kdbus_conn_policy_talk() where they perform their DAC checking?
You would need to restructure it slightly to ensure that the security
hook is only called if it passes the DAC (privileged || uid_eq) check so
that we do not trigger MAC denials when DAC wouldn't have allowed it
anyway. Also, kdbus_conn_policy_talk() takes a separate conn_creds
argument - that should be passed through to the hook as well.
^ permalink raw reply
* Re: [RFC 5/8] kdbus: use LSM hooks in kdbus code
From: Stephen Smalley @ 2015-07-10 18:20 UTC (permalink / raw)
To: Paul Osmialowski, Paul Moore, James Morris, Casey Schaufler,
Serge E. Hallyn, Kees Cook, Tetsuo Handa, Neil Brown, Mark Rustad,
Greg Kroah-Hartman, Daniel Mack, David Herrmann, Djalal Harouni,
Shuah Khan, Al Viro, linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Karol Lewandowski, Lukasz Skalski
In-Reply-To: <559D27AB.4010402-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
On 07/08/2015 09:37 AM, Stephen Smalley wrote:
> On 07/08/2015 06:25 AM, Paul Osmialowski wrote:
>> Originates from:
>>
>> https://github.com/lmctl/kdbus.git (branch: kdbus-lsm-v4.for-systemd-v212)
>> commit: aa0885489d19be92fa41c6f0a71df28763228a40
>>
>> Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Paul Osmialowski <p.osmialowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>> ipc/kdbus/bus.c | 12 ++++++++++-
>> ipc/kdbus/bus.h | 3 +++
>> ipc/kdbus/connection.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> ipc/kdbus/connection.h | 4 ++++
>> ipc/kdbus/domain.c | 9 ++++++++-
>> ipc/kdbus/domain.h | 2 ++
>> ipc/kdbus/endpoint.c | 11 ++++++++++
>> ipc/kdbus/names.c | 11 ++++++++++
>> ipc/kdbus/queue.c | 30 ++++++++++++++++++----------
>> 9 files changed, 124 insertions(+), 12 deletions(-)
>>
>>
>
>> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
>> index 9993753..b85cdc7 100644
>> --- a/ipc/kdbus/connection.c
>> +++ b/ipc/kdbus/connection.c
>> @@ -31,6 +31,7 @@
>> #include <linux/slab.h>
>> #include <linux/syscalls.h>
>> #include <linux/uio.h>
>> +#include <linux/security.h>
>>
>> #include "bus.h"
>> #include "connection.h"
>> @@ -73,6 +74,8 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep, bool privileged,
>> bool is_activator;
>> bool is_monitor;
>> struct kvec kvec;
>> + u32 sid, len;
>> + char *label;
>> int ret;
>>
>> struct {
>> @@ -222,6 +225,14 @@ static struct kdbus_conn *kdbus_conn_new(struct kdbus_ep *ep, bool privileged,
>> }
>> }
>>
>> + security_task_getsecid(current, &sid);
>> + security_secid_to_secctx(sid, &label, &len);
>> + ret = security_kdbus_connect(conn, label, len);
>> + if (ret) {
>> + ret = -EPERM;
>> + goto exit_unref;
>> + }
>
> This seems convoluted and expensive. If you always want the label of
> the current task here, then why not just have security_kdbus_connect()
> internally extract the label of the current task?
Furthermore, why do we need a separate security field and copy of the
current label in the conn->security, when we already have
conn->cred->security available to us?
I don't think we need new security fields unless we are going to assign
some kind of object labeling to these structures separate from their
cred, and offhand I don't see why we would do that.
^ permalink raw reply
* Re: [REVIEW][PATCH 1/2] vfs: Commit to never having exectuables on proc and sysfs.
From: Richard Weinberger @ 2015-07-10 18:24 UTC (permalink / raw)
To: Eric W. Biederman, Linux Containers
Cc: Seth Forshee, Linux API, Al Viro, Andy Lutomirski, Kenton Varda,
Michael Kerrisk-manpages, Greg Kroah-Hartman,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Tejun Heo, Ivan Delalande,
Linus Torvalds
In-Reply-To: <87h9pcyokc.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>
Am 10.07.2015 um 18:17 schrieb Eric W. Biederman:
>
> Today proc and sysfs do not contain any executable files. Several
> applications today mount proc or sysfs without noexec and nosuid and
> then depend on there being no exectuables files on proc or sysfs.
> Having any executable files show on proc or sysfs would cause
> a user space visible regression, and most likely security problems.
>
> Therefore commit to never allowing executables on proc and sysfs by
> adding a new flag to mark them as filesystems without executables and
> enforce that flag.
>
> Test the flag where MNT_NOEXEC is tested today, so that the only user
> visible effect will be that exectuables will be treated as if the
> execute bit is cleared.
>
> The filesystems proc and sysfs do not currently incoporate any
> executable files so this does not result in any user visible effects.
>
> This makes it unnecessary to vet changes to proc and sysfs tightly for
> adding exectuable files or changes to chattr that would modify
> existing files, as no matter what the individual file say they will
> not be treated as exectuable files by the vfs.
>
> Not having to vet changes to closely is important as without this we
> are only one proc_create call (or another goof up in the
> implementation of notify_change) from having problematic executables
> on proc. Those mistakes are all too easy to make and would create
> a situation where there are security issues or the assumptions of
> some program having to be broken (and cause userspace regressions).
Would it make sense to add SB_I_NOEXEC to more pseudo filesystems?
Say pstore or devpts?
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH v7 4/9] nvmem: Add bindings for simple nvmem framework
From: Rob Herring @ 2015-07-10 19:04 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Greg Kroah-Hartman, Rob Herring, Kumar Gala, Mark Brown,
Sascha Hauer, linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-msm,
Arnd Bergmann, Stephen Boyd, Pantelis Antoniou, Matt Porter,
Stefan Wahren, wxt-TNX95d0MmH7DzftRWevZcw, Maxime Ripard
In-Reply-To: <1436521513-10838-1-git-send-email-srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Fri, Jul 10, 2015 at 4:45 AM, Srinivas Kandagatla
<srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> This patch adds bindings for simple nvmem framework which allows nvmem
> consumers to talk to nvmem providers to get access to nvmem cell data.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> [Maxime Ripard: intial version of eeprom framework]
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Documentation/devicetree/bindings/nvmem/nvmem.txt | 85 +++++++++++++++++++++++
> 1 file changed, 85 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvmem/nvmem.txt
>
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.txt b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> new file mode 100644
> index 0000000..849f1e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.txt
> @@ -0,0 +1,85 @@
> += NVMEM(Non Volatile Memory) Data Device Tree Bindings =
> +
> +This binding is intended to represent the location of hardware
> +configuration data stored in NVMEMs like eeprom, efuses and so on.
> +
> +On a significant proportion of boards, the manufacturer has stored
> +some data on NVMEM, for the OS to be able to retrieve these information
> +and act upon it. Obviously, the OS has to know about where to retrieve
> +these data from, and where they are stored on the storage device.
> +
> +This document is here to document this.
> +
> += Data providers =
> +Contains bindings specific to provider drivers and data cells as children
> +of this node.
#address-cells and #size-cells are required here.
> +
> +Optional properties:
> + read-only: Mark the provider as read only.
Couldn't this be per field rather than global?
> +
> += Data cells =
> +These are the child nodes of the provider which contain data cell
> +information like offset and size in nvmem provider.
> +
> +Required properties:
> +reg: specifies the offset in byte within that storage device, start bit
> + in the byte and the length in bytes of the data we care about.
> + There could be more than one offset-length pairs in this property.
> +
> +Optional properties:
> +
> +bit-offset: specifies the offset in bit within the address range specified
> + by reg property. Can take values from 0-7.
> +nbits: specifies number of bits this cell occupies starting from bit-offset.
How about just: "bits = <<offset> <size>>"
Then the bit specification is more aligned with the byte location
(i.e. reg property).
You could also do this all in the reg property with 2 address cells
for byte and bit position and then size can be in bits. reg doesn't
have to match a memory mapped bus addressing meanings. If you wanted
to handle ranges and address translation, then you would need custom
functions like PCI does. I'm not sure you would need that.
> +
> +For example:
> +
> + /* Provider */
> + qfprom: qfprom@00700000 {
> + ...
> +
> + /* Data cells */
> + tsens_calibration: calib@404 {
> + reg = <0x404 0x10>;
> + };
> +
> + tsens_calibration_bckp: calib_bckp@504 {
> + reg = <0x504 0x11>;
> + bit-offset = 6;
> + nbits = 128;
> + };
> +
> + pvs_version: pvs-version@6 {
> + reg = <0x6 0x2>
> + bit-offset = 7;
> + nbits = 2;
> + };
> +
> + speed_bin: speed-bin@c{
> + reg = <0xc 0x1>;
> + bit-offset = 2;
> + nbits = 3;
> +
> + };
> + ...
> + };
> +
> += Data consumers =
> +Are device nodes which consume nvmem data cells/providers.
> +
> +Required-properties:
> +nvmem-cells: list of phandle to the nvmem data cells.
> +nvmem-cell-names: names for the each nvmem-cells specified. Required if
> + nvmem-cells is used.
> +
> +Optional-properties:
> +nvmem : list of phandles to nvmem providers.
> +nvmem-names: names for the each nvmem provider. required if nvmem is used.
> +
> +For example:
> +
> + tsens {
> + ...
> + nvmem-cells = <&tsens_calibration>;
> + nvmem-cell-names = "calibration";
> + };
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH -mm v6 5/6] proc: add kpageidle file
From: Andres Lagar-Cavilla @ 2015-07-10 19:10 UTC (permalink / raw)
To: Vladimir Davydov
Cc: Andrew Morton, Minchan Kim, Raghavendra K T, Johannes Weiner,
Michal Hocko, Greg Thelen, Michel Lespinasse, David Rientjes,
Pavel Emelyanov, Cyrill Gorcunov, Jonathan Corbet,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
cgroups-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150709131900.GK2436@esperanza>
Hi Vladimir,
On Thu, Jul 9, 2015 at 6:19 AM, Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:
> Hi Andres,
>
> On Wed, Jul 08, 2015 at 04:01:13PM -0700, Andres Lagar-Cavilla wrote:
>> On Fri, Jun 12, 2015 at 2:52 AM, Vladimir Davydov
> [...]
>> > @@ -275,6 +276,179 @@ static const struct file_operations proc_kpagecgroup_operations = {
>> > };
>> > #endif /* CONFIG_MEMCG */
>> >
>> > +#ifdef CONFIG_IDLE_PAGE_TRACKING
>> > +/*
>> > + * Idle page tracking only considers user memory pages, for other types of
>> > + * pages the idle flag is always unset and an attempt to set it is silently
>> > + * ignored.
>> > + *
>> > + * We treat a page as a user memory page if it is on an LRU list, because it is
>> > + * always safe to pass such a page to page_referenced(), which is essential for
>> > + * idle page tracking. With such an indicator of user pages we can skip
>> > + * isolated pages, but since there are not usually many of them, it will hardly
>> > + * affect the overall result.
>> > + *
>> > + * This function tries to get a user memory page by pfn as described above.
>> > + */
>> > +static struct page *kpageidle_get_page(unsigned long pfn)
>> > +{
>> > + struct page *page;
>> > + struct zone *zone;
>> > +
>> > + if (!pfn_valid(pfn))
>> > + return NULL;
>> > +
>> > + page = pfn_to_page(pfn);
>> > + if (!page || !PageLRU(page))
>>
>> Isolation can race in while you're processing the page, after these
>> checks. This is ok, but worth a small comment.
>
> Agree, will add one.
>
>>
>> > + return NULL;
>> > + if (!get_page_unless_zero(page))
>> > + return NULL;
>> > +
>> > + zone = page_zone(page);
>> > + spin_lock_irq(&zone->lru_lock);
>> > + if (unlikely(!PageLRU(page))) {
>> > + put_page(page);
>> > + page = NULL;
>> > + }
>> > + spin_unlock_irq(&zone->lru_lock);
>> > + return page;
>> > +}
>> > +
>> > +/*
>> > + * This function calls page_referenced() to clear the referenced bit for all
>> > + * mappings to a page. Since the latter also clears the page idle flag if the
>> > + * page was referenced, it can be used to update the idle flag of a page.
>> > + */
>> > +static void kpageidle_clear_pte_refs(struct page *page)
>> > +{
>> > + unsigned long dummy;
>> > +
>> > + if (page_referenced(page, 0, NULL, &dummy, NULL))
>>
>> Because of pte/pmd_clear_flush_young* called in the guts of
>> page_referenced_one, an N byte write or read to /proc/kpageidle will
>> cause N * 64 TLB flushes.
>>
>> Additionally, because of the _notify connection to mmu notifiers, this
>> will also cause N * 64 EPT TLB flushes (in the KVM Intel case, similar
>> for other notifier flavors, you get the point).
>>
>> The solution is relatively straightforward: augment
>> page_referenced_one with a mode marker or boolean that determines
>> whether tlb flushing is required.
>
> Frankly, I don't think that tlb flushes are such a big deal in the scope
> of this feature, because one is not supposed to write to kpageidle that
> often. However, I agree we'd better avoid overhead we can easily avoid,
> so I'll add a new flag to differentiate between kpageidle and reclaim
> path in page_referenced, as you suggested.
Yes, it's a performance optimization, but fairly critical. Once you
open up a user-space interface, it will take off. What prevents people
from writing a daemon that will scan the entire host space every N
seconds (N=10? 60? 90? 120?). That means tens or hundreds of millions
of individual TLB flushes, which will hurt performance.
The KVM issue is not minor.
>
>>
>> For an access pattern tracker such as the one you propose, flushing is
>> not strictly necessary: the next context switch will take care. Too
>> bad if you missed a few accesses because the pte/pmd was loaded in the
>> TLB. Not so easy for MMU notifiers, because each secondary MMU has its
>> own semantics. You could arguably throw the towel in there, or try to
>> provide a framework (i.e. propagate the flushing flag) and let each
>> implementation fill the gaps.
>>
>> > + /*
>> > + * We cleared the referenced bit in a mapping to this page. To
>> > + * avoid interference with the reclaimer, mark it young so that
>> > + * the next call to page_referenced() will also return > 0 (see
>> > + * page_referenced_one())
>> > + */
>> > + set_page_young(page);
>> > +}
>> > +
>> > +static ssize_t kpageidle_read(struct file *file, char __user *buf,
>> > + size_t count, loff_t *ppos)
>> > +{
>> > + u64 __user *out = (u64 __user *)buf;
>> > + struct page *page;
>> > + unsigned long pfn, end_pfn;
>> > + ssize_t ret = 0;
>> > + u64 idle_bitmap = 0;
>> > + int bit;
>> > +
>> > + if (*ppos & KPMMASK || count & KPMMASK)
>> > + return -EINVAL;
>> > +
>> > + pfn = *ppos * BITS_PER_BYTE;
>> > + if (pfn >= max_pfn)
>> > + return 0;
>> > +
>> > + end_pfn = pfn + count * BITS_PER_BYTE;
>> > + if (end_pfn > max_pfn)
>> > + end_pfn = ALIGN(max_pfn, KPMBITS);
>> > +
>> > + for (; pfn < end_pfn; pfn++) {
>> > + bit = pfn % KPMBITS;
>> > + page = kpageidle_get_page(pfn);
>> > + if (page) {
>> > + if (page_is_idle(page)) {
>> > + /*
>> > + * The page might have been referenced via a
>> > + * pte, in which case it is not idle. Clear
>> > + * refs and recheck.
>> > + */
>> > + kpageidle_clear_pte_refs(page);
>> > + if (page_is_idle(page))
>> > + idle_bitmap |= 1ULL << bit;
>> > + }
>> > + put_page(page);
>> > + }
>> > + if (bit == KPMBITS - 1) {
>> > + if (put_user(idle_bitmap, out)) {
>> > + ret = -EFAULT;
>> > + break;
>> > + }
>> > + idle_bitmap = 0;
>> > + out++;
>> > + }
>> > + }
>> > +
>> > + *ppos += (char __user *)out - buf;
>> > + if (!ret)
>> > + ret = (char __user *)out - buf;
>> > + return ret;
>> > +}
>> > +
>> > +static ssize_t kpageidle_write(struct file *file, const char __user *buf,
>>
>> Your reasoning for a host wide /proc/kpageidle is well argued, but I'm
>> still hesitant.
>>
>> mincore() shows how to (relatively simply) resolve unmapped file pages
>> to their backing page cache destination. You could recycle that code
>> and then you'd have per process idle/idling interfaces. With the
>> advantage of a clear TLB flush demarcation.
>
> Hmm, I still don't see how we could handle page cache that does not
> belong to any process in the scope of sys_mincore.
You're correct.
I wasn't asking to use mincore, just pointing out an extant code
pattern that could get you beyond the concerns re unmapping (and which
can be implemented as a proc file).
My view is that the key pieces of infrastructure (the flags, the
interactions with page_referenced_one, clear_refs, mark_page_accessed)
your patchset brings along then can be reused in many ways. Michel
Lespinasse's kernel thread can reuse them, or proc/smaps can be
augmented (or a new proc entry), to get per process idle maps.
So /proc/kpageidle is fine with me, but not crazy appealing.
>
> Besides, it'd be awkward to reuse sys_mincore for idle page tracking,
> because we need two operations, set idle and check idle, while the
> sys_mincore semantic implies only getting information from the kernel,
> not vice versa.
>
> Of course, we could introduce a separate syscall, say sys_idlecore, but
> IMO it is not a good idea to add a syscall for such a specific feature,
> which can be compiled out. I think a proc file suits better for the
> purpose, especially counting that we have a bunch of similar files
> (pagemap, kpageflags, kpagecount).
>
> Anyway, I'm open for suggestions. If you have a different user API
> design in mind, which in your opinion would fit better, please share.
>
>>
>> > + size_t count, loff_t *ppos)
>> > +{
>> > + const u64 __user *in = (const u64 __user *)buf;
>> > + struct page *page;
>> > + unsigned long pfn, end_pfn;
>> > + ssize_t ret = 0;
>> > + u64 idle_bitmap = 0;
>> > + int bit;
>> > +
>> > + if (*ppos & KPMMASK || count & KPMMASK)
>> > + return -EINVAL;
>> > +
>> > + pfn = *ppos * BITS_PER_BYTE;
>> > + if (pfn >= max_pfn)
>> > + return -ENXIO;
>> > +
>> > + end_pfn = pfn + count * BITS_PER_BYTE;
>> > + if (end_pfn > max_pfn)
>> > + end_pfn = ALIGN(max_pfn, KPMBITS);
>> > +
>> > + for (; pfn < end_pfn; pfn++) {
>>
>> Relatively straight forward to teleport forward 512 (or more
>> correctly: 1 << compound_order(page)) pages for THP pages, once done
>> with a THP head, and avoid 511 fruitless trips down rmap.c for each
>> tail.
>
> Right, will fix.
>
>>
>> > + bit = pfn % KPMBITS;
>> > + if (bit == 0) {
>> > + if (get_user(idle_bitmap, in)) {
>> > + ret = -EFAULT;
>> > + break;
>> > + }
>> > + in++;
>> > + }
>> > + if (idle_bitmap >> bit & 1) {
>> > + page = kpageidle_get_page(pfn);
>> > + if (page) {
>> > + kpageidle_clear_pte_refs(page);
>> > + set_page_idle(page);
>>
>> In the common case this will make a page both young and idle. This is
>> fine. We will come back to it below.
>>
>> > + put_page(page);
>> > + }
>> > + }
>> > + }
>> > +
>> > + *ppos += (const char __user *)in - buf;
>> > + if (!ret)
>> > + ret = (const char __user *)in - buf;
>> > + return ret;
>> > +}
>> > +
>> > +static const struct file_operations proc_kpageidle_operations = {
>> > + .llseek = mem_lseek,
>> > + .read = kpageidle_read,
>> > + .write = kpageidle_write,
>> > +};
>> > +
>> > +#ifndef CONFIG_64BIT
>> > +static bool need_page_idle(void)
>> > +{
>> > + return true;
>> > +}
>> > +struct page_ext_operations page_idle_ops = {
>> > + .need = need_page_idle,
>> > +};
>> > +#endif
>> > +#endif /* CONFIG_IDLE_PAGE_TRACKING */
>> > +
>> > static int __init proc_page_init(void)
>> > {
>> > proc_create("kpagecount", S_IRUSR, NULL, &proc_kpagecount_operations);
> [...]
>> > @@ -798,6 +798,14 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
>> > pte_unmap_unlock(pte, ptl);
>>
>> This is not in your patch, but further up in page_referenced_one there
>> is the pmd case.
>>
>> So what happens on THP split? That was a leading question: you should
>> propagate the young and idle flags to the split-up tail pages.
>
> Good catch! I completely forgot about THP slit. Will fix in the next
> iteration.
>
>>
>> > }
>> >
>> > + if (referenced && page_is_idle(page))
>> > + clear_page_idle(page);
>>
>> Is it so expensive to just call clear without the test .. ?
>
> This function is normally called from a relatively cold path - memory
> reclaim, where we modify page->flags anyway, so I think it won't make
> any difference if we drop this check.
>
>>
>> > +
>> > + if (page_is_young(page)) {
>> > + clear_page_young(page);
>>
>> referenced += test_and_clear_page_young(page) .. ?
>
> Yeah, that does look better.
>
>>
>> > + referenced++;
>> > + }
>> > +
>>
>> Invert the order. A page can be both young and idle -- we noted that
>> closer to the top of the patch.
>>
>> So young bumps referenced up, and then the final referenced value is
>> used to clear idle.
>
> I don't think it'd work. Look, kpageidle_write clears pte references and
> sets the idle flag. If the page was referenced it also sets the young
> flag in order not to interfere with the reclaimer. When kpageidle_read
> is called afterwards, it must see the idle flag set iff the page has not
> been referenced since kpageidle_write set it. However, if
> page_referenced was not called on the page from the reclaim path, it
> will still be young no matter if it has been referenced or not and
> therefore will always be identified as not idle, which is incorrect.
You're right. Thanks!
Andres
>
>>
>> > if (referenced) {
>>
>> At this point, if you follow my suggestion of augmenting
>> page_referenced_one with a mode indicator (for TLB flushing), you can
>> set page young here. There is the added benefit of holding the
>> mmap_mutex lock or vma_lock, which prevents reclaim, try_to_unmap,
>> migration, from exploiting a small window where page young is not set
>> but should.
>
> Yeah, if we go with the page_referenced mode switcher you suggested
> above, it's definitely worth moving set_page_young here.
>
> Thank you for the review!
>
> Vladimir
>
>>
>> > pra->referenced++;
>> > pra->vm_flags |= vma->vm_flags;
>> > diff --git a/mm/swap.c b/mm/swap.c
>> > index ab7c338eda87..db43c9b4891d 100644
>> > --- a/mm/swap.c
>> > +++ b/mm/swap.c
>> > @@ -623,6 +623,8 @@ void mark_page_accessed(struct page *page)
>> > } else if (!PageReferenced(page)) {
>> > SetPageReferenced(page);
>> > }
>> > + if (page_is_idle(page))
>> > + clear_page_idle(page);
>> > }
>> > EXPORT_SYMBOL(mark_page_accessed);
>> >
--
Andres Lagar-Cavilla | Google Kernel Team | andreslc-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
^ permalink raw reply
* Re: [REVIEW][PATCH 1/2] vfs: Commit to never having exectuables on proc and sysfs.
From: Greg Kroah-Hartman @ 2015-07-10 19:30 UTC (permalink / raw)
To: Richard Weinberger
Cc: Eric W. Biederman, Linux Containers, Linus Torvalds,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux API, Andy Lutomirski,
Serge E. Hallyn, Kenton Varda, Michael Kerrisk-manpages,
Stéphane Graber, Eric Windisch, Tejun Heo, Seth Forshee,
Omar Sandoval, Ivan Delalande, Al Viro
In-Reply-To: <55A00DE9.7060806-/L3Ra7n9ekc@public.gmane.org>
On Fri, Jul 10, 2015 at 08:24:41PM +0200, Richard Weinberger wrote:
> Am 10.07.2015 um 18:17 schrieb Eric W. Biederman:
> >
> > Today proc and sysfs do not contain any executable files. Several
> > applications today mount proc or sysfs without noexec and nosuid and
> > then depend on there being no exectuables files on proc or sysfs.
> > Having any executable files show on proc or sysfs would cause
> > a user space visible regression, and most likely security problems.
> >
> > Therefore commit to never allowing executables on proc and sysfs by
> > adding a new flag to mark them as filesystems without executables and
> > enforce that flag.
> >
> > Test the flag where MNT_NOEXEC is tested today, so that the only user
> > visible effect will be that exectuables will be treated as if the
> > execute bit is cleared.
> >
> > The filesystems proc and sysfs do not currently incoporate any
> > executable files so this does not result in any user visible effects.
> >
> > This makes it unnecessary to vet changes to proc and sysfs tightly for
> > adding exectuable files or changes to chattr that would modify
> > existing files, as no matter what the individual file say they will
> > not be treated as exectuable files by the vfs.
> >
> > Not having to vet changes to closely is important as without this we
> > are only one proc_create call (or another goof up in the
> > implementation of notify_change) from having problematic executables
> > on proc. Those mistakes are all too easy to make and would create
> > a situation where there are security issues or the assumptions of
> > some program having to be broken (and cause userspace regressions).
>
> Would it make sense to add SB_I_NOEXEC to more pseudo filesystems?
> Say pstore or devpts?
And configfs and cgroupfs?
^ permalink raw reply
* Re: [REVIEW][PATCH 1/2] vfs: Commit to never having exectuables on proc and sysfs.
From: Richard Weinberger @ 2015-07-10 19:38 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Eric W. Biederman, Linux Containers, Linus Torvalds,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux API, Andy Lutomirski,
Serge E. Hallyn, Kenton Varda, Michael Kerrisk-manpages,
Stéphane Graber, Eric Windisch, Tejun Heo, Seth Forshee,
Omar Sandoval, Ivan Delalande, Al Viro
In-Reply-To: <20150710193052.GB19824-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Am 10.07.2015 um 21:30 schrieb Greg Kroah-Hartman:
> On Fri, Jul 10, 2015 at 08:24:41PM +0200, Richard Weinberger wrote:
>> Am 10.07.2015 um 18:17 schrieb Eric W. Biederman:
>>>
>>> Today proc and sysfs do not contain any executable files. Several
>>> applications today mount proc or sysfs without noexec and nosuid and
>>> then depend on there being no exectuables files on proc or sysfs.
>>> Having any executable files show on proc or sysfs would cause
>>> a user space visible regression, and most likely security problems.
>>>
>>> Therefore commit to never allowing executables on proc and sysfs by
>>> adding a new flag to mark them as filesystems without executables and
>>> enforce that flag.
>>>
>>> Test the flag where MNT_NOEXEC is tested today, so that the only user
>>> visible effect will be that exectuables will be treated as if the
>>> execute bit is cleared.
>>>
>>> The filesystems proc and sysfs do not currently incoporate any
>>> executable files so this does not result in any user visible effects.
>>>
>>> This makes it unnecessary to vet changes to proc and sysfs tightly for
>>> adding exectuable files or changes to chattr that would modify
>>> existing files, as no matter what the individual file say they will
>>> not be treated as exectuable files by the vfs.
>>>
>>> Not having to vet changes to closely is important as without this we
>>> are only one proc_create call (or another goof up in the
>>> implementation of notify_change) from having problematic executables
>>> on proc. Those mistakes are all too easy to make and would create
>>> a situation where there are security issues or the assumptions of
>>> some program having to be broken (and cause userspace regressions).
>>
>> Would it make sense to add SB_I_NOEXEC to more pseudo filesystems?
>> Say pstore or devpts?
>
> And configfs and cgroupfs?
Yep. Any filesystem where exectuables do not make sense. :-)
Thanks,
//richard
^ permalink raw reply
* Re: [REVIEW][PATCH 1/2] vfs: Commit to never having exectuables on proc and sysfs.
From: Eric W. Biederman @ 2015-07-10 20:00 UTC (permalink / raw)
To: Richard Weinberger
Cc: Greg Kroah-Hartman, Linux Containers, Linus Torvalds,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Linux API, Andy Lutomirski,
Serge E. Hallyn, Kenton Varda, Michael Kerrisk-manpages,
Stéphane Graber, Eric Windisch, Tejun Heo, Seth Forshee,
Omar Sandoval, Ivan Delalande, Al Viro
In-Reply-To: <55A01F4B.9010205-/L3Ra7n9ekc@public.gmane.org>
Richard Weinberger <richard-/L3Ra7n9ekc@public.gmane.org> writes:
> Am 10.07.2015 um 21:30 schrieb Greg Kroah-Hartman:
>> On Fri, Jul 10, 2015 at 08:24:41PM +0200, Richard Weinberger wrote:
>>> Am 10.07.2015 um 18:17 schrieb Eric W. Biederman:
>>>>
>>>> Today proc and sysfs do not contain any executable files. Several
>>>> applications today mount proc or sysfs without noexec and nosuid and
>>>> then depend on there being no exectuables files on proc or sysfs.
>>>> Having any executable files show on proc or sysfs would cause
>>>> a user space visible regression, and most likely security problems.
>>>>
>>>> Therefore commit to never allowing executables on proc and sysfs by
>>>> adding a new flag to mark them as filesystems without executables and
>>>> enforce that flag.
>>>>
>>>> Test the flag where MNT_NOEXEC is tested today, so that the only user
>>>> visible effect will be that exectuables will be treated as if the
>>>> execute bit is cleared.
>>>>
>>>> The filesystems proc and sysfs do not currently incoporate any
>>>> executable files so this does not result in any user visible effects.
>>>>
>>>> This makes it unnecessary to vet changes to proc and sysfs tightly for
>>>> adding exectuable files or changes to chattr that would modify
>>>> existing files, as no matter what the individual file say they will
>>>> not be treated as exectuable files by the vfs.
>>>>
>>>> Not having to vet changes to closely is important as without this we
>>>> are only one proc_create call (or another goof up in the
>>>> implementation of notify_change) from having problematic executables
>>>> on proc. Those mistakes are all too easy to make and would create
>>>> a situation where there are security issues or the assumptions of
>>>> some program having to be broken (and cause userspace regressions).
>>>
>>> Would it make sense to add SB_I_NOEXEC to more pseudo filesystems?
>>> Say pstore or devpts?
>>
>> And configfs and cgroupfs?
>
> Yep. Any filesystem where exectuables do not make sense. :-)
With a threat model of a developer overlooking something and allows
executables by accident? I certainly think it makes sense.
Mostly because we are solving things at the vfs level. Which gives us
one well tested solution instead of several filesystem specific
solutions.
That isn't quite the same problem that caused me to write this code,
so I am not going to volunteer to write the patches for the additional
filesystems.
Eric
^ permalink raw reply
* [PATCH 0/3] sys_membarrier (x86, generic)
From: Mathieu Desnoyers @ 2015-07-10 20:58 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, linux-api, Mathieu Desnoyers
Hi Andrew,
Here is a repost of sys_membarrier, rebased on top of Linus commit
c4b5fd3fb2058b650447372472ad24e2a989f9f6 without any change since the
last v19 post other that proceeding to further testing. When merging
with other system calls, system call number conflicts should be quite
straightforward to handle, there is nothing special there.
Please consider pulling it into your tree in preparation for the
following merge window.
Thanks!
Mathieu
Mathieu Desnoyers (2):
sys_membarrier(): system-wide memory barrier (generic, x86)
selftests: enhance membarrier syscall test
Pranith Kumar (1):
selftests: add membarrier syscall test
MAINTAINERS | 8 ++
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 2 +
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/membarrier.h | 53 +++++++++
init/Kconfig | 12 ++
kernel/Makefile | 1 +
kernel/membarrier.c | 66 +++++++++++
kernel/sys_ni.c | 3 +
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/membarrier/.gitignore | 1 +
tools/testing/selftests/membarrier/Makefile | 11 ++
.../testing/selftests/membarrier/membarrier_test.c | 121 +++++++++++++++++++++
15 files changed, 285 insertions(+), 1 deletion(-)
create mode 100644 include/uapi/linux/membarrier.h
create mode 100644 kernel/membarrier.c
create mode 100644 tools/testing/selftests/membarrier/.gitignore
create mode 100644 tools/testing/selftests/membarrier/Makefile
create mode 100644 tools/testing/selftests/membarrier/membarrier_test.c
--
2.1.4
^ permalink raw reply
* [PATCH 1/3 v19] sys_membarrier(): system-wide memory barrier (generic, x86)
From: Mathieu Desnoyers @ 2015-07-10 20:58 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-api, Mathieu Desnoyers, KOSAKI Motohiro,
Steven Rostedt, Nicholas Miell, Linus Torvalds, Ingo Molnar,
Alan Cox, Lai Jiangshan, Stephen Hemminger, Thomas Gleixner,
Peter Zijlstra, David Howells, Pranith Kumar, Michael Kerrisk
In-Reply-To: <1436561912-24365-1-git-send-email-mathieu.desnoyers@efficios.com>
Here is an implementation of a new system call, sys_membarrier(), which
executes a memory barrier on all threads running on the system. It is
implemented by calling synchronize_sched(). It can be used to distribute
the cost of user-space memory barriers asymmetrically by transforming
pairs of memory barriers into pairs consisting of sys_membarrier() and a
compiler barrier. For synchronization primitives that distinguish
between read-side and write-side (e.g. userspace RCU [1], rwlocks), the
read-side can be accelerated significantly by moving the bulk of the
memory barrier overhead to the write-side.
The existing applications of which I am aware that would be improved by this
system call are as follows:
* Through Userspace RCU library (http://urcu.so)
- DNS server (Knot DNS) https://www.knot-dns.cz/
- Network sniffer (http://netsniff-ng.org/)
- Distributed object storage (https://sheepdog.github.io/sheepdog/)
- User-space tracing (http://lttng.org)
- Network storage system (https://www.gluster.org/)
- Virtual routers (https://events.linuxfoundation.org/sites/events/files/slides/DPDK_RCU_0MQ.pdf)
- Financial software (https://lkml.org/lkml/2015/3/23/189)
Those projects use RCU in userspace to increase read-side speed and
scalability compared to locking. Especially in the case of RCU used
by libraries, sys_membarrier can speed up the read-side by moving the
bulk of the memory barrier cost to synchronize_rcu().
* Direct users of sys_membarrier
- core dotnet garbage collector (https://github.com/dotnet/coreclr/issues/198)
Microsoft core dotnet GC developers are planning to use the mprotect()
side-effect of issuing memory barriers through IPIs as a way to implement
Windows FlushProcessWriteBuffers() on Linux. They are referring to
sys_membarrier in their github thread, specifically stating that
sys_membarrier() is what they are looking for.
This implementation is based on kernel v4.1-rc8.
To explain the benefit of this scheme, let's introduce two example threads:
Thread A (non-frequent, e.g. executing liburcu synchronize_rcu())
Thread B (frequent, e.g. executing liburcu
rcu_read_lock()/rcu_read_unlock())
In a scheme where all smp_mb() in thread A are ordering memory accesses
with respect to smp_mb() present in Thread B, we can change each
smp_mb() within Thread A into calls to sys_membarrier() and each
smp_mb() within Thread B into compiler barriers "barrier()".
Before the change, we had, for each smp_mb() pairs:
Thread A Thread B
previous mem accesses previous mem accesses
smp_mb() smp_mb()
following mem accesses following mem accesses
After the change, these pairs become:
Thread A Thread B
prev mem accesses prev mem accesses
sys_membarrier() barrier()
follow mem accesses follow mem accesses
As we can see, there are two possible scenarios: either Thread B memory
accesses do not happen concurrently with Thread A accesses (1), or they
do (2).
1) Non-concurrent Thread A vs Thread B accesses:
Thread A Thread B
prev mem accesses
sys_membarrier()
follow mem accesses
prev mem accesses
barrier()
follow mem accesses
In this case, thread B accesses will be weakly ordered. This is OK,
because at that point, thread A is not particularly interested in
ordering them with respect to its own accesses.
2) Concurrent Thread A vs Thread B accesses
Thread A Thread B
prev mem accesses prev mem accesses
sys_membarrier() barrier()
follow mem accesses follow mem accesses
In this case, thread B accesses, which are ensured to be in program
order thanks to the compiler barrier, will be "upgraded" to full
smp_mb() by synchronize_sched().
* Benchmarks
On Intel Xeon E5405 (8 cores)
(one thread is calling sys_membarrier, the other 7 threads are busy
looping)
1000 non-expedited sys_membarrier calls in 33s = 33 milliseconds/call.
* User-space user of this system call: Userspace RCU library
Both the signal-based and the sys_membarrier userspace RCU schemes
permit us to remove the memory barrier from the userspace RCU
rcu_read_lock() and rcu_read_unlock() primitives, thus significantly
accelerating them. These memory barriers are replaced by compiler
barriers on the read-side, and all matching memory barriers on the
write-side are turned into an invocation of a memory barrier on all
active threads in the process. By letting the kernel perform this
synchronization rather than dumbly sending a signal to every process
threads (as we currently do), we diminish the number of unnecessary wake
ups and only issue the memory barriers on active threads. Non-running
threads do not need to execute such barrier anyway, because these are
implied by the scheduler context switches.
Results in liburcu:
Operations in 10s, 6 readers, 2 writers:
memory barriers in reader: 1701557485 reads, 2202847 writes
signal-based scheme: 9830061167 reads, 6700 writes
sys_membarrier: 9952759104 reads, 425 writes
sys_membarrier (dyn. check): 7970328887 reads, 425 writes
The dynamic sys_membarrier availability check adds some overhead to
the read-side compared to the signal-based scheme, but besides that,
sys_membarrier slightly outperforms the signal-based scheme. However,
this non-expedited sys_membarrier implementation has a much slower grace
period than signal and memory barrier schemes.
Besides diminishing the number of wake-ups, one major advantage of the
membarrier system call over the signal-based scheme is that it does not
need to reserve a signal. This plays much more nicely with libraries,
and with processes injected into for tracing purposes, for which we
cannot expect that signals will be unused by the application.
An expedited version of this system call can be added later on to speed
up the grace period. Its implementation will likely depend on reading
the cpu_curr()->mm without holding each CPU's rq lock.
This patch adds the system call to x86 and to asm-generic.
[1] http://urcu.so
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Nicholas Miell <nmiell@comcast.net>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Ingo Molnar <mingo@redhat.com>
CC: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
CC: Lai Jiangshan <laijs@cn.fujitsu.com>
CC: Stephen Hemminger <stephen@networkplumber.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Peter Zijlstra <peterz@infradead.org>
CC: David Howells <dhowells@redhat.com>
CC: Pranith Kumar <bobby.prani@gmail.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: linux-api@vger.kernel.org
---
membarrier(2) man page:
--------------- snip -------------------
MEMBARRIER(2) Linux Programmer's Manual MEMBARRIER(2)
NAME
membarrier - issue memory barriers on a set of threads
SYNOPSIS
#include <linux/membarrier.h>
int membarrier(int cmd, int flags);
DESCRIPTION
The cmd argument is one of the following:
MEMBARRIER_CMD_QUERY
Query the set of supported commands. It returns a bitmask of
supported commands.
MEMBARRIER_CMD_SHARED
Execute a memory barrier on all threads running on the system.
Upon return from system call, the caller thread is ensured that
all running threads have passed through a state where all memory
accesses to user-space addresses match program order between
entry to and return from the system call (non-running threads
are de facto in such a state). This covers threads from all pro‐
cesses running on the system. This command returns 0.
The flags argument needs to be 0. For future extensions.
All memory accesses performed in program order from each targeted
thread is guaranteed to be ordered with respect to sys_membarrier(). If
we use the semantic "barrier()" to represent a compiler barrier forcing
memory accesses to be performed in program order across the barrier,
and smp_mb() to represent explicit memory barriers forcing full memory
ordering across the barrier, we have the following ordering table for
each pair of barrier(), sys_membarrier() and smp_mb():
The pair ordering is detailed as (O: ordered, X: not ordered):
barrier() smp_mb() sys_membarrier()
barrier() X X O
smp_mb() X O O
sys_membarrier() O O O
RETURN VALUE
On success, these system calls return zero. On error, -1 is returned,
and errno is set appropriately. For a given command, with flags
argument set to 0, this system call is guaranteed to always return the
same value until reboot.
ERRORS
ENOSYS System call is not implemented.
EINVAL Invalid arguments.
Linux 2015-04-15 MEMBARRIER(2)
--------------- snip -------------------
Changes since v18:
- Add unlikely() check to flags,
- Describe current users in changelog.
Changes since v17:
- Update commit message.
Changes since v16:
- Update documentation.
- Add man page to changelog.
- Build sys_membarrier on !CONFIG_SMP. It allows userspace applications
to not care about the number of processors on the system. Based on
recommendations from Stephen Hemminger and Steven Rostedt.
- Check that flags argument is 0, update documentation to require it.
Changes since v15:
- Add flags argument in addition to cmd.
- Update documentation.
Changes since v14:
- Take care of Thomas Gleixner's comments.
Changes since v13:
- Move to kernel/membarrier.c.
- Remove MEMBARRIER_PRIVATE flag.
- Add MAINTAINERS file entry.
Changes since v12:
- Remove _FLAG suffix from uapi flags.
- Add Expert menuconfig option CONFIG_MEMBARRIER (default=y).
- Remove EXPEDITED mode. Only implement non-expedited for now, until
reading the cpu_curr()->mm can be done without holding the CPU's rq
lock.
Changes since v11:
- 5 years have passed.
- Rebase on v3.19 kernel.
- Add futex-alike PRIVATE vs SHARED semantic: private for per-process
barriers, non-private for memory mappings shared between processes.
- Simplify user API.
- Code refactoring.
Changes since v10:
- Apply Randy's comments.
- Rebase on 2.6.34-rc4 -tip.
Changes since v9:
- Clean up #ifdef CONFIG_SMP.
Changes since v8:
- Go back to rq spin locks taken by sys_membarrier() rather than adding
memory barriers to the scheduler. It implies a potential RoS
(reduction of service) if sys_membarrier() is executed in a busy-loop
by a user, but nothing more than what is already possible with other
existing system calls, but saves memory barriers in the scheduler fast
path.
- re-add the memory barrier comments to x86 switch_mm() as an example to
other architectures.
- Update documentation of the memory barriers in sys_membarrier and
switch_mm().
- Append execution scenarios to the changelog showing the purpose of
each memory barrier.
Changes since v7:
- Move spinlock-mb and scheduler related changes to separate patches.
- Add support for sys_membarrier on x86_32.
- Only x86 32/64 system calls are reserved in this patch. It is planned
to incrementally reserve syscall IDs on other architectures as these
are tested.
Changes since v6:
- Remove some unlikely() not so unlikely.
- Add the proper scheduler memory barriers needed to only use the RCU
read lock in sys_membarrier rather than take each runqueue spinlock:
- Move memory barriers from per-architecture switch_mm() to schedule()
and finish_lock_switch(), where they clearly document that all data
protected by the rq lock is guaranteed to have memory barriers issued
between the scheduler update and the task execution. Replacing the
spin lock acquire/release barriers with these memory barriers imply
either no overhead (x86 spinlock atomic instruction already implies a
full mb) or some hopefully small overhead caused by the upgrade of the
spinlock acquire/release barriers to more heavyweight smp_mb().
- The "generic" version of spinlock-mb.h declares both a mapping to
standard spinlocks and full memory barriers. Each architecture can
specialize this header following their own need and declare
CONFIG_HAVE_SPINLOCK_MB to use their own spinlock-mb.h.
- Note: benchmarks of scheduler overhead with specialized spinlock-mb.h
implementations on a wide range of architecture would be welcome.
Changes since v5:
- Plan ahead for extensibility by introducing mandatory/optional masks
to the "flags" system call parameter. Past experience with accept4(),
signalfd4(), eventfd2(), epoll_create1(), dup3(), pipe2(), and
inotify_init1() indicates that this is the kind of thing we want to
plan for. Return -EINVAL if the mandatory flags received are unknown.
- Create include/linux/membarrier.h to define these flags.
- Add MEMBARRIER_QUERY optional flag.
Changes since v4:
- Add "int expedited" parameter, use synchronize_sched() in the
non-expedited case. Thanks to Lai Jiangshan for making us consider
seriously using synchronize_sched() to provide the low-overhead
membarrier scheme.
- Check num_online_cpus() == 1, quickly return without doing nothing.
Changes since v3a:
- Confirm that each CPU indeed runs the current task's ->mm before
sending an IPI. Ensures that we do not disturb RT tasks in the
presence of lazy TLB shootdown.
- Document memory barriers needed in switch_mm().
- Surround helper functions with #ifdef CONFIG_SMP.
Changes since v2:
- simply send-to-many to the mm_cpumask. It contains the list of
processors we have to IPI to (which use the mm), and this mask is
updated atomically.
Changes since v1:
- Only perform the IPI in CONFIG_SMP.
- Only perform the IPI if the process has more than one thread.
- Only send IPIs to CPUs involved with threads belonging to our process.
- Adaptative IPI scheme (single vs many IPI with threshold).
- Issue smp_mb() at the beginning and end of the system call.
---
MAINTAINERS | 8 +++++
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/syscalls.h | 2 ++
include/uapi/asm-generic/unistd.h | 4 ++-
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/membarrier.h | 53 +++++++++++++++++++++++++++
init/Kconfig | 12 +++++++
kernel/Makefile | 1 +
kernel/membarrier.c | 66 ++++++++++++++++++++++++++++++++++
kernel/sys_ni.c | 3 ++
11 files changed, 151 insertions(+), 1 deletion(-)
create mode 100644 include/uapi/linux/membarrier.h
create mode 100644 kernel/membarrier.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 0d70760..b560da6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6642,6 +6642,14 @@ W: http://www.mellanox.com
Q: http://patchwork.ozlabs.org/project/netdev/list/
F: drivers/net/ethernet/mellanox/mlx4/en_*
+MEMBARRIER SUPPORT
+M: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+M: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
+L: linux-kernel@vger.kernel.org
+S: Supported
+F: kernel/membarrier.c
+F: include/uapi/linux/membarrier.h
+
MEMORY MANAGEMENT
L: linux-mm@kvack.org
W: http://www.linux-mm.org
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index ef8187f..e63ad61 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -365,3 +365,4 @@
356 i386 memfd_create sys_memfd_create
357 i386 bpf sys_bpf
358 i386 execveat sys_execveat stub32_execveat
+359 i386 membarrier sys_membarrier
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 9ef32d5..87f3cd6 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -329,6 +329,7 @@
320 common kexec_file_load sys_kexec_file_load
321 common bpf sys_bpf
322 64 execveat stub_execveat
+323 common membarrier sys_membarrier
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index b45c45b..d4ab99b 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -884,4 +884,6 @@ asmlinkage long sys_execveat(int dfd, const char __user *filename,
const char __user *const __user *argv,
const char __user *const __user *envp, int flags);
+asmlinkage long sys_membarrier(int cmd, int flags);
+
#endif
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index e016bd9..8da542a 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -709,9 +709,11 @@ __SYSCALL(__NR_memfd_create, sys_memfd_create)
__SYSCALL(__NR_bpf, sys_bpf)
#define __NR_execveat 281
__SC_COMP(__NR_execveat, sys_execveat, compat_sys_execveat)
+#define __NR_membarrier 282
+__SYSCALL(__NR_membarrier, sys_membarrier)
#undef __NR_syscalls
-#define __NR_syscalls 282
+#define __NR_syscalls 283
/*
* All syscalls below here should go away really,
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 1ff9942..e6f229a 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -251,6 +251,7 @@ header-y += mdio.h
header-y += media.h
header-y += media-bus-format.h
header-y += mei.h
+header-y += membarrier.h
header-y += memfd.h
header-y += mempolicy.h
header-y += meye.h
diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
new file mode 100644
index 0000000..e0b108b
--- /dev/null
+++ b/include/uapi/linux/membarrier.h
@@ -0,0 +1,53 @@
+#ifndef _UAPI_LINUX_MEMBARRIER_H
+#define _UAPI_LINUX_MEMBARRIER_H
+
+/*
+ * linux/membarrier.h
+ *
+ * membarrier system call API
+ *
+ * Copyright (c) 2010, 2015 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+/**
+ * enum membarrier_cmd - membarrier system call command
+ * @MEMBARRIER_CMD_QUERY: Query the set of supported commands. It returns
+ * a bitmask of valid commands.
+ * @MEMBARRIER_CMD_SHARED: Execute a memory barrier on all running threads.
+ * Upon return from system call, the caller thread
+ * is ensured that all running threads have passed
+ * through a state where all memory accesses to
+ * user-space addresses match program order between
+ * entry to and return from the system call
+ * (non-running threads are de facto in such a
+ * state). This covers threads from all processes
+ * running on the system. This command returns 0.
+ *
+ * Command to be passed to the membarrier system call. The commands need to
+ * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
+ * the value 0.
+ */
+enum membarrier_cmd {
+ MEMBARRIER_CMD_QUERY = 0,
+ MEMBARRIER_CMD_SHARED = (1 << 0),
+};
+
+#endif /* _UAPI_LINUX_MEMBARRIER_H */
diff --git a/init/Kconfig b/init/Kconfig
index af09b4f..4bba60f 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1577,6 +1577,18 @@ config PCI_QUIRKS
bugs/quirks. Disable this only if your target machine is
unaffected by PCI quirks.
+config MEMBARRIER
+ bool "Enable membarrier() system call" if EXPERT
+ default y
+ help
+ Enable the membarrier() system call that allows issuing memory
+ barriers across all running threads, which can be used to distribute
+ the cost of user-space memory barriers asymmetrically by transforming
+ pairs of memory barriers into pairs consisting of membarrier() and a
+ compiler barrier.
+
+ If unsure, say Y.
+
config EMBEDDED
bool "Embedded system"
option allnoconfig_y
diff --git a/kernel/Makefile b/kernel/Makefile
index 43c4c92..92a481b 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -98,6 +98,7 @@ obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
obj-$(CONFIG_JUMP_LABEL) += jump_label.o
obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
obj-$(CONFIG_TORTURE_TEST) += torture.o
+obj-$(CONFIG_MEMBARRIER) += membarrier.o
$(obj)/configs.o: $(obj)/config_data.h
diff --git a/kernel/membarrier.c b/kernel/membarrier.c
new file mode 100644
index 0000000..536c727
--- /dev/null
+++ b/kernel/membarrier.c
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2010, 2015 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ *
+ * membarrier system call
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/syscalls.h>
+#include <linux/membarrier.h>
+
+/*
+ * Bitmask made from a "or" of all commands within enum membarrier_cmd,
+ * except MEMBARRIER_CMD_QUERY.
+ */
+#define MEMBARRIER_CMD_BITMASK (MEMBARRIER_CMD_SHARED)
+
+/**
+ * sys_membarrier - issue memory barriers on a set of threads
+ * @cmd: Takes command values defined in enum membarrier_cmd.
+ * @flags: Currently needs to be 0. For future extensions.
+ *
+ * If this system call is not implemented, -ENOSYS is returned. If the
+ * command specified does not exist, or if the command argument is invalid,
+ * this system call returns -EINVAL. For a given command, with flags argument
+ * set to 0, this system call is guaranteed to always return the same value
+ * until reboot.
+ *
+ * All memory accesses performed in program order from each targeted thread
+ * is guaranteed to be ordered with respect to sys_membarrier(). If we use
+ * the semantic "barrier()" to represent a compiler barrier forcing memory
+ * accesses to be performed in program order across the barrier, and
+ * smp_mb() to represent explicit memory barriers forcing full memory
+ * ordering across the barrier, we have the following ordering table for
+ * each pair of barrier(), sys_membarrier() and smp_mb():
+ *
+ * The pair ordering is detailed as (O: ordered, X: not ordered):
+ *
+ * barrier() smp_mb() sys_membarrier()
+ * barrier() X X O
+ * smp_mb() X O O
+ * sys_membarrier() O O O
+ */
+SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
+{
+ if (unlikely(flags))
+ return -EINVAL;
+ switch (cmd) {
+ case MEMBARRIER_CMD_QUERY:
+ return MEMBARRIER_CMD_BITMASK;
+ case MEMBARRIER_CMD_SHARED:
+ if (num_online_cpus() > 1)
+ synchronize_sched();
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 7995ef5..eb4fde0 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -243,3 +243,6 @@ cond_syscall(sys_bpf);
/* execveat */
cond_syscall(sys_execveat);
+
+/* membarrier */
+cond_syscall(sys_membarrier);
--
2.1.4
^ permalink raw reply related
* [PATCH 2/3] selftests: add membarrier syscall test
From: Mathieu Desnoyers @ 2015-07-10 20:58 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-api, Pranith Kumar, Michael Ellerman,
Mathieu Desnoyers
In-Reply-To: <1436561912-24365-1-git-send-email-mathieu.desnoyers@efficios.com>
From: Pranith Kumar <bobby.prani@gmail.com>
This patch adds a self test for the membarrier system call.
CC: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/membarrier/.gitignore | 1 +
tools/testing/selftests/membarrier/Makefile | 11 ++++
.../testing/selftests/membarrier/membarrier_test.c | 71 ++++++++++++++++++++++
4 files changed, 84 insertions(+)
create mode 100644 tools/testing/selftests/membarrier/.gitignore
create mode 100644 tools/testing/selftests/membarrier/Makefile
create mode 100644 tools/testing/selftests/membarrier/membarrier_test.c
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 24ae9e8..df577a4 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -6,6 +6,7 @@ TARGETS += firmware
TARGETS += ftrace
TARGETS += futex
TARGETS += kcmp
+TARGETS += membarrier
TARGETS += memfd
TARGETS += memory-hotplug
TARGETS += mount
diff --git a/tools/testing/selftests/membarrier/.gitignore b/tools/testing/selftests/membarrier/.gitignore
new file mode 100644
index 0000000..020c44f4
--- /dev/null
+++ b/tools/testing/selftests/membarrier/.gitignore
@@ -0,0 +1 @@
+membarrier_test
diff --git a/tools/testing/selftests/membarrier/Makefile b/tools/testing/selftests/membarrier/Makefile
new file mode 100644
index 0000000..877a503
--- /dev/null
+++ b/tools/testing/selftests/membarrier/Makefile
@@ -0,0 +1,11 @@
+CFLAGS += -g -I../../../../usr/include/
+
+all:
+ $(CC) $(CFLAGS) membarrier_test.c -o membarrier_test
+
+TEST_PROGS := membarrier_test
+
+include ../lib.mk
+
+clean:
+ $(RM) membarrier_test
diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c
new file mode 100644
index 0000000..3c9f217
--- /dev/null
+++ b/tools/testing/selftests/membarrier/membarrier_test.c
@@ -0,0 +1,71 @@
+#define _GNU_SOURCE
+#define __EXPORTED_HEADERS__
+
+#include <linux/membarrier.h>
+#include <asm-generic/unistd.h>
+#include <sys/syscall.h>
+#include <stdio.h>
+#include <errno.h>
+#include <string.h>
+
+#include "../kselftest.h"
+
+static int sys_membarrier(int cmd, int flags)
+{
+ return syscall(__NR_membarrier, cmd, flags);
+}
+
+static void test_membarrier_fail(void)
+{
+ int cmd = -1, flags = 0;
+
+ if (sys_membarrier(cmd, flags) != -1) {
+ printf("membarrier: Should fail but passed\n");
+ ksft_exit_fail();
+ }
+}
+
+static void test_membarrier_success(void)
+{
+ int flags = 0;
+
+ if (sys_membarrier(MEMBARRIER_CMD_SHARED, flags) != 0) {
+ printf("membarrier: Executing MEMBARRIER failed, %s\n",
+ strerror(errno));
+ ksft_exit_fail();
+ }
+
+ printf("membarrier: MEMBARRIER_CMD_SHARED success\n");
+}
+
+static void test_membarrier(void)
+{
+ test_membarrier_fail();
+ test_membarrier_success();
+}
+
+static int test_membarrier_exists(void)
+{
+ int flags = 0;
+
+ if (sys_membarrier(MEMBARRIER_CMD_QUERY, flags))
+ return 0;
+
+ return 1;
+}
+
+int main(int argc, char **argv)
+{
+ printf("membarrier: MEMBARRIER_CMD_QUERY ");
+ if (test_membarrier_exists()) {
+ printf("syscall implemented\n");
+ test_membarrier();
+ } else {
+ printf("syscall not implemented!\n");
+ return ksft_exit_fail();
+ }
+
+ printf("membarrier: tests done!\n");
+
+ return ksft_exit_pass();
+}
--
2.1.4
^ 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