* [PATCH v7 01/29] docs: fpga: add a document for FPGA Device Feature List (DFL) Framework Overview
From: Wu Hao @ 2018-06-30 0:53 UTC (permalink / raw)
To: atull, mdf, linux-fpga, linux-kernel
Cc: linux-api, luwei.kang, yi.z.zhang, hao.wu, Enno Luebbers,
Xiao Guangrong
In-Reply-To: <1530320016-24712-1-git-send-email-hao.wu@intel.com>
Add a document for FPGA Device Feature List (DFL) Framework Overview.
Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
---
v2: added FME fpga-mgr/bridge/region platform driver to driver organization.
updated open discussion per current implementation.
fixed some typos.
v3: use FPGA base region as container device instead of fpga-dev class.
split common enumeration code from pcie driver to functions exposed by
device feature list framework.
update FME performance reporting which supports both integrated (iperf/)
and discrete (dperf/) FPGA solutions.
v4: rename this doc to Device Feature List (DFL) Framework Overview (dfl.txt)
add Device Feature List introduction and re-organize the content.
add description for port reset, bitstream_id/metadata and etc.
v5: remove introduction of the APIs/features which aren't covered in this patchset.
replace "blue/green bitstream" terminology with "static region" and "PR bitstream".
add a "DFL_" prefix to IOCTL APIs introduced by DFL framework.
s/FPGA Bus Device Module/FPGA DFL Device Module/
fix typos, improve descriptions per comments from Alan Tull against v4.
v6: add Acked-by from Alan.
v7: fix typoes
---
Documentation/fpga/dfl.txt | 285 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 285 insertions(+)
create mode 100644 Documentation/fpga/dfl.txt
diff --git a/Documentation/fpga/dfl.txt b/Documentation/fpga/dfl.txt
new file mode 100644
index 0000000..6df4621
--- /dev/null
+++ b/Documentation/fpga/dfl.txt
@@ -0,0 +1,285 @@
+===============================================================================
+ FPGA Device Feature List (DFL) Framework Overview
+-------------------------------------------------------------------------------
+ Enno Luebbers <enno.luebbers@intel.com>
+ Xiao Guangrong <guangrong.xiao@linux.intel.com>
+ Wu Hao <hao.wu@intel.com>
+
+The Device Feature List (DFL) FPGA framework (and drivers according to this
+this framework) hides the very details of low layer hardwares and provides
+unified interfaces to userspace. Applications could use these interfaces to
+configure, enumerate, open and access FPGA accelerators on platforms which
+implement the DFL in the device memory. Besides this, the DFL framework
+enables system level management functions such as FPGA reconfiguration.
+
+
+Device Feature List (DFL) Overview
+==================================
+Device Feature List (DFL) defines a linked list of feature headers within the
+device MMIO space to provide an extensible way of adding features. Software can
+walk through these predefined data structures to enumerate FPGA features:
+FPGA Interface Unit (FIU), Accelerated Function Unit (AFU) and Private Features,
+as illustrated below:
+
+ Header Header Header Header
+ +----------+ +-->+----------+ +-->+----------+ +-->+----------+
+ | Type | | | Type | | | Type | | | Type |
+ | FIU | | | Private | | | Private | | | Private |
+ +----------+ | | Feature | | | Feature | | | Feature |
+ | Next_DFH |--+ +----------+ | +----------+ | +----------+
+ +----------+ | Next_DFH |--+ | Next_DFH |--+ | Next_DFH |--> NULL
+ | ID | +----------+ +----------+ +----------+
+ +----------+ | ID | | ID | | ID |
+ | Next_AFU |--+ +----------+ +----------+ +----------+
+ +----------+ | | Feature | | Feature | | Feature |
+ | Header | | | Register | | Register | | Register |
+ | Register | | | Set | | Set | | Set |
+ | Set | | +----------+ +----------+ +----------+
+ +----------+ | Header
+ +-->+----------+
+ | Type |
+ | AFU |
+ +----------+
+ | Next_DFH |--> NULL
+ +----------+
+ | GUID |
+ +----------+
+ | Header |
+ | Register |
+ | Set |
+ +----------+
+
+FPGA Interface Unit (FIU) represents a standalone functional unit for the
+interface to FPGA, e.g. the FPGA Management Engine (FME) and Port (more
+descriptions on FME and Port in later sections).
+
+Accelerated Function Unit (AFU) represents a FPGA programmable region and
+always connects to a FIU (e.g. a Port) as its child as illustrated above.
+
+Private Features represent sub features of the FIU and AFU. They could be
+various function blocks with different IDs, but all private features which
+belong to the same FIU or AFU, must be linked to one list via the Next Device
+Feature Header (Next_DFH) pointer.
+
+Each FIU, AFU and Private Feature could implement its own functional registers.
+The functional register set for FIU and AFU, is named as Header Register Set,
+e.g. FME Header Register Set, and the one for Private Feature, is named as
+Feature Register Set, e.g. FME Partial Reconfiguration Feature Register Set.
+
+This Device Feature List provides a way of linking features together, it's
+convenient for software to locate each feature by walking through this list,
+and can be implemented in register regions of any FPGA device.
+
+
+FIU - FME (FPGA Management Engine)
+==================================
+The FPGA Management Engine performs reconfiguration and other infrastructure
+functions. Each FPGA device only has one FME.
+
+User-space applications can acquire exclusive access to the FME using open(),
+and release it using close().
+
+The following functions are exposed through ioctls:
+
+ Get driver API version (DFL_FPGA_GET_API_VERSION)
+ Check for extensions (DFL_FPGA_CHECK_EXTENSION)
+ Program bitstream (DFL_FPGA_FME_PORT_PR)
+
+More functions are exposed through sysfs
+(/sys/class/fpga_region/regionX/dfl-fme.n/):
+
+ Read bitstream ID (bitstream_id)
+ bitstream_id indicates version of the static FPGA region.
+
+ Read bitstream metadata (bitstream_metadata)
+ bitstream_metadata includes detailed information of static FPGA region,
+ e.g. synthesis date and seed.
+
+ Read number of ports (ports_num)
+ one FPGA device may have more than one port, this sysfs interface indicates
+ how many ports the FPGA device has.
+
+
+FIU - PORT
+==========
+A port represents the interface between the static FPGA fabric and a partially
+reconfigurable region containing an AFU. It controls the communication from SW
+to the accelerator and exposes features such as reset and debug. Each FPGA
+device may have more than one port, but always one AFU per port.
+
+
+AFU
+===
+An AFU is attached to a port FIU and exposes a fixed length MMIO region to be
+used for accelerator-specific control registers.
+
+User-space applications can acquire exclusive access to an AFU attached to a
+port by using open() on the port device node and release it using close().
+
+The following functions are exposed through ioctls:
+
+ Get driver API version (DFL_FPGA_GET_API_VERSION)
+ Check for extensions (DFL_FPGA_CHECK_EXTENSION)
+ Get port info (DFL_FPGA_PORT_GET_INFO)
+ Get MMIO region info (DFL_FPGA_PORT_GET_REGION_INFO)
+ Map DMA buffer (DFL_FPGA_PORT_DMA_MAP)
+ Unmap DMA buffer (DFL_FPGA_PORT_DMA_UNMAP)
+ Reset AFU (*DFL_FPGA_PORT_RESET)
+
+*DFL_FPGA_PORT_RESET: reset the FPGA Port and its AFU. Userspace can do Port
+reset at any time, e.g. during DMA or Partial Reconfiguration. But it should
+never cause any system level issue, only functional failure (e.g. DMA or PR
+operation failure) and be recoverable from the failure.
+
+User-space applications can also mmap() accelerator MMIO regions.
+
+More functions are exposed through sysfs:
+(/sys/class/fpga_region/<regionX>/<dfl-port.m>/):
+
+ Read Accelerator GUID (afu_id)
+ afu_id indicates which PR bitstream is programmed to this AFU.
+
+
+DFL Framework Overview
+======================
+
+ +----------+ +--------+ +--------+ +--------+
+ | FME | | AFU | | AFU | | AFU |
+ | Module | | Module | | Module | | Module |
+ +----------+ +--------+ +--------+ +--------+
+ +-----------------------+
+ | FPGA Container Device | Device Feature List
+ | (FPGA Base Region) | Framework
+ +-----------------------+
+--------------------------------------------------------------------
+ +----------------------------+
+ | FPGA DFL Device Module |
+ | (e.g. PCIE/Platform Device)|
+ +----------------------------+
+ +------------------------+
+ | FPGA Hardware Device |
+ +------------------------+
+
+DFL framework in kernel provides common interfaces to create container device
+(FPGA base region), discover feature devices and their private features from the
+given Device Feature Lists and create platform devices for feature devices
+(e.g. FME, Port and AFU) with related resources under the container device. It
+also abstracts operations for the private features and exposes common ops to
+feature device drivers.
+
+The FPGA DFL Device could be different hardwares, e.g. PCIe device, platform
+device and etc. Its driver module is always loaded first once the device is
+created by the system. This driver plays an infrastructural role in the
+driver architecture. It locates the DFLs in the device memory, handles them
+and related resources to common interfaces from DFL framework for enumeration.
+(Please refer to drivers/fpga/dfl.c for detailed enumeration APIs).
+
+The FPGA Management Engine (FME) driver is a platform driver which is loaded
+automatically after FME platform device creation from the DFL device module. It
+provides the key features for FPGA management, including:
+
+ a) Expose static FPGA region information, e.g. version and metadata.
+ Users can read related information via sysfs interfaces exposed
+ by FME driver.
+
+ b) Partial Reconfiguration. The FME driver creates FPGA manager, FPGA
+ bridges and FPGA regions during PR sub feature initialization. Once
+ it receives a DFL_FPGA_FME_PORT_PR ioctl from user, it invokes the
+ common interface function from FPGA Region to complete the partial
+ reconfiguration of the PR bitstream to the given port.
+
+Similar to the FME driver, the FPGA Accelerated Function Unit (AFU) driver is
+probed once the AFU platform device is created. The main function of this module
+is to provide an interface for userspace applications to access the individual
+accelerators, including basic reset control on port, AFU MMIO region export, dma
+buffer mapping service functions.
+
+After feature platform devices creation, matched platform drivers will be loaded
+automatically to handle different functionalities. Please refer to next sections
+for detailed information on functional units which have been already implemented
+under this DFL framework.
+
+
+Partial Reconfiguration
+=======================
+As mentioned above, accelerators can be reconfigured through partial
+reconfiguration of a PR bitstream file. The PR bitstream file must have been
+generated for the exact static FPGA region and targeted reconfigurable region
+(port) of the FPGA, otherwise, the reconfiguration operation will fail and
+possibly cause system instability. This compatibility can be checked by
+comparing the compatibility ID noted in the header of PR bitstream file against
+the compat_id exposed by the target FPGA region. This check is usually done by
+userspace before calling the reconfiguration IOCTL.
+
+
+Device enumeration
+==================
+This section introduces how applications enumerate the fpga device from
+the sysfs hierarchy under /sys/class/fpga_region.
+
+In the example below, two DFL based FPGA devices are installed in the host. Each
+fpga device has one FME and two ports (AFUs).
+
+FPGA regions are created under /sys/class/fpga_region/
+
+ /sys/class/fpga_region/region0
+ /sys/class/fpga_region/region1
+ /sys/class/fpga_region/region2
+ ...
+
+Application needs to search each regionX folder, if feature device is found,
+(e.g. "dfl-port.n" or "dfl-fme.m" is found), then it's the base
+fpga region which represents the FPGA device.
+
+Each base region has one FME and two ports (AFUs) as child devices:
+
+ /sys/class/fpga_region/region0/dfl-fme.0
+ /sys/class/fpga_region/region0/dfl-port.0
+ /sys/class/fpga_region/region0/dfl-port.1
+ ...
+
+ /sys/class/fpga_region/region3/dfl-fme.1
+ /sys/class/fpga_region/region3/dfl-port.2
+ /sys/class/fpga_region/region3/dfl-port.3
+ ...
+
+In general, the FME/AFU sysfs interfaces are named as follows:
+
+ /sys/class/fpga_region/<regionX>/<dfl-fme.n>/
+ /sys/class/fpga_region/<regionX>/<dfl-port.m>/
+
+with 'n' consecutively numbering all FMEs and 'm' consecutively numbering all
+ports.
+
+The device nodes used for ioctl() or mmap() can be referenced through:
+
+ /sys/class/fpga_region/<regionX>/<dfl-fme.n>/dev
+ /sys/class/fpga_region/<regionX>/<dfl-port.n>/dev
+
+
+Add new FIUs support
+====================
+It's possible that developers made some new function blocks (FIUs) under this
+DFL framework, then new platform device driver needs to be developed for the
+new feature dev (FIU) following the same way as existing feature dev drivers
+(e.g. FME and Port/AFU platform device driver). Besides that, it requires
+modification on DFL framework enumeration code too, for new FIU type detection
+and related platform devices creation.
+
+
+Add new private features support
+================================
+In some cases, we may need to add some new private features to existing FIUs
+(e.g. FME or Port). Developers don't need to touch enumeration code in DFL
+framework, as each private feature will be parsed automatically and related
+mmio resources can be found under FIU platform device created by DFL framework.
+Developer only needs to provide a sub feature driver with matched feature id.
+FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
+could be a reference.
+
+
+Open discussion
+===============
+FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
+to user now. In the future, if unified user interfaces for reconfiguration are
+added, FME driver should switch to them from ioctl interface.
--
1.8.3.1
^ permalink raw reply related
* [PATCH v7 00/29] FPGA Device Feature List (DFL) Device Drivers
From: Wu Hao @ 2018-06-30 0:53 UTC (permalink / raw)
To: atull, mdf, linux-fpga, linux-kernel
Cc: linux-api, luwei.kang, yi.z.zhang, hao.wu
Hi All,
Here is v7 patch-series adding drivers for FPGA DFL devices.
This patch series provides a common framework to support FPGA Device
Feature List (DFL) and also feature dev drivers under this DFL framework
to provide interfaces for userspace applications to configure, enumerate,
open and access FPGA accelerators on DFL based FPGA device and enables
system level management functions such as FPGA partial reconfiguration,
power management and virtualization.
This patch series only adds the basic functions for FPGA accelerators and
partial reconfiguration. Patches for more functions, e.g. power management
and virtualization, will be submitted after this series gets reviewed.
Note this patch series is only verified on DFL based Intel(R) FPGA PCIe
devices (e.g. Intel Server Platform with In-package FPGA and Intel FPGA
PCIe Acceleration Cards).
Patch 1: add a document for FPGA DFL framework driver overview, including
Device Feature List (DFL) introduction, the HW architecture, driver
organization, device enumeration and opens.
Patch 2: add region_id for fpga_image_info data structure, which allows
driver to pass region id information to fpga-mgr for FPGA reconfiguration
function. (Used by Patch 17)
Patch 3: add a 'status' sysfs interface to fpga-mgr class, it reflects
the status of the fpga-mgr including reconfiguration errors. (Used by
Patch 18)
Patch 4-5: add compat_id support in fpga manager and region, this compat
id is used for compatibility check before further actions (e.g. partial
reconfiguration). (Used by Patch 19 and 22)
Patch 6-11: add FPGA device feature list support, it provides common
enumeration interfaces which creates container device (FPGA base region)
and all feature devices by walking through all the 'Device Feature Lists'
provided low level drivers. A global list is added to DFL framework to
manage port ops from different ports.
Patch 12-13: implement FPGA PCIe device driver. It locates all 'Device
Feature Lists' in PCIe device memory and invokes common interfaces from
above device feature list framework to finish the enumeration.
Patch 14-17: implement FPGA Management Engine (FME) driver. It's a
platform driver matching with the FME platform device created by above
device feature list framework during enumeration. Sysfs and device file
ioctls are exposed as user interfaces to allow partial reconfiguration
to Accelerated Function Units (AFUs) from user space applications.
Patch 18-22: implement FPGA manager/bridge/region platform drivers for
Intel FPGA Management Engine (FME). These platform drivers match with
platform devices created by above FME driver, they use the generic
fpga-mgr/bridge/region class infrastructure to implement FPGA partial
reconfiguration function.
Patch 23-28: implement FPGA Accelerated Function Unit (AFU) driver.
It's a platform driver matching with AFU platform device created by above
device feature list framework during enumeration. It provides user
interfaces to expose the AFU MMIO region, map/unmap dma buffer and
control the port which AFU connects to.
Patch 29: add a entry in MAINTAINERS for this FPGA DFL drivers patchset.
Changes from v6:
- Improve Kconfig description, fix typos and other comments.
- Update target kernelversion in sysfs doc.
- Fix issues reported by kbuild.
- Simplify pcie driver by using pcim_xxx functions.
Changes from v5:
- Improve functions/APIs naming per suggestion from Alan Tull.
- Improve DFL framework code and comments to simplify the work for adding
a new feature device support.
- Correct the time in copyright and fix other comments from Alan Tull.
- Add a entry in MAINTAINERS for this FPGA DFL drivers patchset.
Changes from v4:
- Update the dfl.txt documentation, remove descriptions for the APIs and
features which are not implemented in this patch series.
- Add DFL_ / dfl_ prefix for APIs and data structure, to avoid directly
using fpga_xxx as definition.
- Use "static region" and "PR bistream" instead of "blue bitstream" and
"green bistream" in description to avoid misunderstanding.
- Fix building issues caused by BIT() on 64bit register definition and
missing correct header file for readq and writeq.
- Remove port specific code in DFL framework and introduce port ops
support to resolve the dependency issue between FME driver module and
Port driver module. (more details in Patch 8).
- Add compat id to fpga manager, as in case some hardware implements the
compat id in fpga manager's register, not register belongs to fpga region
and it's value is shared by all related fpga regions.
- Pass mapped ioaddr to fme manager platform device from dfl-fme-pr via
pdata.
- Fix other comments from Alan and Moritze, including description
improvement, coding style issue and etc.
Changes from v3:
- Fix SPDX license issue.
- Rename documentation to dfl.txt, add introduction for Device Feature List
(DFL) and re-organize the content.
- Rename to FPGA Device Feature List (DFL) drivers from Intel FPGA device
drivers for better reuse purposes. Unified driver and files to dfl-*.*
- Remove static feature_info table from common enumeration code and switch
to use feature id for sub feature driver matching.
- Remove next_afu register checking for AFU from common enumeration code.
- Remove interface_id sysfs for dfl-fme-mgr and use per fpga-region
compat_id instead. (new patch 13, 15, 19).
- Add more comments for driver data structures and functions.
- Fix typos, issues in debug message/commit message and other places.
Changes from v2:
- Split common enumeration code from pcie driver to a separated module
which for device feature list support.
- Drop fpga-dev class and switch to use fpga base region as container.
- Update the intel-fpga.txt documentation for new driver organization.
- Rename feature device drivers for future code reuse.
- Rebase code due to fpga APIs changes
- replace bitfields with marco and shift.
- fix typos, checkpatch issue and other comments.
Changes from v1:
- Use GPLv2 license instead of Dual BSD/GPL.
- Move the code to drivers/fpga folder.
- Update the intel-fpga.txt documentation for new driver organization.
- Add documentation for new sysfs interfaces.
- Switch to use common fpga-region interface for partial reconfiguration
(PR) function in FME. It creates fpga-region/fpga-mgr/fpga-bridge
platform devices and leave the implementation to their platform drivers.
- Add platform drivers for FME fpga-mgr/bridge/region platform devices.
- Fix kbuild warnings, typos and other comments.
Kang Luwei (3):
fpga: dfl: add FPGA Management Engine driver basic framework
fpga: dfl: fme: add header sub feature support
fpga: dfl: fme: add partial reconfiguration sub feature support
Wu Hao (23):
docs: fpga: add a document for FPGA Device Feature List (DFL)
Framework Overview
fpga: mgr: add region_id to fpga_image_info
fpga: mgr: add status for fpga-manager
fpga: mgr: add compat_id support
fpga: region: add compat_id support
fpga: add device feature list support
fpga: dfl: add chardev support for feature devices
fpga: dfl: add dfl_fpga_cdev_find_port
fpga: dfl: add dfl_fpga_port_ops support.
fpga: dfl: add dfl_fpga_check_port_id function.
fpga: dfl-pci: add enumeration for feature devices
fpga: dfl: fme: add DFL_FPGA_GET_API_VERSION/CHECK_EXTENSION ioctls
support
fpga: dfl: add fpga manager platform driver for FME
fpga: dfl: fme-mgr: add compat_id support
fpga: dfl: add fpga bridge platform driver for FME
fpga: dfl: add fpga region platform driver for FME
fpga: dfl: fme-region: add support for compat_id
fpga: dfl: add FPGA Accelerated Function Unit driver basic framework
fpga: dfl: afu: add port ops support
fpga: dfl: afu: add header sub feature support
fpga: dfl: afu: add DFL_FPGA_GET_API_VERSION/CHECK_EXTENSION ioctls
support
fpga: dfl: afu: add DFL_FPGA_PORT_DMA_MAP/UNMAP ioctls support
MAINTAINERS: add entry for FPGA DFL drivers
Xiao Guangrong (2):
fpga: dfl: add feature device infrastructure
fpga: dfl: afu: add afu sub feature support
Zhang Yi (1):
fpga: add FPGA DFL PCIe device driver
Documentation/ABI/testing/sysfs-class-fpga-manager | 24 +
Documentation/ABI/testing/sysfs-class-fpga-region | 9 +
Documentation/ABI/testing/sysfs-platform-dfl-fme | 23 +
Documentation/ABI/testing/sysfs-platform-dfl-port | 16 +
Documentation/fpga/dfl.txt | 285 ++++++
Documentation/ioctl/ioctl-number.txt | 1 +
MAINTAINERS | 8 +
drivers/fpga/Kconfig | 68 ++
drivers/fpga/Makefile | 14 +
drivers/fpga/dfl-afu-dma-region.c | 463 +++++++++
drivers/fpga/dfl-afu-main.c | 636 ++++++++++++
drivers/fpga/dfl-afu-region.c | 166 ++++
drivers/fpga/dfl-afu.h | 100 ++
drivers/fpga/dfl-fme-br.c | 114 +++
drivers/fpga/dfl-fme-main.c | 279 ++++++
drivers/fpga/dfl-fme-mgr.c | 349 +++++++
drivers/fpga/dfl-fme-pr.c | 479 +++++++++
drivers/fpga/dfl-fme-pr.h | 84 ++
drivers/fpga/dfl-fme-region.c | 89 ++
drivers/fpga/dfl-fme.h | 38 +
drivers/fpga/dfl-pci.c | 243 +++++
drivers/fpga/dfl.c | 1044 ++++++++++++++++++++
drivers/fpga/dfl.h | 410 ++++++++
drivers/fpga/fpga-mgr.c | 28 +
drivers/fpga/fpga-region.c | 22 +
include/linux/fpga/fpga-mgr.h | 24 +
include/linux/fpga/fpga-region.h | 2 +
include/uapi/linux/fpga-dfl.h | 179 ++++
28 files changed, 5197 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-region
create mode 100644 Documentation/ABI/testing/sysfs-platform-dfl-fme
create mode 100644 Documentation/ABI/testing/sysfs-platform-dfl-port
create mode 100644 Documentation/fpga/dfl.txt
create mode 100644 drivers/fpga/dfl-afu-dma-region.c
create mode 100644 drivers/fpga/dfl-afu-main.c
create mode 100644 drivers/fpga/dfl-afu-region.c
create mode 100644 drivers/fpga/dfl-afu.h
create mode 100644 drivers/fpga/dfl-fme-br.c
create mode 100644 drivers/fpga/dfl-fme-main.c
create mode 100644 drivers/fpga/dfl-fme-mgr.c
create mode 100644 drivers/fpga/dfl-fme-pr.c
create mode 100644 drivers/fpga/dfl-fme-pr.h
create mode 100644 drivers/fpga/dfl-fme-region.c
create mode 100644 drivers/fpga/dfl-fme.h
create mode 100644 drivers/fpga/dfl-pci.c
create mode 100644 drivers/fpga/dfl.c
create mode 100644 drivers/fpga/dfl.h
create mode 100644 include/uapi/linux/fpga-dfl.h
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH v2 5/7] mm: rename and change semantics of nr_indirectly_reclaimable_bytes
From: Roman Gushchin @ 2018-06-29 21:12 UTC (permalink / raw)
To: Vlastimil Babka
Cc: kbuild test robot, kbuild-all, Andrew Morton, linux-mm,
linux-kernel, Michal Hocko, Johannes Weiner, linux-api,
Christoph Lameter, David Rientjes, Mel Gorman, Matthew Wilcox,
Vijayanand Jitta, Laura Abbott, Sumit Semwal
In-Reply-To: <38c6a6e1-c5e0-fd7d-4baf-1f0f09be5094@suse.cz>
On Fri, Jun 29, 2018 at 05:37:02PM +0200, Vlastimil Babka wrote:
> On 06/20/2018 01:23 PM, kbuild test robot wrote:
> > Hi Vlastimil,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on mmotm/master]
> > [also build test ERROR on v4.18-rc1 next-20180619]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url: https://github.com/0day-ci/linux/commits/Vlastimil-Babka/kmalloc-reclaimable-caches/20180618-172912
> > base: git://git.cmpxchg.org/linux-mmotm.git master
> > config: x86_64-allmodconfig (attached as .config)
> > compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> > reproduce:
> > # save the attached .config to linux build tree
> > make ARCH=x86_64
> >
> > All errors (new ones prefixed by >>):
> >
> > drivers/staging//android/ion/ion_page_pool.c: In function 'ion_page_pool_remove':
> >>> drivers/staging//android/ion/ion_page_pool.c:56:40: error: 'NR_INDIRECTLY_RECLAIMABLE_BYTES' undeclared (first use in this function)
> > mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/staging//android/ion/ion_page_pool.c:56:40: note: each undeclared identifier is reported only once for each function it appears in
> >
> > vim +/NR_INDIRECTLY_RECLAIMABLE_BYTES +56 drivers/staging//android/ion/ion_page_pool.c
>
> Looks like I missed a hunk, updated patch below.
>
> ----8<----
> From a0053c64c72d7e094252d0d7462de8569d87c543 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Tue, 22 May 2018 16:10:10 +0200
> Subject: [PATCH v3 5/7] mm: rename and change semantics of
> nr_indirectly_reclaimable_bytes
>
> The vmstat counter NR_INDIRECTLY_RECLAIMABLE_BYTES was introduced by commit
> eb59254608bc ("mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES") with the goal of
> accounting objects that can be reclaimed, but cannot be allocated via a
> SLAB_RECLAIM_ACCOUNT cache. This is now possible via kmalloc() with
> __GFP_RECLAIMABLE flag, and the dcache external names user is converted.
>
> The counter is however still useful for accounting direct page allocations
> (i.e. not slab) with a shrinker, such as the ION page pool. So keep it, and:
Btw, it looks like I've another example of usefulness of this counter:
dynamic per-cpu data.
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Andy Lutomirski @ 2018-06-29 20:39 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Linus Torvalds, Andy Lutomirski, Thomas Gleixner, linux-kernel,
linux-api, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas
In-Reply-To: <184287091.10022.1530301738384.JavaMail.zimbra@efficios.com>
On Fri, Jun 29, 2018 at 12:48 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> There are two aspects I'm concerned about here:
>
> 1) security: we don't want 32-bit user-space to feed a 64-bit value over 4GB
> as abort_ip that may end up causing OOPSes on architectures that would
> lack proper validation of those values on return to userspace.
I'm not too worried about this. As long as you're doing it from
signal-delivery context (which you are AFAICT) you're fine.
But I re-read the code and I think I have a really straightforward
solution. Two choices:
(1) Change instruction_pointer_set() to return an error code if the
address passed in is garbage in a way that could cause unexpected
behavior (like >=2^32 on x86_64 if regs->cs is 32-bit). It has very
very few callers.
(2) Add instruction_pointer_validate() to go along with
instruction_pointer_set().
That should be enough to solve the problem, right?
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-29 19:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Andy Lutomirski, Thomas Gleixner, linux-kernel,
linux-api, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas,
Will Deacon <will.deaco>
In-Reply-To: <CA+55aFz8KyVqFsVnp-6g+thz7vmm5nK_FSdRTPSks7gsNVNRdQ@mail.gmail.com>
----- On Jun 29, 2018, at 1:03 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Fri, Jun 29, 2018 at 9:07 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> This code is not invoked from syscalls, but rather on return from
>> interrupt/trap after a preemption.
>
> But when we register the rseq, we could easily check that the top bits
> of the IP is clear, no?
When a thread registers rseq, it registers a pointer to a user-space
address where a struct rseq is located.
That struct rseq is typically in a TLS area. It contains a pointer
to the current "struct rseq_cs": the content of rseq_cs describes the
current rseq critical section.
So when we register rseq, the rseq->rseq_cs pointer value is typically
NULL, because there is no currently active critical section. It's after
return from sys_rseq registration that user-space eventually sets the
pointer to a non-NULL value when it enters a critical section.
So at rseq registration, there is no point in validating the value of
the rseq_cs pointer, nor of any fields in the struct rseq_cs that would
be currently pointed to by that rseq_cs pointer, because those all change
after registration.
> Sure, user space can change it after the fact, but at that point it's
> literally "user space is being intentionally stupid".
User-space can be either stupid, or really clever and trying to attack
the kernel.
> The real worry is that 32-bit compat code never initializes those bits
> at all, no?
There are two aspects I'm concerned about here:
1) security: we don't want 32-bit user-space to feed a 64-bit value over 4GB
as abort_ip that may end up causing OOPSes on architectures that would
lack proper validation of those values on return to userspace.
2) behavior consistency of 32-bit userspace on both native 32-bit and 32-bit
compat on 64-bit kernel:
- for testing: having repeatable behavior on native and compat deployments
ensures that testing results are the same. This is the difference between
having "undefined behavior" when the upper bits are set or "defined behavior:
the process is terminated with sigsegv",
- for security: if the behavior differs between 32-bit compat and native
32-bit, this leaks information about which specific architecture the kernel
is running on, which facilitates attacks on the kernel.
But perhaps I'm caring too much about those aspects ? Maybe they matter less
than I presume.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Linus Torvalds @ 2018-06-29 17:03 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Andrew Lutomirski, Thomas Gleixner,
Linux Kernel Mailing List, Linux API, Peter Zijlstra,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Andi Kleen, Christoph Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Catalin Marinas
In-Reply-To: <247789350.9741.1530288432573.JavaMail.zimbra@efficios.com>
On Fri, Jun 29, 2018 at 9:07 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> This code is not invoked from syscalls, but rather on return from
> interrupt/trap after a preemption.
But when we register the rseq, we could easily check that the top bits
of the IP is clear, no?
Sure, user space can change it after the fact, but at that point it's
literally "user space is being intentionally stupid".
The real worry is that 32-bit compat code never initializes those bits
at all, no?
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Andy Lutomirski @ 2018-06-29 16:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Mathieu Desnoyers, Andrew Lutomirski, Thomas Gleixner,
linux-kernel, linux-api, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas, Will
In-Reply-To: <CA+55aFw==YnFJn7iGnKMW=RbPT74YHNa0QDF96mEdMPA2oX9SA@mail.gmail.com>
On Fri, Jun 29, 2018 at 8:27 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Fri, Jun 29, 2018, 08:03 Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>>
>> Considering those inconsistencies between architectures (either
>> the task gets killed, or the top bits are silently cleared), I'm
>> very much tempted to be restrictive in the inputs accepted by
>> rseq, and not rely on architectures as providing consistent
>> validation of the return IP.
>>
>> Thoughts ?
>
>
> Then you need to make it a compat system call, since clearly you and Andy
> want the 32-bit case to do something different from the 64-bit case.
I personally would like the compat and non-compat cases to do exactly
the same thing. If abort_ip is the address (as a u64) of a valid
executable instruction and an abort happens, then that instruction
should get executed. If abort_ip does not point to user-executable
memory, then the process should get a signal.
The problem isn't with rseq per se -- it's with the daft way that the
x86 return-to-userspace instructions work. If I apply this patch:
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 3b2490b81918..26e4ba44e87b 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -346,6 +346,8 @@ __visible void do_int80_syscall_32(struct pt_regs *regs)
{
enter_from_user_mode();
local_irq_enable();
+ if (!user_64bit_mode(regs))
+ regs->ip |= (1UL << 32);
do_syscall_32_irqs_on(regs);
}
the kernel *still works*. But this unconditionally uses the IRET
path, and I don't even want to speculate as to what the hell happens
if we exit using SYSRETL, or LRET, or SYSEXITL, or if Intel or AMD
ever gives us a new mode that gets rid of the espfix shite. IOW, I
don't think that the x86 entry code should make a promise that it will
continue ignoring the high bits of regs->ip when
!user_64bit_mode(regs) on a 64-bit kernel.
The problem with rseq as it stands is that this oddity gets
accidentally exposed all the way to userspace. If we're not careful,
it'll be possible for a slightly buggy user program to goof up the
code that generates the data structure that supplies abort_ip such
that the high bits are garbage (0xcccccccc due to padding, for
example), and the kernel will do exactly what the user code requested,
and we'll get regs->ip = 0xcccccccc00000000 | (the actual intended
abort_ip), and we'll pass that crap value all the way to IRET, and
IRET will truncate it back down to the correct value. And then some
CPU will add new behavior or we'll invoke SYSRETL or whatever on some
weird CPU, and the program will crash. And we'll be sad.
I suppose we could handle this in the entry code by coming up with a
way to reject out-of-bounds regs->ip for 32-bit tasks, but that's
going to be a bit messy and will slow down normal code that doesn't
use rseq.
Other than rseq, I don't think that there's any real issue. The only
ways to get regs->ip >= 2^32 in a 32-bit task involve ptrace or manual
fiddling with signal contexts, and I don't expect to ever have any
real software depend on precisely what happens.
^ permalink raw reply related
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-29 16:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Andy Lutomirski, Thomas Gleixner, linux-kernel,
linux-api, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas,
Will Deacon <will.deaco>
In-Reply-To: <CA+55aFwJWxsE_LL=-8WgB7uH_mYeMSu=boWF4N5KdOW673g4hA@mail.gmail.com>
----- On Jun 29, 2018, at 11:54 AM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Fri, Jun 29, 2018 at 8:27 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> You simply can't have it both ways.
>
> Put another way.
>
> This is ok in the native path:
>
> if ((unsigned long) rseq_cs->abort_ip != rseq_cs->abort_ip)
> return -EINVAL;
>
> because it's checking that the value fits in the native register size
> (and it also ends up being a no-op if the native size is the same size
> as abort_ip).
>
> And this is very much ok in a compat syscall:
>
> if (rseq_cs->abort_ip & ~(unsigned long)-1u)
> return -EINVAL;
>
> because it's checking that the pointer doesn't have (invalid in
> compat) high bits set.
>
> But it is NOT OK to say "the rseq system call doesn't have any compat
> syscall, but we'll do that compat check in the native case, because we
> worry about compat issues".
>
> See what I'm saying? Either you worry about compat issues (and have a
> compat syscall), or you don't.
>
> The whole "let's not do a compat syscall, but then check compat issues
> at run-time in the native system call because compat processes will
> use it" is braindamage.
This code is not invoked from syscalls, but rather on return from
interrupt/trap after a preemption.
So a compat system call does not solve it. Unless we grab the "compat"
state on rseq registration, save it in a rseq_compat flag within the
task struct, and then use it on return from interrupt/trap/syscall.
Otherwise we need to figure out whether we are dealing with a compat
task when interrupt and trap context return to userspace. We had
is_compat_task() for that before, but now it has vanished from x86.
We could use user_64bit_mode(struct pt_regs *) on x86, but it does not
exist on other architectures.
One possibility is to introduce a new API that calls user_64bit_mode()
on x86, and is_compat_task() on other archs.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Linus Torvalds @ 2018-06-29 15:54 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Andrew Lutomirski, Thomas Gleixner,
Linux Kernel Mailing List, Linux API, Peter Zijlstra,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Andi Kleen, Christoph Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Catalin Marinas
In-Reply-To: <CA+55aFw==YnFJn7iGnKMW=RbPT74YHNa0QDF96mEdMPA2oX9SA@mail.gmail.com>
On Fri, Jun 29, 2018 at 8:27 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> You simply can't have it both ways.
Put another way.
This is ok in the native path:
if ((unsigned long) rseq_cs->abort_ip != rseq_cs->abort_ip)
return -EINVAL;
because it's checking that the value fits in the native register size
(and it also ends up being a no-op if the native size is the same size
as abort_ip).
And this is very much ok in a compat syscall:
if (rseq_cs->abort_ip & ~(unsigned long)-1u)
return -EINVAL;
because it's checking that the pointer doesn't have (invalid in
compat) high bits set.
But it is NOT OK to say "the rseq system call doesn't have any compat
syscall, but we'll do that compat check in the native case, because we
worry about compat issues".
See what I'm saying? Either you worry about compat issues (and have a
compat syscall), or you don't.
The whole "let's not do a compat syscall, but then check compat issues
at run-time in the native system call because compat processes will
use it" is braindamage.
Linus
^ permalink raw reply
* Re: [PATCH v2 5/7] mm: rename and change semantics of nr_indirectly_reclaimable_bytes
From: Vlastimil Babka @ 2018-06-29 15:37 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Andrew Morton, linux-mm, linux-kernel, Roman Gushchin,
Michal Hocko, Johannes Weiner, linux-api, Christoph Lameter,
David Rientjes, Mel Gorman, Matthew Wilcox, Vijayanand Jitta,
Laura Abbott, Sumit Semwal
In-Reply-To: <201806201923.mC5ZpigB%fengguang.wu@intel.com>
On 06/20/2018 01:23 PM, kbuild test robot wrote:
> Hi Vlastimil,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on mmotm/master]
> [also build test ERROR on v4.18-rc1 next-20180619]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Vlastimil-Babka/kmalloc-reclaimable-caches/20180618-172912
> base: git://git.cmpxchg.org/linux-mmotm.git master
> config: x86_64-allmodconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
> drivers/staging//android/ion/ion_page_pool.c: In function 'ion_page_pool_remove':
>>> drivers/staging//android/ion/ion_page_pool.c:56:40: error: 'NR_INDIRECTLY_RECLAIMABLE_BYTES' undeclared (first use in this function)
> mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/staging//android/ion/ion_page_pool.c:56:40: note: each undeclared identifier is reported only once for each function it appears in
>
> vim +/NR_INDIRECTLY_RECLAIMABLE_BYTES +56 drivers/staging//android/ion/ion_page_pool.c
Looks like I missed a hunk, updated patch below.
----8<----
>From a0053c64c72d7e094252d0d7462de8569d87c543 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Tue, 22 May 2018 16:10:10 +0200
Subject: [PATCH v3 5/7] mm: rename and change semantics of
nr_indirectly_reclaimable_bytes
The vmstat counter NR_INDIRECTLY_RECLAIMABLE_BYTES was introduced by commit
eb59254608bc ("mm: introduce NR_INDIRECTLY_RECLAIMABLE_BYTES") with the goal of
accounting objects that can be reclaimed, but cannot be allocated via a
SLAB_RECLAIM_ACCOUNT cache. This is now possible via kmalloc() with
__GFP_RECLAIMABLE flag, and the dcache external names user is converted.
The counter is however still useful for accounting direct page allocations
(i.e. not slab) with a shrinker, such as the ION page pool. So keep it, and:
- change granularity to pages to be more like other counters; sub-page
allocations should be able to use kmalloc
- rename the counter to NR_KERNEL_MISC_RECLAIMABLE
- expose the counter again in vmstat as "nr_kernel_misc_reclaimable"; we can
again remove the check for not printing "hidden" counters
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Vijayanand Jitta <vjitta@codeaurora.org>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
---
drivers/staging/android/ion/ion_page_pool.c | 8 ++++----
include/linux/mmzone.h | 2 +-
mm/page_alloc.c | 19 +++++++------------
mm/util.c | 3 +--
mm/vmstat.c | 6 +-----
5 files changed, 14 insertions(+), 24 deletions(-)
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c
index 9bc56eb48d2a..0d2a95957ee8 100644
--- a/drivers/staging/android/ion/ion_page_pool.c
+++ b/drivers/staging/android/ion/ion_page_pool.c
@@ -33,8 +33,8 @@ static void ion_page_pool_add(struct ion_page_pool *pool, struct page *page)
pool->low_count++;
}
- mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
- (1 << (PAGE_SHIFT + pool->order)));
+ mod_node_page_state(page_pgdat(page), NR_KERNEL_MISC_RECLAIMABLE,
+ 1 << pool->order);
mutex_unlock(&pool->mutex);
}
@@ -53,8 +53,8 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high)
}
list_del(&page->lru);
- mod_node_page_state(page_pgdat(page), NR_INDIRECTLY_RECLAIMABLE_BYTES,
- -(1 << (PAGE_SHIFT + pool->order)));
+ mod_node_page_state(page_pgdat(page), NR_KERNEL_MISC_RECLAIMABLE,
+ -(1 << pool->order));
return page;
}
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 32699b2dc52a..c2f6bc4c9e8a 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -180,7 +180,7 @@ enum node_stat_item {
NR_VMSCAN_IMMEDIATE, /* Prioritise for reclaim when writeback ends */
NR_DIRTIED, /* page dirtyings since bootup */
NR_WRITTEN, /* page writings since bootup */
- NR_INDIRECTLY_RECLAIMABLE_BYTES, /* measured in bytes */
+ NR_KERNEL_MISC_RECLAIMABLE, /* reclaimable non-slab kernel pages */
NR_VM_NODE_STAT_ITEMS
};
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1521100f1e63..8ceb45e11b97 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4704,6 +4704,7 @@ long si_mem_available(void)
unsigned long pagecache;
unsigned long wmark_low = 0;
unsigned long pages[NR_LRU_LISTS];
+ unsigned long reclaimable;
struct zone *zone;
int lru;
@@ -4729,19 +4730,13 @@ long si_mem_available(void)
available += pagecache;
/*
- * Part of the reclaimable slab consists of items that are in use,
- * and cannot be freed. Cap this estimate at the low watermark.
+ * Part of the reclaimable slab and other kernel memory consists of
+ * items that are in use, and cannot be freed. Cap this estimate at the
+ * low watermark.
*/
- available += global_node_page_state(NR_SLAB_RECLAIMABLE) -
- min(global_node_page_state(NR_SLAB_RECLAIMABLE) / 2,
- wmark_low);
-
- /*
- * Part of the kernel memory, which can be released under memory
- * pressure.
- */
- available += global_node_page_state(NR_INDIRECTLY_RECLAIMABLE_BYTES) >>
- PAGE_SHIFT;
+ reclaimable = global_node_page_state(NR_SLAB_RECLAIMABLE) +
+ global_node_page_state(NR_KERNEL_MISC_RECLAIMABLE);
+ available += reclaimable - min(reclaimable / 2, wmark_low);
if (available < 0)
available = 0;
diff --git a/mm/util.c b/mm/util.c
index 3351659200e6..891f0654e7b5 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -675,8 +675,7 @@ int __vm_enough_memory(struct mm_struct *mm, long pages, int cap_sys_admin)
* Part of the kernel memory, which can be released
* under memory pressure.
*/
- free += global_node_page_state(
- NR_INDIRECTLY_RECLAIMABLE_BYTES) >> PAGE_SHIFT;
+ free += global_node_page_state(NR_KERNEL_MISC_RECLAIMABLE);
/*
* Leave reserved pages. The pages are not for anonymous pages.
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 75eda9c2b260..7c677d3a61ec 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1161,7 +1161,7 @@ const char * const vmstat_text[] = {
"nr_vmscan_immediate_reclaim",
"nr_dirtied",
"nr_written",
- "", /* nr_indirectly_reclaimable */
+ "nr_kernel_misc_reclaimable",
/* enum writeback_stat_item counters */
"nr_dirty_threshold",
@@ -1704,10 +1704,6 @@ static int vmstat_show(struct seq_file *m, void *arg)
unsigned long *l = arg;
unsigned long off = l - (unsigned long *)m->private;
- /* Skip hidden vmstat items. */
- if (*vmstat_text[off] == '\0')
- return 0;
-
seq_puts(m, vmstat_text[off]);
seq_put_decimal_ull(m, " ", *l);
seq_putc(m, '\n');
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-29 15:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Andy Lutomirski, Thomas Gleixner, linux-kernel,
linux-api, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas,
Will Deacon <will.deaco>
In-Reply-To: <CA+55aFxRJWjbgLeSQ_swi3F5nF1SYgZnC-R7QsOvZdcuSaWeaw@mail.gmail.com>
----- On Jun 29, 2018, at 10:17 AM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Fri, Jun 29, 2018 at 7:05 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> What I'm worried about is setting regs->ip of a compat 32-bit task to
>> addresses in the range 0x100000000-0xFFFFFFFFFFFFFFFF.
>
> Well, they won't have anything mapped in that range, so it really
> shouldn't matter.
It appears that arm64 simply clears the top bits of regs->ip when
returning to 32-bit compat userspace. So this would be inconsistent
between 32-bit kernel and 64-bit kernel with a 32-bit compat task:
a 32-bit kernel would kill the process, but a 64-bit kernel would
silently clear the top bits.
Considering those inconsistencies between architectures (either
the task gets killed, or the top bits are silently cleared), I'm
very much tempted to be restrictive in the inputs accepted by
rseq, and not rely on architectures as providing consistent
validation of the return IP.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Linus Torvalds @ 2018-06-29 14:17 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Andrew Lutomirski, Thomas Gleixner,
Linux Kernel Mailing List, Linux API, Peter Zijlstra,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Andi Kleen, Christoph Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Catalin Marinas
In-Reply-To: <1706339668.9644.1530281144560.JavaMail.zimbra@efficios.com>
On Fri, Jun 29, 2018 at 7:05 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> What I'm worried about is setting regs->ip of a compat 32-bit task to
> addresses in the range 0x100000000-0xFFFFFFFFFFFFFFFF.
Well, they won't have anything mapped in that range, so it really
shouldn't matter.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-29 14:05 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Andy Lutomirski, Thomas Gleixner, linux-kernel,
linux-api, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, Paul Turner, Andrew Morton, Russell King,
Ingo Molnar, H. Peter Anvin, Andi Kleen, Chris Lameter,
Ben Maurer, rostedt, Josh Triplett, Catalin Marinas,
Will Deacon <will.deaco>
In-Reply-To: <CA+55aFzpD5xF80iiFNd+EMkJnRQdKPumM5p24Sr+LeBt8Gg=wg@mail.gmail.com>
----- On Jun 29, 2018, at 10:02 AM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Thu, Jun 28, 2018 at 6:08 PM Andy Lutomirski <luto@amacapital.net> wrote:
>> > On Jun 28, 2018, at 5:18 PM, Linus Torvalds <torvalds@linux-foundation.org>
>> > wrote:
>> >
>> >
>> > Make it do
>> >
>> > if (rseq_cs->abort_ip != (unsigned long)rseq_cs->abort_ip)
>> > return -EINVAL;
>> >
>> > at abort time.
>>
>> You sure? Because, unless I remember wrong, a 32-bit user program on a 64-bit
>> kernel will actually work at least most of the time even if high bits are set.
>
> Sure.
>
> If you run a 32-bit binary on a 64-bit kernel,. you will have access
> to the 0xc0000000 - 0xffffffff area that you wouldn't have had access
> to if it ran on a 32-bit kernel.
>
> But exactly *because* you have access to that area, those addresses
> are actually valid addresses for the 32-bit case, so they shouldn't be
> considered bad. They can't happen on a native 32-bit kerne, but a
> 32-bit program doesn't even care. If it has user memory mapped in that
> area, it should work.
>
> And if it *doesn't* have user memory mapped in that area, then it will
> fail when the trying to execute the (non-existent) abort sequence.
>
> After all, depending on configuration, a native 32-bit kernel might
> limit user space even more (ie some vendors had a 2G:2G split instead
> of the traditional 3G:1G split.
>
> Was that the case you were thinking of, or was it something else?
What I'm worried about is setting regs->ip of a compat 32-bit task to
addresses in the range 0x100000000-0xFFFFFFFFFFFFFFFF.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Linus Torvalds @ 2018-06-29 14:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andrew Lutomirski, Mathieu Desnoyers, Thomas Gleixner,
Linux Kernel Mailing List, Linux API, Peter Zijlstra,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Andi Kleen, Christoph Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Catalin Marinas <catalin.marinas@
In-Reply-To: <9200ED2A-AE4B-4094-81C9-E92240B4840F@amacapital.net>
On Thu, Jun 28, 2018 at 6:08 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > On Jun 28, 2018, at 5:18 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> >
> > Make it do
> >
> > if (rseq_cs->abort_ip != (unsigned long)rseq_cs->abort_ip)
> > return -EINVAL;
> >
> > at abort time.
>
> You sure? Because, unless I remember wrong, a 32-bit user program on a 64-bit kernel will actually work at least most of the time even if high bits are set.
Sure.
If you run a 32-bit binary on a 64-bit kernel,. you will have access
to the 0xc0000000 - 0xffffffff area that you wouldn't have had access
to if it ran on a 32-bit kernel.
But exactly *because* you have access to that area, those addresses
are actually valid addresses for the 32-bit case, so they shouldn't be
considered bad. They can't happen on a native 32-bit kerne, but a
32-bit program doesn't even care. If it has user memory mapped in that
area, it should work.
And if it *doesn't* have user memory mapped in that area, then it will
fail when the trying to execute the (non-existent) abort sequence.
After all, depending on configuration, a native 32-bit kernel might
limit user space even more (ie some vendors had a 2G:2G split instead
of the traditional 3G:1G split.
Was that the case you were thinking of, or was it something else?
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-29 13:55 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon, Michael
In-Reply-To: <CALCETrUrXk3wRg8SKhWf98v8jK=HwX9XLvP+Ypi+TktZoNx_Jg@mail.gmail.com>
----- On Jun 28, 2018, at 7:29 PM, Andy Lutomirski luto@kernel.org wrote:
> On Thu, Jun 28, 2018 at 2:22 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Thu, Jun 28, 2018 at 1:23 PM Andy Lutomirski <luto@kernel.org> wrote:
>>>
>>> This is okay with me for a fix outside the merge window. Can you do a
>>> followup for the next merge window that fixes it better, though? In
>>> particular, TASK_SIZE is generally garbage. I think a better fix
>>> would be something like adding a new arch-overridable helper like:
>>>
>>> static inline unsigned long current_max_user_addr(void) { return TASK_SIZE; }
>>
>> We already have that. It's called "user_addr_max()".
>
> Nah, that one is more or less equivalent to TASK_SIZE_MAX, except that
> it's different if set_fs() is used.
So which one would be right in this case ? AFAIU we want to ensure we don't
populate regs->ip with a bogus address that would make SYSRET or other return
to userspace instructions explode.
Is that guaranteed by TASK_SIZE or TASK_SIZE_MAX (aliased by user_addr_max()) ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Andy Lutomirski @ 2018-06-29 1:08 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Lutomirski, Mathieu Desnoyers, Thomas Gleixner,
Linux Kernel Mailing List, Linux API, Peter Zijlstra,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Andi Kleen, Christoph Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Catalin Marinas <catalin.marinas@
In-Reply-To: <CA+55aFzxCjkSbfRUeX3W_oXSJ6LMUdRVYB=DR2b3rUNkiixM1A@mail.gmail.com>
> On Jun 28, 2018, at 5:18 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Thu, Jun 28, 2018 at 4:30 PM Andy Lutomirski <luto@kernel.org> wrote:
>>
>> The idea is that, if someone screws up and sticks a number like
>> 0xbaadf00d00045678 into their rseq abort_ip in a 32-bit x86 program
>> (when they actually mean 0x00045678), we want to something consistent.
>
> I think the "something consistent" is perfectly fine with just "it won't work".
>
> Make it do
>
> if (rseq_cs->abort_ip != (unsigned long)rseq_cs->abort_ip)
> return -EINVAL;
>
> at abort time.
You sure? Because, unless I remember wrong, a 32-bit user program on a 64-bit kernel will actually work at least most of the time even if high bits are set. I’m okay with straight-up promising “will always work” or “will never work”, but “sometimes” is bad.
>
> Done.
>
> If it's a 32-bit kernel, the above will reject the thing, and if it's
> a 64-bit kernel, it will be a no-op, but the abort won't work in a
> 32-bit caller.
>
> Problem solved.
>
> Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-29 0:54 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.manpag>
In-Reply-To: <CA+55aFzxCjkSbfRUeX3W_oXSJ6LMUdRVYB=DR2b3rUNkiixM1A@mail.gmail.com>
----- On Jun 28, 2018, at 8:18 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Thu, Jun 28, 2018 at 4:30 PM Andy Lutomirski <luto@kernel.org> wrote:
>>
>> The idea is that, if someone screws up and sticks a number like
>> 0xbaadf00d00045678 into their rseq abort_ip in a 32-bit x86 program
>> (when they actually mean 0x00045678), we want to something consistent.
>
> I think the "something consistent" is perfectly fine with just "it won't work".
>
> Make it do
>
> if (rseq_cs->abort_ip != (unsigned long)rseq_cs->abort_ip)
> return -EINVAL;
>
> at abort time.
>
> Done.
>
> If it's a 32-bit kernel, the above will reject the thing, and if it's
> a 64-bit kernel, it will be a no-op, but the abort won't work in a
> 32-bit caller.
>
> Problem solved.
This assumes a 64-bit kernel returning to a 32-bit compat task with
garbage it the upper 32 bits of regs->ip behaves correctly (e.g.
kill the offending process rather than crash the kernel) on all
architectures.
Is this something we can rely on ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Linus Torvalds @ 2018-06-29 0:18 UTC (permalink / raw)
To: Andrew Lutomirski
Cc: Mathieu Desnoyers, Thomas Gleixner, Linux Kernel Mailing List,
Linux API, Peter Zijlstra, Paul McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King - ARM Linux, Ingo Molnar,
Peter Anvin, Andi Kleen, Christoph Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Catalin Marinas, Will Deacon
In-Reply-To: <CALCETrUrXk3wRg8SKhWf98v8jK=HwX9XLvP+Ypi+TktZoNx_Jg@mail.gmail.com>
On Thu, Jun 28, 2018 at 4:30 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> The idea is that, if someone screws up and sticks a number like
> 0xbaadf00d00045678 into their rseq abort_ip in a 32-bit x86 program
> (when they actually mean 0x00045678), we want to something consistent.
I think the "something consistent" is perfectly fine with just "it won't work".
Make it do
if (rseq_cs->abort_ip != (unsigned long)rseq_cs->abort_ip)
return -EINVAL;
at abort time.
Done.
If it's a 32-bit kernel, the above will reject the thing, and if it's
a 64-bit kernel, it will be a no-op, but the abort won't work in a
32-bit caller.
Problem solved.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Andy Lutomirski @ 2018-06-28 23:29 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Lutomirski, Mathieu Desnoyers, Thomas Gleixner,
Linux Kernel Mailing List, Linux API, Peter Zijlstra,
Paul McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King - ARM Linux, Ingo Molnar, Peter Anvin,
Andi Kleen, Christoph Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Catalin Marinas <catalin.marinas@
In-Reply-To: <CA+55aFxZrv9koOGKNzpBjcpzwaKQBg9ybqcvQ9u=5d94y1F+mQ@mail.gmail.com>
On Thu, Jun 28, 2018 at 2:22 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jun 28, 2018 at 1:23 PM Andy Lutomirski <luto@kernel.org> wrote:
>>
>> This is okay with me for a fix outside the merge window. Can you do a
>> followup for the next merge window that fixes it better, though? In
>> particular, TASK_SIZE is generally garbage. I think a better fix
>> would be something like adding a new arch-overridable helper like:
>>
>> static inline unsigned long current_max_user_addr(void) { return TASK_SIZE; }
>
> We already have that. It's called "user_addr_max()".
Nah, that one is more or less equivalent to TASK_SIZE_MAX, except that
it's different if set_fs() is used.
>
> It's the limit we use for user accesses.
>
> That said, I don't see why we should even check the IP. It's not like
> that's done by signal handling either.
The idea is that, if someone screws up and sticks a number like
0xbaadf00d00045678 into their rseq abort_ip in a 32-bit x86 program
(when they actually mean 0x00045678), we want to something consistent.
On a 32-bit kernel, presumably it gets cast to u32 somewhere and it
works. On a 64-bit kernel, we end up shoving 0xbaadf00d00045678 into
regs->ip, and then the entry code will do, um, something. If I had to
guess, I would guess that at least IRET is likely to truncate if we're
returning to a 32-bit CS. But I really don't want to start promising
that we won't segfault if a different path gets invoked on some future
kernel on some future CPU of if we're on an AMD CPU using their
utterly braindead SYSRETL microcode, etc.
So I think we're much better off if we either promise that rseq
truncates the address for 32-bit users or that it segfaults if high
bits are set for 32-bit users.
TASK_SIZE is a super shitty way to do this. The correct thing is to
either add some check to the exit-to-usermode slowpath that rseq can
trigger or if we add some reasonable way for rseq to say "is this
address a legitimate addressable virtual address for the current
task's user space operating mode." We don't have such a thing right
now.
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-28 22:29 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Thomas Gleixner, linux-kernel, linux-api,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.manpag>
In-Reply-To: <CA+55aFxZrv9koOGKNzpBjcpzwaKQBg9ybqcvQ9u=5d94y1F+mQ@mail.gmail.com>
----- On Jun 28, 2018, at 5:22 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Thu, Jun 28, 2018 at 1:23 PM Andy Lutomirski <luto@kernel.org> wrote:
>>
>> This is okay with me for a fix outside the merge window. Can you do a
>> followup for the next merge window that fixes it better, though? In
>> particular, TASK_SIZE is generally garbage. I think a better fix
>> would be something like adding a new arch-overridable helper like:
>>
>> static inline unsigned long current_max_user_addr(void) { return TASK_SIZE; }
>
> We already have that. It's called "user_addr_max()".
>
> It's the limit we use for user accesses.
Good point. I will simply replace TASK_SIZE with user_addr_max() then.
> That said, I don't see why we should even check the IP. It's not like
> that's done by signal handling either.
The goal stated by Andy and Thomas is to design rseq so it
does not need any compat syscall whatsoever. Considering
that this is a new system call, we should be able to do that.
One thing we want is to provide a consistent behavior when
a 32-bit binary is executed on native 32-bit kernel or on
64-bit kernel in compat mode, even if userspace chooses to
put garbage in the upper 32-bit of padding within 64-bit
fields.
Now let's look at the comparison with signals. If we look
at ia32_setup_frame() for instance, it sets:
regs->ip = (unsigned long) ksig->ka.sa.sa_handler;
where the sa_handler pointer has been received as input
as a 32-bit pointer by the compat syscall rt_sigaction
through struct compat_sigaction. I agree with you that
it does not appear to validate that it's below
user_addr_max(), but at least it's guaranteed to never be
over 32-bit.
The first big difference between rseq and signals: rseq
does not have a compat structure. The layout is the same
for both 32-bit and 64-bit userspace (see include/uapi/linux/rseq.h).
Another difference between rseq and signals is that rseq
only registers the TLS struct rseq for each thread. Then,
it's up to user-space to update the rseq_cs field of
struct rseq to indicate that it enters a critical section.
So the actual content of (struct rseq *)->rseq_cs is updated
with single-copy atomicity after registration of rseq.
Therefore, the current critical section pointed to by the current
rseq_cs user-space pointer also changes after registration. So we
cannot validate the content of the rseq_cs field, nor of the fields
contained within every possible struct rseq_cs descriptor when
registering rseq through sys_rseq: those need to be read when
returning to user-space. Not just on return from system call, but
also on return from interrupt/trap after a preemption.
This is very much different from registering a sigaction,
where the kernel can validate or truncate the content of
sa_handler at will.
Without validation of the content of e.g. rseq_cs->abort_ip
(read as a 64-bit integer by a 64-bit kernel), we end up setting
the return IP to that address on abort, even though user-space may
have put garbage in the high bits:
instruction_pointer_set(regs, (unsigned long)rseq_cs.abort_ip);
without any validation or truncation whatsoever.
I'm concerned that some architecture code may not deal so well without
prior validation or truncation of the IP register content upper 32 bits
when returning to a 32-bit compat task.
This is why I'm considering comparison of abort_ip against user_addr_max()
to ensure we're not provided with an incorrect user input.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Linus Torvalds @ 2018-06-28 21:22 UTC (permalink / raw)
To: Andrew Lutomirski
Cc: Mathieu Desnoyers, Thomas Gleixner, Linux Kernel Mailing List,
Linux API, Peter Zijlstra, Paul McKenney, Boqun Feng, Dave Watson,
Paul Turner, Andrew Morton, Russell King - ARM Linux, Ingo Molnar,
Peter Anvin, Andi Kleen, Christoph Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Catalin Marinas, Will Deacon
In-Reply-To: <CALCETrULo_VP4XHbXcZTevWY5d-6PdHUtrDS8DEF_OaozQOaNg@mail.gmail.com>
On Thu, Jun 28, 2018 at 1:23 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> This is okay with me for a fix outside the merge window. Can you do a
> followup for the next merge window that fixes it better, though? In
> particular, TASK_SIZE is generally garbage. I think a better fix
> would be something like adding a new arch-overridable helper like:
>
> static inline unsigned long current_max_user_addr(void) { return TASK_SIZE; }
We already have that. It's called "user_addr_max()".
It's the limit we use for user accesses.
That said, I don't see why we should even check the IP. It's not like
that's done by signal handling either.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Mathieu Desnoyers @ 2018-06-28 20:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H. Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon, Michael
In-Reply-To: <CALCETrULo_VP4XHbXcZTevWY5d-6PdHUtrDS8DEF_OaozQOaNg@mail.gmail.com>
----- On Jun 28, 2018, at 4:22 PM, Andy Lutomirski luto@kernel.org wrote:
> On Thu, Jun 28, 2018 at 9:23 AM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> Validating the abort_ip field of rseq_cs ensures that the kernel don't
>> return to an invalid address when returning to userspace after an abort.
>> I don't fully trust each architecture code to cleanly deal with invalid
>> return addresses.
>>
>> Validating the range [ start_ip, start_ip + post_commit_offset ] is an
>> extra validation step ensuring that userspace provides valid values to
>> describe the critical section.
>>
>> If validation fails, the process is killed with a segmentation fault.
>>
>> Change the rseq ABI so rseq_cs start_ip, post_commit_offset and abort_ip
>> fields are seen as 64-bit fields by both 32-bit and 64-bit kernels rather
>> that ignoring the 32 upper bits on 32-bit kernels. This ensures we have
>> a consistent behavior for a 32-bit binary executed on 32-bit kernels and
>> in compat mode on 64-bit kernels.
>
> This is okay with me for a fix outside the merge window. Can you do a
> followup for the next merge window that fixes it better, though? In
> particular, TASK_SIZE is generally garbage. I think a better fix
> would be something like adding a new arch-overridable helper like:
>
> static inline unsigned long current_max_user_addr(void) { return TASK_SIZE; }
>
> and overriding it on x86 as something like:
>
> static inline unsigned long current_max_user_addr(void) {
> #ifdef CONFIG_IA32_EMULATION
> return user_64bit_mode(current_pt_regs()) ? TASK_SIZE_MAX : (1UL << 32) - 1;
> #else
> return TASK_SIZE_MAX;
> }
>
> TASK_SIZE really needs to die.
Sure, I'll put it in my backlog.
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 2/2] rseq: check that rseq->rseq_cs padding is zero
From: Mathieu Desnoyers @ 2018-06-28 20:55 UTC (permalink / raw)
To: Will Deacon
Cc: Thomas Gleixner, linux-kernel, linux-api, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Michael
In-Reply-To: <20180628165348.GE10751@arm.com>
----- On Jun 28, 2018, at 12:53 PM, Will Deacon will.deacon@arm.com wrote:
> Hi Mathieu,
>
> On Thu, Jun 28, 2018 at 12:23:59PM -0400, Mathieu Desnoyers wrote:
>> On 32-bit kernels, the rseq->rseq_cs_padding field is never read by the
>> kernel. However, 64-bit kernels dealing with 32-bit compat tasks read the
>> full 64-bit in its entirety, and terminates the offending process with
>> a segmentation fault if the upper 32 bits are set due to failure of
>> copy_from_user().
>>
>> Ensure that both 32-bit and 64-bit kernels dealing with 32-bit tasks end
>> up terminating offending tasks with a segmentation fault if the upper
>> 32-bit padding bits (rseq->rseq_cs_padding) are set by adding an explicit
>> check that padding is zero on 32-bit kernels.
>>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>> CC: Peter Zijlstra <peterz@infradead.org>
>> CC: Paul Turner <pjt@google.com>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Andy Lutomirski <luto@amacapital.net>
>> CC: Andi Kleen <andi@firstfloor.org>
>> CC: Dave Watson <davejwatson@fb.com>
>> CC: Chris Lameter <cl@linux.com>
>> CC: Ingo Molnar <mingo@redhat.com>
>> CC: "H. Peter Anvin" <hpa@zytor.com>
>> CC: Ben Maurer <bmaurer@fb.com>
>> CC: Steven Rostedt <rostedt@goodmis.org>
>> CC: Josh Triplett <josh@joshtriplett.org>
>> CC: Linus Torvalds <torvalds@linux-foundation.org>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: Russell King <linux@arm.linux.org.uk>
>> CC: Catalin Marinas <catalin.marinas@arm.com>
>> CC: Will Deacon <will.deacon@arm.com>
>> CC: Michael Kerrisk <mtk.manpages@gmail.com>
>> CC: Boqun Feng <boqun.feng@gmail.com>
>> CC: linux-api@vger.kernel.org
>> ---
>> kernel/rseq.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/kernel/rseq.c b/kernel/rseq.c
>> index 4ba582046fcd..b038f35a60d6 100644
>> --- a/kernel/rseq.c
>> +++ b/kernel/rseq.c
>> @@ -112,6 +112,29 @@ static int rseq_reset_rseq_cpu_id(struct task_struct *t)
>> return 0;
>> }
>>
>> +#ifndef __LP64__
>> +/*
>> + * Ensure that padding is zero.
>> + */
>> +static int check_rseq_cs_padding(struct task_struct *t)
>> +{
>> + unsigned long pad;
>> + int ret;
>> +
>> + ret = __get_user(pad, &t->rseq->rseq_cs_padding);
>> + if (ret)
>> + return ret;
>> + if (pad)
>> + return -EFAULT;
>> + return 0;
>> +}
>> +#else
>> +static int check_rseq_cs_padding(struct task_struct *t)
>> +{
>> + return 0;
>> +}
>> +#endif
>
> I'm still not sure how this works with a 64-bit kernel and a compat (32-bit)
> task. The check_rseq_cs_padding() will return 0 regardless of the upper bits
> of the rseq_cs field, whereas a native 32-bit kernel would actually go and
> check them.
>
> What am I missing here?
With a 64-bit kernel, we end up in the #else, which means check_rseq_cs_padding()
always returns 0.
On that 64-bit kernel, all 64 bits of rseq->rseq_cs are read, including the
padding. Therefore, all those bits are contained in the pointer passed as
argument to copy_from_user(), which will cause copy_from_user() to accurately
fail on an invalid user-space address.
Therefore, 64-bit kernels already check those padding bits by means of trying to use
that pointer to access user-space data with copy_from_user, which does an access_ok
check.
So both 32-bit and 64-bit kernels will end up killing the process with segmentation
fault if a 32-bit userland populates those padding bits with anything other than
0.
Does it seem acceptable ?
Thanks,
Mathieu
>
> Will
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.18 1/2] rseq: validate rseq_cs fields are < TASK_SIZE
From: Andy Lutomirski @ 2018-06-28 20:22 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, LKML, Linux API, Peter Zijlstra,
Paul E . McKenney, Boqun Feng, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Ingo Molnar, H . Peter Anvin,
Andi Kleen, Chris Lameter, Ben Maurer, Steven Rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Micha
In-Reply-To: <20180628162359.9054-1-mathieu.desnoyers@efficios.com>
On Thu, Jun 28, 2018 at 9:23 AM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> Validating the abort_ip field of rseq_cs ensures that the kernel don't
> return to an invalid address when returning to userspace after an abort.
> I don't fully trust each architecture code to cleanly deal with invalid
> return addresses.
>
> Validating the range [ start_ip, start_ip + post_commit_offset ] is an
> extra validation step ensuring that userspace provides valid values to
> describe the critical section.
>
> If validation fails, the process is killed with a segmentation fault.
>
> Change the rseq ABI so rseq_cs start_ip, post_commit_offset and abort_ip
> fields are seen as 64-bit fields by both 32-bit and 64-bit kernels rather
> that ignoring the 32 upper bits on 32-bit kernels. This ensures we have
> a consistent behavior for a 32-bit binary executed on 32-bit kernels and
> in compat mode on 64-bit kernels.
This is okay with me for a fix outside the merge window. Can you do a
followup for the next merge window that fixes it better, though? In
particular, TASK_SIZE is generally garbage. I think a better fix
would be something like adding a new arch-overridable helper like:
static inline unsigned long current_max_user_addr(void) { return TASK_SIZE; }
and overriding it on x86 as something like:
static inline unsigned long current_max_user_addr(void) {
#ifdef CONFIG_IA32_EMULATION
return user_64bit_mode(current_pt_regs()) ? TASK_SIZE_MAX : (1UL << 32) - 1;
#else
return TASK_SIZE_MAX;
}
TASK_SIZE really needs to die.
^ permalink raw reply
* Re: [PATCH] mm/madvise: allow MADV_DONTNEED to free memory that is MLOCK_ONFAULT
From: Jason Baron @ 2018-06-28 20:20 UTC (permalink / raw)
To: Michal Hocko
Cc: Vlastimil Babka, akpm, linux-kernel, linux-mm, Joonsoo Kim,
Mel Gorman, Kirill A. Shutemov, linux-api, emunson
In-Reply-To: <20180620110022.GK13685@dhcp22.suse.cz>
On 06/20/2018 07:00 AM, Michal Hocko wrote:
> On Fri 15-06-18 15:36:07, Jason Baron wrote:
>>
>>
>> On 06/13/2018 03:15 AM, Michal Hocko wrote:
>>> On Wed 13-06-18 08:32:19, Vlastimil Babka wrote:
> [...]
>>>> BTW I didn't get why we should allow this for MADV_DONTNEED but not
>>>> MADV_FREE. Can you expand on that?
>>>
>>> Well, I wanted to bring this up as well. I guess this would require some
>>> more hacks to handle the reclaim path correctly because we do rely on
>>> VM_LOCK at many places for the lazy mlock pages culling.
>>>
>>
>> The point of not allowing MADV_FREE on mlock'd pages for me was that
>> with mlock and even MLOCK_ON_FAULT, one can always can always determine
>> if a page is present or not (and thus avoid the major fault). Allowing
>> MADV_FREE on lock'd pages breaks that assumption.
>
> But once you have called MADV_FREE you cannot assume anything about the
> content until you touch the memory again. So you can safely assume a
> major fault for the worst case. Btw. why knowing whether you major fault
> is important in the first place? What is an application going to do
> about that information?
>
Fair enough, I think that means you end up with a MADV_FREE_FORCE to
support that case? As I said I worked around this by using tmpfs and
fallocate(FALLOC_FL_PUNCH_HOLE). However, I still think there is a
use-case for doing this for anonymous memory, to avoid the unlock() calls.
The use-case I had in mind was simply an application that has a fast
path for when it knows that the requested item is locked in memory.
Thanks,
-Jason
^ permalink raw reply
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