* Re: [PATCH v3 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Phil Auld @ 2019-05-29 19:28 UTC (permalink / raw)
To: Dave Chiluk
Cc: Ben Segall, Peter Oskolkov, Peter Zijlstra, Ingo Molnar, cgroups,
linux-kernel, Brendan Gregg, Kyle Anderson, Gabriel Munos,
John Hammond, Cong Wang, Jonathan Corbet, linux-doc
In-Reply-To: <1559156926-31336-2-git-send-email-chiluk+linux@indeed.com>
On Wed, May 29, 2019 at 02:08:46PM -0500 Dave Chiluk wrote:
> It has been observed, that highly-threaded, non-cpu-bound applications
> running under cpu.cfs_quota_us constraints can hit a high percentage of
> periods throttled while simultaneously not consuming the allocated
> amount of quota. This use case is typical of user-interactive non-cpu
> bound applications, such as those running in kubernetes or mesos when
> run on multiple cpu cores.
>
> This has been root caused to threads being allocated per cpu bandwidth
> slices, and then not fully using that slice within the period. At which
> point the slice and quota expires. This expiration of unused slice
> results in applications not being able to utilize the quota for which
> they are allocated.
>
> The expiration of per-cpu slices was recently fixed by
> 'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
> condition")'. Prior to that it appears that this has been broken since
> at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
> cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
> added the following conditional which resulted in slices never being
> expired.
>
> if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
> /* extend local deadline, drift is bounded above by 2 ticks */
> cfs_rq->runtime_expires += TICK_NSEC;
>
> Because this was broken for nearly 5 years, and has recently been fixed
> and is now being noticed by many users running kubernetes
> (https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
> that the mechanisms around expiring runtime should be removed
> altogether.
>
> This allows only per-cpu slices to live longer than the period boundary.
> This allows threads on runqueues that do not use much CPU to continue to
> use their remaining slice over a longer period of time than
> cpu.cfs_period_us. However, this helps prevents the above condition of
> hitting throttling while also not fully utilizing your cpu quota.
>
> This theoretically allows a machine to use slightly more than it's
> allotted quota in some periods. This overflow would be bounded by the
> remaining per-cpu slice that was left un-used in the previous period.
> For CPU bound tasks this will change nothing, as they should
> theoretically fully utilize all of their quota and slices in each
> period. For user-interactive tasks as described above this provides a
> much better user/application experience as their cpu utilization will
> more closely match the amount they requested when they hit throttling.
>
> This greatly improves performance of high-thread-count, non-cpu bound
> applications with low cfs_quota_us allocation on high-core-count
> machines. In the case of an artificial testcase, this performance
> discrepancy has been observed to be almost 30x performance improvement,
> while still maintaining correct cpu quota restrictions albeit over
> longer time intervals than cpu.cfs_period_us. That testcase is
> available at https://github.com/indeedeng/fibtest.
>
> Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
> Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
> ---
> Documentation/scheduler/sched-bwc.txt | 56 ++++++++++++++++++++++-----
> kernel/sched/fair.c | 71 +++--------------------------------
> kernel/sched/sched.h | 4 --
> 3 files changed, 53 insertions(+), 78 deletions(-)
>
> diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
> index f6b1873..260fd65 100644
> --- a/Documentation/scheduler/sched-bwc.txt
> +++ b/Documentation/scheduler/sched-bwc.txt
> @@ -8,15 +8,16 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
> specification of the maximum CPU bandwidth available to a group or hierarchy.
>
> The bandwidth allowed for a group is specified using a quota and period. Within
> -each given "period" (microseconds), a group is allowed to consume only up to
> -"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
> -group exceeds this limit (for that period), the tasks belonging to its
> -hierarchy will be throttled and are not allowed to run again until the next
> -period.
> -
> -A group's unused runtime is globally tracked, being refreshed with quota units
> -above at each period boundary. As threads consume this bandwidth it is
> -transferred to cpu-local "silos" on a demand basis. The amount transferred
> +each given "period" (microseconds), a task group is allocated up to "quota"
> +microseconds of CPU time. That quota is assigned to per cpu run queues in
> +slices as threads in the cgroup become runnable. Once all quota has been
> +assigned any additional requests for quota will result in those threads being
> +throttled. Throttled threads will not be able to run again until the next
> +period when the quota is replenished.
> +
> +A group's unassigned quota is globally tracked, being refreshed back to
> +cfs_quota units at each period boundary. As threads consume this bandwidth it
> +is transferred to cpu-local "silos" on a demand basis. The amount transferred
> within each of these updates is tunable and described as the "slice".
>
> Management
> @@ -90,6 +91,43 @@ There are two ways in which a group may become throttled:
> In case b) above, even though the child may have runtime remaining it will not
> be allowed to until the parent's runtime is refreshed.
>
> +Real-world behavior of slice non-expiration
> +-------------------------------------------
> +The fact that cpu-local slices do not expire results in some interesting corner
> +cases that should be understood.
> +
> +For cgroup cpu constrained applications that are cpu limited this is a
> +relatively moot point because they will naturally consume the entirety of their
> +quota as well as the entirety of each cpu-local slice in each period. As a
> +result it is expected that nr_periods roughly equal nr_throttled, and that
> +cpuacct.usage will increase roughly equal to cfs_quota_us in each period.
> +
> +However in a worst-case scenario, highly-threaded, interactive/non-cpu bound
> +applications this non-expiration nuance allows applications to briefly burst
> +past their quota limits by the amount of unused slice on each cpu that the task
> +group is running on. This slight burst requires that quota had been assigned
> +and then not fully used in previous periods. This burst amount will not be
> +transferred between cores. As a result, this mechanism still strictly limits
> +the task group to quota average usage, albeit over a longer time window than
> +period. This provides better more predictable user experience for highly
> +threaded applications with small quota limits on high core count machines. It
> +also eliminates the propensity to throttle these applications while
> +simultanously using less than quota amounts of cpu. Another way to say this,
> +is that by allowing the unused portion of a slice to remain valid across
> +periods we have decreased the possibility of wasting quota on cpu-local silos
> +that don't need a full slice's amount of cpu time.
> +
> +The interaction between cpu-bound and non-cpu-bound-interactive applications
> +should also be considered, especially when single core usage hits 100%. If you
> +gave each of these applications half of a cpu-core and they both got scheduled
> +on the same CPU it is theoretically possible that the non-cpu bound application
> +will use up to sched_cfs_bandwidth_slice_us additional quota in some periods,
> +thereby preventing the cpu-bound application from fully using it's quota by
"its quota"
> +that same amount. In these instances it will be up to the CFS algorithm (see
> +sched-design-CFS.txt) to decide which application is chosen to run, as they
> +will both be runnable and have remaining quota. This runtime discrepancy will
> +should made up in the following periods when the interactive application idles.
> +
"discrepancy will be made" or "descrepancy should be made" but not both :)
Otherwise, fwiw,
Acked-by: Phil Auld <pauld@redhat.com>
Cheers,
Phil
--
^ permalink raw reply
* [PATCH v3 0/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Dave Chiluk @ 2019-05-29 19:08 UTC (permalink / raw)
To: Ben Segall, Phil Auld, Peter Oskolkov, Peter Zijlstra,
Ingo Molnar, cgroups, linux-kernel, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc
In-Reply-To: <1558121424-2914-1-git-send-email-chiluk+linux@indeed.com>
Changelog v3
- Reworked documentation to better describe behavior of slice expiration
per feedback from Peter Oskolkov
Changelog v2
- Fixed some checkpatch errors in the commit message.
^ permalink raw reply
* [PATCH v3 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Dave Chiluk @ 2019-05-29 19:08 UTC (permalink / raw)
To: Ben Segall, Phil Auld, Peter Oskolkov, Peter Zijlstra,
Ingo Molnar, cgroups, linux-kernel, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc
In-Reply-To: <1559156926-31336-1-git-send-email-chiluk+linux@indeed.com>
It has been observed, that highly-threaded, non-cpu-bound applications
running under cpu.cfs_quota_us constraints can hit a high percentage of
periods throttled while simultaneously not consuming the allocated
amount of quota. This use case is typical of user-interactive non-cpu
bound applications, such as those running in kubernetes or mesos when
run on multiple cpu cores.
This has been root caused to threads being allocated per cpu bandwidth
slices, and then not fully using that slice within the period. At which
point the slice and quota expires. This expiration of unused slice
results in applications not being able to utilize the quota for which
they are allocated.
The expiration of per-cpu slices was recently fixed by
'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
condition")'. Prior to that it appears that this has been broken since
at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
added the following conditional which resulted in slices never being
expired.
if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
/* extend local deadline, drift is bounded above by 2 ticks */
cfs_rq->runtime_expires += TICK_NSEC;
Because this was broken for nearly 5 years, and has recently been fixed
and is now being noticed by many users running kubernetes
(https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
that the mechanisms around expiring runtime should be removed
altogether.
This allows only per-cpu slices to live longer than the period boundary.
This allows threads on runqueues that do not use much CPU to continue to
use their remaining slice over a longer period of time than
cpu.cfs_period_us. However, this helps prevents the above condition of
hitting throttling while also not fully utilizing your cpu quota.
This theoretically allows a machine to use slightly more than it's
allotted quota in some periods. This overflow would be bounded by the
remaining per-cpu slice that was left un-used in the previous period.
For CPU bound tasks this will change nothing, as they should
theoretically fully utilize all of their quota and slices in each
period. For user-interactive tasks as described above this provides a
much better user/application experience as their cpu utilization will
more closely match the amount they requested when they hit throttling.
This greatly improves performance of high-thread-count, non-cpu bound
applications with low cfs_quota_us allocation on high-core-count
machines. In the case of an artificial testcase, this performance
discrepancy has been observed to be almost 30x performance improvement,
while still maintaining correct cpu quota restrictions albeit over
longer time intervals than cpu.cfs_period_us. That testcase is
available at https://github.com/indeedeng/fibtest.
Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
---
Documentation/scheduler/sched-bwc.txt | 56 ++++++++++++++++++++++-----
kernel/sched/fair.c | 71 +++--------------------------------
kernel/sched/sched.h | 4 --
3 files changed, 53 insertions(+), 78 deletions(-)
diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
index f6b1873..260fd65 100644
--- a/Documentation/scheduler/sched-bwc.txt
+++ b/Documentation/scheduler/sched-bwc.txt
@@ -8,15 +8,16 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
specification of the maximum CPU bandwidth available to a group or hierarchy.
The bandwidth allowed for a group is specified using a quota and period. Within
-each given "period" (microseconds), a group is allowed to consume only up to
-"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
-group exceeds this limit (for that period), the tasks belonging to its
-hierarchy will be throttled and are not allowed to run again until the next
-period.
-
-A group's unused runtime is globally tracked, being refreshed with quota units
-above at each period boundary. As threads consume this bandwidth it is
-transferred to cpu-local "silos" on a demand basis. The amount transferred
+each given "period" (microseconds), a task group is allocated up to "quota"
+microseconds of CPU time. That quota is assigned to per cpu run queues in
+slices as threads in the cgroup become runnable. Once all quota has been
+assigned any additional requests for quota will result in those threads being
+throttled. Throttled threads will not be able to run again until the next
+period when the quota is replenished.
+
+A group's unassigned quota is globally tracked, being refreshed back to
+cfs_quota units at each period boundary. As threads consume this bandwidth it
+is transferred to cpu-local "silos" on a demand basis. The amount transferred
within each of these updates is tunable and described as the "slice".
Management
@@ -90,6 +91,43 @@ There are two ways in which a group may become throttled:
In case b) above, even though the child may have runtime remaining it will not
be allowed to until the parent's runtime is refreshed.
+Real-world behavior of slice non-expiration
+-------------------------------------------
+The fact that cpu-local slices do not expire results in some interesting corner
+cases that should be understood.
+
+For cgroup cpu constrained applications that are cpu limited this is a
+relatively moot point because they will naturally consume the entirety of their
+quota as well as the entirety of each cpu-local slice in each period. As a
+result it is expected that nr_periods roughly equal nr_throttled, and that
+cpuacct.usage will increase roughly equal to cfs_quota_us in each period.
+
+However in a worst-case scenario, highly-threaded, interactive/non-cpu bound
+applications this non-expiration nuance allows applications to briefly burst
+past their quota limits by the amount of unused slice on each cpu that the task
+group is running on. This slight burst requires that quota had been assigned
+and then not fully used in previous periods. This burst amount will not be
+transferred between cores. As a result, this mechanism still strictly limits
+the task group to quota average usage, albeit over a longer time window than
+period. This provides better more predictable user experience for highly
+threaded applications with small quota limits on high core count machines. It
+also eliminates the propensity to throttle these applications while
+simultanously using less than quota amounts of cpu. Another way to say this,
+is that by allowing the unused portion of a slice to remain valid across
+periods we have decreased the possibility of wasting quota on cpu-local silos
+that don't need a full slice's amount of cpu time.
+
+The interaction between cpu-bound and non-cpu-bound-interactive applications
+should also be considered, especially when single core usage hits 100%. If you
+gave each of these applications half of a cpu-core and they both got scheduled
+on the same CPU it is theoretically possible that the non-cpu bound application
+will use up to sched_cfs_bandwidth_slice_us additional quota in some periods,
+thereby preventing the cpu-bound application from fully using it's quota by
+that same amount. In these instances it will be up to the CFS algorithm (see
+sched-design-CFS.txt) to decide which application is chosen to run, as they
+will both be runnable and have remaining quota. This runtime discrepancy will
+should made up in the following periods when the interactive application idles.
+
Examples
--------
1. Limit a group to 1 CPU worth of runtime.
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f35930f..a675c69 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4295,8 +4295,6 @@ void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
now = sched_clock_cpu(smp_processor_id());
cfs_b->runtime = cfs_b->quota;
- cfs_b->runtime_expires = now + ktime_to_ns(cfs_b->period);
- cfs_b->expires_seq++;
}
static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
@@ -4318,8 +4316,7 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
{
struct task_group *tg = cfs_rq->tg;
struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
- u64 amount = 0, min_amount, expires;
- int expires_seq;
+ u64 amount = 0, min_amount;
/* note: this is a positive sum as runtime_remaining <= 0 */
min_amount = sched_cfs_bandwidth_slice() - cfs_rq->runtime_remaining;
@@ -4336,61 +4333,17 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
cfs_b->idle = 0;
}
}
- expires_seq = cfs_b->expires_seq;
- expires = cfs_b->runtime_expires;
raw_spin_unlock(&cfs_b->lock);
cfs_rq->runtime_remaining += amount;
- /*
- * we may have advanced our local expiration to account for allowed
- * spread between our sched_clock and the one on which runtime was
- * issued.
- */
- if (cfs_rq->expires_seq != expires_seq) {
- cfs_rq->expires_seq = expires_seq;
- cfs_rq->runtime_expires = expires;
- }
return cfs_rq->runtime_remaining > 0;
}
-/*
- * Note: This depends on the synchronization provided by sched_clock and the
- * fact that rq->clock snapshots this value.
- */
-static void expire_cfs_rq_runtime(struct cfs_rq *cfs_rq)
-{
- struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
-
- /* if the deadline is ahead of our clock, nothing to do */
- if (likely((s64)(rq_clock(rq_of(cfs_rq)) - cfs_rq->runtime_expires) < 0))
- return;
-
- if (cfs_rq->runtime_remaining < 0)
- return;
-
- /*
- * If the local deadline has passed we have to consider the
- * possibility that our sched_clock is 'fast' and the global deadline
- * has not truly expired.
- *
- * Fortunately we can check determine whether this the case by checking
- * whether the global deadline(cfs_b->expires_seq) has advanced.
- */
- if (cfs_rq->expires_seq == cfs_b->expires_seq) {
- /* extend local deadline, drift is bounded above by 2 ticks */
- cfs_rq->runtime_expires += TICK_NSEC;
- } else {
- /* global deadline is ahead, expiration has passed */
- cfs_rq->runtime_remaining = 0;
- }
-}
-
static void __account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec)
{
/* dock delta_exec before expiring quota (as it could span periods) */
cfs_rq->runtime_remaining -= delta_exec;
- expire_cfs_rq_runtime(cfs_rq);
if (likely(cfs_rq->runtime_remaining > 0))
return;
@@ -4581,8 +4534,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
resched_curr(rq);
}
-static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
- u64 remaining, u64 expires)
+static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b, u64 remaining)
{
struct cfs_rq *cfs_rq;
u64 runtime;
@@ -4604,7 +4556,6 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
remaining -= runtime;
cfs_rq->runtime_remaining += runtime;
- cfs_rq->runtime_expires = expires;
/* we check whether we're throttled above */
if (cfs_rq->runtime_remaining > 0)
@@ -4629,7 +4580,7 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
*/
static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
{
- u64 runtime, runtime_expires;
+ u64 runtime;
int throttled;
/* no need to continue the timer with no bandwidth constraint */
@@ -4657,8 +4608,6 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
/* account preceding periods in which throttling occurred */
cfs_b->nr_throttled += overrun;
- runtime_expires = cfs_b->runtime_expires;
-
/*
* This check is repeated as we are holding onto the new bandwidth while
* we unthrottle. This can potentially race with an unthrottled group
@@ -4671,8 +4620,7 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
cfs_b->distribute_running = 1;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
/* we can't nest cfs_b->lock while distributing bandwidth */
- runtime = distribute_cfs_runtime(cfs_b, runtime,
- runtime_expires);
+ runtime = distribute_cfs_runtime(cfs_b, runtime);
raw_spin_lock_irqsave(&cfs_b->lock, flags);
cfs_b->distribute_running = 0;
@@ -4749,8 +4697,7 @@ static void __return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
return;
raw_spin_lock(&cfs_b->lock);
- if (cfs_b->quota != RUNTIME_INF &&
- cfs_rq->runtime_expires == cfs_b->runtime_expires) {
+ if (cfs_b->quota != RUNTIME_INF) {
cfs_b->runtime += slack_runtime;
/* we are under rq->lock, defer unthrottling using a timer */
@@ -4783,7 +4730,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
{
u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
unsigned long flags;
- u64 expires;
/* confirm we're still not at a refresh boundary */
raw_spin_lock_irqsave(&cfs_b->lock, flags);
@@ -4800,7 +4746,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
runtime = cfs_b->runtime;
- expires = cfs_b->runtime_expires;
if (runtime)
cfs_b->distribute_running = 1;
@@ -4809,11 +4754,9 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
if (!runtime)
return;
- runtime = distribute_cfs_runtime(cfs_b, runtime, expires);
+ runtime = distribute_cfs_runtime(cfs_b, runtime);
raw_spin_lock_irqsave(&cfs_b->lock, flags);
- if (expires == cfs_b->runtime_expires)
- lsub_positive(&cfs_b->runtime, runtime);
cfs_b->distribute_running = 0;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
}
@@ -4969,8 +4912,6 @@ void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
cfs_b->period_active = 1;
overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
- cfs_b->runtime_expires += (overrun + 1) * ktime_to_ns(cfs_b->period);
- cfs_b->expires_seq++;
hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b52ed1a..0c0ed23 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -341,8 +341,6 @@ struct cfs_bandwidth {
u64 quota;
u64 runtime;
s64 hierarchical_quota;
- u64 runtime_expires;
- int expires_seq;
short idle;
short period_active;
@@ -562,8 +560,6 @@ struct cfs_rq {
#ifdef CONFIG_CFS_BANDWIDTH
int runtime_enabled;
- int expires_seq;
- u64 runtime_expires;
s64 runtime_remaining;
u64 throttled_clock;
--
1.8.3.1
^ permalink raw reply related
* [GIT PULL] Fixes for sphinx 2.0 docs build failures
From: Jonathan Corbet @ 2019-05-29 15:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: LKML, linux-doc
The following changes since commit
a65fd4f0def56f59822b2c49522d36319bc8da8b:
Documentation: kdump: fix minor typo (2019-05-21 09:31:28 -0600)
are available in the Git repository at:
git://git.lwn.net/linux.git tags/docs-5.2-fixes2
for you to fetch changes up to 551bd3368a7b3cfef01edaade8970948d178d40a:
drm/i915: Maintain consistent documentation subsection ordering (2019-05-24 09:15:45 -0600)
----------------------------------------------------------------
The Sphinx 2.0 release contained a few incompatible API changes that broke
our extensions and, thus, the documentation build in general. Who knew
that those deprecation warnings it was outputting actually meant we should
change something? This set of fixes makes the build work again with
Sphinx 2.0 and eliminates the warnings for 1.8. As part of that, we also
need a few fixes to the docs for places where the new Sphinx is more
strict.
It is a bit late in the cycle for this kind of change, but it does fix
problems that people are experiencing now.
There has been some talk of raising the minimum version of Sphinx we
support. I don't want to do that abruptly, though, so these changes add
some glue to continue to support versions back to 1.3. We will be adding
some infrastructure soon to nudge users of old versions forward, with the
idea of maybe increasing our minimum version (and removing this glue)
sometime in the future.
----------------------------------------------------------------
Jonathan Corbet (7):
doc: Cope with Sphinx logging deprecations
doc: Cope with the deprecation of AutoReporter
docs: fix numaperf.rst and add it to the doc tree
lib/list_sort: fix kerneldoc build error
docs: fix multiple doc build warnings in enumeration.rst
docs: Fix conf.py for Sphinx 2.0
drm/i915: Maintain consistent documentation subsection ordering
Mauro Carvalho Chehab (1):
scripts/sphinx-pre-install: make it handle Sphinx versions
Documentation/admin-guide/mm/index.rst | 1 +
Documentation/admin-guide/mm/numaperf.rst | 2 +-
Documentation/conf.py | 2 +-
Documentation/firmware-guide/acpi/enumeration.rst | 2 +-
Documentation/sphinx/kerneldoc.py | 44 +++++++++---
Documentation/sphinx/kernellog.py | 28 ++++++++
Documentation/sphinx/kfigure.py | 40 ++++++-----
drivers/gpu/drm/i915/i915_reg.h | 6 +-
drivers/gpu/drm/i915/intel_workarounds.c | 2 +-
lib/list_sort.c | 3 +-
scripts/sphinx-pre-install | 86 +++++++++++++++++++++--
11 files changed, 173 insertions(+), 43 deletions(-)
create mode 100644 Documentation/sphinx/kernellog.py
^ permalink raw reply
* Re: [PATCH v2 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Marco Elver @ 2019-05-29 15:40 UTC (permalink / raw)
To: Mark Rutland
Cc: peterz, Andrey Ryabinin, Dmitry Vyukov, Alexander Potapenko,
Andrey Konovalov, corbet, tglx, mingo, bp, hpa, x86, arnd,
jpoimboe, linux-doc, LKML, linux-arch, kasan-dev
In-Reply-To: <20190529153258.GJ31777@lakrids.cambridge.arm.com>
On Wed, 29 May 2019 at 17:33, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, May 29, 2019 at 04:15:01PM +0200, Marco Elver wrote:
> > This adds a new header to asm-generic to allow optionally instrumenting
> > architecture-specific asm implementations of bitops.
> >
> > This change includes the required change for x86 as reference and
> > changes the kernel API doc to point to bitops-instrumented.h instead.
> > Rationale: the functions in x86's bitops.h are no longer the kernel API
> > functions, but instead the arch_ prefixed functions, which are then
> > instrumented via bitops-instrumented.h.
> >
> > Other architectures can similarly add support for asm implementations of
> > bitops.
> >
> > The documentation text has been copied/moved, and *no* changes to it
> > have been made in this patch.
> >
> > Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
> >
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> > Changes in v2:
> > * Instrument word-sized accesses, as specified by the interface.
> > ---
> > Documentation/core-api/kernel-api.rst | 2 +-
> > arch/x86/include/asm/bitops.h | 210 ++++----------
> > include/asm-generic/bitops-instrumented.h | 317 ++++++++++++++++++++++
> > 3 files changed, 370 insertions(+), 159 deletions(-)
> > create mode 100644 include/asm-generic/bitops-instrumented.h
>
> [...]
>
> > diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
> > new file mode 100644
> > index 000000000000..b01b0dd93964
> > --- /dev/null
> > +++ b/include/asm-generic/bitops-instrumented.h
> > @@ -0,0 +1,317 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * This file provides wrappers with sanitizer instrumentation for bit
> > + * operations.
> > + *
> > + * To use this functionality, an arch's bitops.h file needs to define each of
> > + * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
> > + * arch___set_bit(), etc.), #define each provided arch_ function, and include
> > + * this file after their definitions. For undefined arch_ functions, it is
> > + * assumed that they are provided via asm-generic/bitops, which are implicitly
> > + * instrumented.
> > + */
>
> If using the asm-generic/bitops.h, all of the below will be defined
> unconditionally, so I don't believe we need the ifdeffery for each
> function.
>
> > +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> > +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> > +
> > +#include <linux/kasan-checks.h>
> > +
> > +#if defined(arch_set_bit)
> > +/**
> > + * set_bit - Atomically set a bit in memory
> > + * @nr: the bit to set
> > + * @addr: the address to start counting from
> > + *
> > + * This function is atomic and may not be reordered. See __set_bit()
> > + * if you do not require the atomic guarantees.
> > + *
> > + * Note: there are no guarantees that this function will not be reordered
> > + * on non x86 architectures, so if you are writing portable code,
> > + * make sure not to rely on its reordering guarantees.
>
> These two paragraphs are contradictory.
>
> Since this is not under arch/x86, please fix this to describe the
> generic semantics; any x86-specific behaviour should be commented under
> arch/x86.
>
> AFAICT per include/asm-generic/bitops/atomic.h, generically this
> provides no ordering guarantees. So I think this can be:
>
> /**
> * set_bit - Atomically set a bit in memory
> * @nr: the bit to set
> * @addr: the address to start counting from
> *
> * This function is atomic and may be reordered.
> *
> * Note that @nr may be almost arbitrarily large; this function is not
> * restricted to acting on a single-word quantity.
> */
>
> ... with the x86 ordering beahviour commented in x86's arch_set_bit.
>
> Peter, do you have a better wording for the above?
>
> [...]
>
> > +#if defined(arch___test_and_clear_bit)
> > +/**
> > + * __test_and_clear_bit - Clear a bit and return its old value
> > + * @nr: Bit to clear
> > + * @addr: Address to count from
> > + *
> > + * This operation is non-atomic and can be reordered.
> > + * If two examples of this operation race, one can appear to succeed
> > + * but actually fail. You must protect multiple accesses with a lock.
> > + *
> > + * Note: the operation is performed atomically with respect to
> > + * the local CPU, but not other CPUs. Portable code should not
> > + * rely on this behaviour.
> > + * KVM relies on this behaviour on x86 for modifying memory that is also
> > + * accessed from a hypervisor on the same CPU if running in a VM: don't change
> > + * this without also updating arch/x86/kernel/kvm.c
> > + */
>
> Likewise, please only specify the generic semantics in this header, and
> leave the x86-specific behaviour commented under arch/x86.
The current official API documentation refers to x86 bitops.h (also
see the Documentation/core-api/kernel-api.rst change):
https://www.kernel.org/doc/htmldocs/kernel-api/API-set-bit.html
I'm happy to change in this patch, but note that this would change the
official API documentation. Alternatively it could be done in a
separate patch.
Let me know what you prefer.
Thanks,
-- Marco
^ permalink raw reply
* Re: [PATCH v2 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Mark Rutland @ 2019-05-29 15:32 UTC (permalink / raw)
To: Marco Elver, peterz
Cc: aryabinin, dvyukov, glider, andreyknvl, corbet, tglx, mingo, bp,
hpa, x86, arnd, jpoimboe, linux-doc, linux-kernel, linux-arch,
kasan-dev
In-Reply-To: <20190529141500.193390-4-elver@google.com>
On Wed, May 29, 2019 at 04:15:01PM +0200, Marco Elver wrote:
> This adds a new header to asm-generic to allow optionally instrumenting
> architecture-specific asm implementations of bitops.
>
> This change includes the required change for x86 as reference and
> changes the kernel API doc to point to bitops-instrumented.h instead.
> Rationale: the functions in x86's bitops.h are no longer the kernel API
> functions, but instead the arch_ prefixed functions, which are then
> instrumented via bitops-instrumented.h.
>
> Other architectures can similarly add support for asm implementations of
> bitops.
>
> The documentation text has been copied/moved, and *no* changes to it
> have been made in this patch.
>
> Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> Changes in v2:
> * Instrument word-sized accesses, as specified by the interface.
> ---
> Documentation/core-api/kernel-api.rst | 2 +-
> arch/x86/include/asm/bitops.h | 210 ++++----------
> include/asm-generic/bitops-instrumented.h | 317 ++++++++++++++++++++++
> 3 files changed, 370 insertions(+), 159 deletions(-)
> create mode 100644 include/asm-generic/bitops-instrumented.h
[...]
> diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
> new file mode 100644
> index 000000000000..b01b0dd93964
> --- /dev/null
> +++ b/include/asm-generic/bitops-instrumented.h
> @@ -0,0 +1,317 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * This file provides wrappers with sanitizer instrumentation for bit
> + * operations.
> + *
> + * To use this functionality, an arch's bitops.h file needs to define each of
> + * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
> + * arch___set_bit(), etc.), #define each provided arch_ function, and include
> + * this file after their definitions. For undefined arch_ functions, it is
> + * assumed that they are provided via asm-generic/bitops, which are implicitly
> + * instrumented.
> + */
If using the asm-generic/bitops.h, all of the below will be defined
unconditionally, so I don't believe we need the ifdeffery for each
function.
> +#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> +#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
> +
> +#include <linux/kasan-checks.h>
> +
> +#if defined(arch_set_bit)
> +/**
> + * set_bit - Atomically set a bit in memory
> + * @nr: the bit to set
> + * @addr: the address to start counting from
> + *
> + * This function is atomic and may not be reordered. See __set_bit()
> + * if you do not require the atomic guarantees.
> + *
> + * Note: there are no guarantees that this function will not be reordered
> + * on non x86 architectures, so if you are writing portable code,
> + * make sure not to rely on its reordering guarantees.
These two paragraphs are contradictory.
Since this is not under arch/x86, please fix this to describe the
generic semantics; any x86-specific behaviour should be commented under
arch/x86.
AFAICT per include/asm-generic/bitops/atomic.h, generically this
provides no ordering guarantees. So I think this can be:
/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
* @addr: the address to start counting from
*
* This function is atomic and may be reordered.
*
* Note that @nr may be almost arbitrarily large; this function is not
* restricted to acting on a single-word quantity.
*/
... with the x86 ordering beahviour commented in x86's arch_set_bit.
Peter, do you have a better wording for the above?
[...]
> +#if defined(arch___test_and_clear_bit)
> +/**
> + * __test_and_clear_bit - Clear a bit and return its old value
> + * @nr: Bit to clear
> + * @addr: Address to count from
> + *
> + * This operation is non-atomic and can be reordered.
> + * If two examples of this operation race, one can appear to succeed
> + * but actually fail. You must protect multiple accesses with a lock.
> + *
> + * Note: the operation is performed atomically with respect to
> + * the local CPU, but not other CPUs. Portable code should not
> + * rely on this behaviour.
> + * KVM relies on this behaviour on x86 for modifying memory that is also
> + * accessed from a hypervisor on the same CPU if running in a VM: don't change
> + * this without also updating arch/x86/kernel/kvm.c
> + */
Likewise, please only specify the generic semantics in this header, and
leave the x86-specific behaviour commented under arch/x86.
Otherwise this looks sound to me.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v2 1/3] lib/test_kasan: Add bitops tests
From: Mark Rutland @ 2019-05-29 15:15 UTC (permalink / raw)
To: Marco Elver
Cc: peterz, aryabinin, dvyukov, glider, andreyknvl, corbet, tglx,
mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc, linux-kernel,
linux-arch, kasan-dev
In-Reply-To: <20190529141500.193390-2-elver@google.com>
On Wed, May 29, 2019 at 04:14:59PM +0200, Marco Elver wrote:
> This adds bitops tests to the test_kasan module. In a follow-up patch,
> support for bitops instrumentation will be added.
>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> Changes in v2:
> * Use BITS_PER_LONG.
> * Use heap allocated memory for test, as newer compilers (correctly)
> warn on OOB stack access.
> ---
> lib/test_kasan.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> index 7de2702621dc..6562df0ca30d 100644
> --- a/lib/test_kasan.c
> +++ b/lib/test_kasan.c
> @@ -11,16 +11,17 @@
>
> #define pr_fmt(fmt) "kasan test: %s " fmt, __func__
>
> +#include <linux/bitops.h>
> #include <linux/delay.h>
> +#include <linux/kasan.h>
> #include <linux/kernel.h>
> -#include <linux/mman.h>
> #include <linux/mm.h>
> +#include <linux/mman.h>
> +#include <linux/module.h>
> #include <linux/printk.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/uaccess.h>
> -#include <linux/module.h>
> -#include <linux/kasan.h>
>
> /*
> * Note: test functions are marked noinline so that their names appear in
> @@ -623,6 +624,73 @@ static noinline void __init kasan_strings(void)
> strnlen(ptr, 1);
> }
>
> +static noinline void __init kasan_bitops(void)
> +{
> + long *bits = kmalloc(sizeof(long), GFP_KERNEL | __GFP_ZERO);
Trivial nit, but this can/should be:
long *bits = kzalloc(sizeof(*bits), GFP_KERNEL);
... which is the usual style for sizeof() to keep the LHS and RHS types
the same, and using kzalloc avoids the need to explicitly pass
__GFP_ZERO.
Otherwise, this looks good to me.
> + if (!bits)
> + return;
> +
> + pr_info("within-bounds in set_bit");
> + set_bit(0, bits);
> +
> + pr_info("within-bounds in set_bit");
> + set_bit(BITS_PER_LONG - 1, bits);
> +
> + pr_info("out-of-bounds in set_bit\n");
> + set_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __set_bit\n");
> + __set_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in clear_bit\n");
> + clear_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __clear_bit\n");
> + __clear_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in clear_bit_unlock\n");
> + clear_bit_unlock(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __clear_bit_unlock\n");
> + __clear_bit_unlock(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in change_bit\n");
> + change_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __change_bit\n");
> + __change_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_and_set_bit\n");
> + test_and_set_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __test_and_set_bit\n");
> + __test_and_set_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_and_set_bit_lock\n");
> + test_and_set_bit_lock(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_and_clear_bit\n");
> + test_and_clear_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __test_and_clear_bit\n");
> + __test_and_clear_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_and_change_bit\n");
> + test_and_change_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in __test_and_change_bit\n");
> + __test_and_change_bit(BITS_PER_LONG, bits);
> +
> + pr_info("out-of-bounds in test_bit\n");
> + (void)test_bit(BITS_PER_LONG, bits);
> +
> +#if defined(clear_bit_unlock_is_negative_byte)
> + pr_info("out-of-bounds in clear_bit_unlock_is_negative_byte\n");
> + clear_bit_unlock_is_negative_byte(BITS_PER_LONG, bits);
> +#endif
> + kfree(bits);
> +}
> +
> static int __init kmalloc_tests_init(void)
> {
> /*
> @@ -664,6 +732,7 @@ static int __init kmalloc_tests_init(void)
> kasan_memchr();
> kasan_memcmp();
> kasan_strings();
> + kasan_bitops();
>
> kasan_restore_multi_shot(multishot);
>
> --
> 2.22.0.rc1.257.g3120a18244-goog
>
^ permalink raw reply
* Re: [PATCH] ftrace: add simple oneshot function tracer
From: Steven Rostedt @ 2019-05-29 14:45 UTC (permalink / raw)
To: Thomas Preisner; +Cc: Ingo Molnar, Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <20190529093124.2872-1-linux@tpreisner.de>
On Wed, 29 May 2019 11:31:23 +0200
Thomas Preisner <linux@tpreisner.de> wrote:
> The "oneshot" tracer records every address (ip, parent_ip) exactly once.
> As a result, "oneshot" can be used to efficiently create kernel function
> coverage/usage reports such as in undertaker-tailor[0].
>
> In order to provide this functionality, "oneshot" uses a
> configurable hashset for blacklisting already recorded addresses. This
> way, no user space application is required to parse the function
> tracer's output and to deactivate functions after they have been
> recorded once. Additionally, the tracer's output is reduced to a bare
> mininum so that it can be passed directly to undertaker-tailor.
>
> Further information regarding this oneshot function tracer can also be
> found at [1].
>
> [0]: https://undertaker.cs.fau.de
> [1]: https://tpreisner.de/pub/ba-thesis.pdf
>
> Signed-off-by: Thomas Preisner <linux@tpreisner.de>
>
Hi,
If you are only interested in seeing what functions are called (and
don't care about the order), why not just make another function
profiler (see register_ftrace_profiler and friends)? Then you could
just list the hash table entries instead of having to record into the
ftrace ring buffer.
-- Steve
^ permalink raw reply
* Re: [PATCH 2/3] x86: Move CPU feature test out of uaccess region
From: hpa @ 2019-05-29 14:29 UTC (permalink / raw)
To: Marco Elver, peterz, aryabinin, dvyukov, glider, andreyknvl,
mark.rutland
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev
In-Reply-To: <20190529141500.193390-3-elver@google.com>
On May 29, 2019 7:15:00 AM PDT, Marco Elver <elver@google.com> wrote:
>This patch is a pre-requisite for enabling KASAN bitops
>instrumentation:
>moves boot_cpu_has feature test out of the uaccess region, as
>boot_cpu_has uses test_bit. With instrumentation, the KASAN check would
>otherwise be flagged by objtool.
>
>This approach is preferred over adding the explicit kasan_check_*
>functions to the uaccess whitelist of objtool, as the case here appears
>to be the only one.
>
>Signed-off-by: Marco Elver <elver@google.com>
>---
>v1:
>* This patch replaces patch: 'tools/objtool: add kasan_check_* to
> uaccess whitelist'
>---
> arch/x86/ia32/ia32_signal.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
>index 629d1ee05599..12264e3c9c43 100644
>--- a/arch/x86/ia32/ia32_signal.c
>+++ b/arch/x86/ia32/ia32_signal.c
>@@ -333,6 +333,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal
>*ksig,
> void __user *restorer;
> int err = 0;
> void __user *fpstate = NULL;
>+ bool has_xsave;
>
> /* __copy_to_user optimizes that into a single 8 byte store */
> static const struct {
>@@ -352,13 +353,19 @@ int ia32_setup_rt_frame(int sig, struct ksignal
>*ksig,
> if (!access_ok(frame, sizeof(*frame)))
> return -EFAULT;
>
>+ /*
>+ * Move non-uaccess accesses out of uaccess region if not strictly
>+ * required; this also helps avoid objtool flagging these accesses
>with
>+ * instrumentation enabled.
>+ */
>+ has_xsave = boot_cpu_has(X86_FEATURE_XSAVE);
> put_user_try {
> put_user_ex(sig, &frame->sig);
> put_user_ex(ptr_to_compat(&frame->info), &frame->pinfo);
> put_user_ex(ptr_to_compat(&frame->uc), &frame->puc);
>
> /* Create the ucontext. */
>- if (boot_cpu_has(X86_FEATURE_XSAVE))
>+ if (has_xsave)
> put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
> else
> put_user_ex(0, &frame->uc.uc_flags);
This was meant to use static_cpu_has(). Why did that get dropped?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* [PATCH v2 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Marco Elver @ 2019-05-29 14:15 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland
Cc: corbet, tglx, mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190529141500.193390-1-elver@google.com>
This adds a new header to asm-generic to allow optionally instrumenting
architecture-specific asm implementations of bitops.
This change includes the required change for x86 as reference and
changes the kernel API doc to point to bitops-instrumented.h instead.
Rationale: the functions in x86's bitops.h are no longer the kernel API
functions, but instead the arch_ prefixed functions, which are then
instrumented via bitops-instrumented.h.
Other architectures can similarly add support for asm implementations of
bitops.
The documentation text has been copied/moved, and *no* changes to it
have been made in this patch.
Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
Signed-off-by: Marco Elver <elver@google.com>
---
Changes in v2:
* Instrument word-sized accesses, as specified by the interface.
---
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/include/asm/bitops.h | 210 ++++----------
include/asm-generic/bitops-instrumented.h | 317 ++++++++++++++++++++++
3 files changed, 370 insertions(+), 159 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index a29c99d13331..65266fa1b706 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -51,7 +51,7 @@ The Linux kernel provides more basic utility functions.
Bit Operations
--------------
-.. kernel-doc:: arch/x86/include/asm/bitops.h
+.. kernel-doc:: include/asm-generic/bitops-instrumented.h
:internal:
Bitmap Operations
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 8e790ec219a5..8ebf7af9a0f4 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -49,23 +49,8 @@
#define CONST_MASK_ADDR(nr, addr) WBYTE_ADDR((void *)(addr) + ((nr)>>3))
#define CONST_MASK(nr) (1 << ((nr) & 7))
-/**
- * set_bit - Atomically set a bit in memory
- * @nr: the bit to set
- * @addr: the address to start counting from
- *
- * This function is atomic and may not be reordered. See __set_bit()
- * if you do not require the atomic guarantees.
- *
- * Note: there are no guarantees that this function will not be reordered
- * on non x86 architectures, so if you are writing portable code,
- * make sure not to rely on its reordering guarantees.
- *
- * Note that @nr may be almost arbitrarily large; this function is not
- * restricted to acting on a single-word quantity.
- */
static __always_inline void
-set_bit(long nr, volatile unsigned long *addr)
+arch_set_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "orb %1,%0"
@@ -77,33 +62,17 @@ set_bit(long nr, volatile unsigned long *addr)
: : RLONG_ADDR(addr), "Ir" (nr) : "memory");
}
}
+#define arch_set_bit arch_set_bit
-/**
- * __set_bit - Set a bit in memory
- * @nr: the bit to set
- * @addr: the address to start counting from
- *
- * Unlike set_bit(), this function is non-atomic and may be reordered.
- * If it's called on the same region of memory simultaneously, the effect
- * may be that only one operation succeeds.
- */
-static __always_inline void __set_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___set_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(bts) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
+#define arch___set_bit arch___set_bit
-/**
- * clear_bit - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * clear_bit() is atomic and may not be reordered. However, it does
- * not contain a memory barrier, so if it is used for locking purposes,
- * you should call smp_mb__before_atomic() and/or smp_mb__after_atomic()
- * in order to ensure changes are visible on other processors.
- */
static __always_inline void
-clear_bit(long nr, volatile unsigned long *addr)
+arch_clear_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "andb %1,%0"
@@ -114,27 +83,25 @@ clear_bit(long nr, volatile unsigned long *addr)
: : RLONG_ADDR(addr), "Ir" (nr) : "memory");
}
}
+#define arch_clear_bit arch_clear_bit
-/*
- * clear_bit_unlock - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * clear_bit() is atomic and implies release semantics before the memory
- * operation. It can be used for an unlock.
- */
-static __always_inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch_clear_bit_unlock(long nr, volatile unsigned long *addr)
{
barrier();
- clear_bit(nr, addr);
+ arch_clear_bit(nr, addr);
}
+#define arch_clear_bit_unlock arch_clear_bit_unlock
-static __always_inline void __clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___clear_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(btr) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
+#define arch___clear_bit arch___clear_bit
-static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
{
bool negative;
asm volatile(LOCK_PREFIX "andb %2,%1"
@@ -143,48 +110,25 @@ static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile
: "ir" ((char) ~(1 << nr)) : "memory");
return negative;
}
+#define arch_clear_bit_unlock_is_negative_byte \
+ arch_clear_bit_unlock_is_negative_byte
-// Let everybody know we have it
-#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
-
-/*
- * __clear_bit_unlock - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * __clear_bit() is non-atomic and implies release semantics before the memory
- * operation. It can be used for an unlock if no other CPUs can concurrently
- * modify other bits in the word.
- */
-static __always_inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___clear_bit_unlock(long nr, volatile unsigned long *addr)
{
- __clear_bit(nr, addr);
+ arch___clear_bit(nr, addr);
}
+#define arch___clear_bit_unlock arch___clear_bit_unlock
-/**
- * __change_bit - Toggle a bit in memory
- * @nr: the bit to change
- * @addr: the address to start counting from
- *
- * Unlike change_bit(), this function is non-atomic and may be reordered.
- * If it's called on the same region of memory simultaneously, the effect
- * may be that only one operation succeeds.
- */
-static __always_inline void __change_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___change_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(btc) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
+#define arch___change_bit arch___change_bit
-/**
- * change_bit - Toggle a bit in memory
- * @nr: Bit to change
- * @addr: Address to start counting from
- *
- * change_bit() is atomic and may not be reordered.
- * Note that @nr may be almost arbitrarily large; this function is not
- * restricted to acting on a single-word quantity.
- */
-static __always_inline void change_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch_change_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "xorb %1,%0"
@@ -195,43 +139,24 @@ static __always_inline void change_bit(long nr, volatile unsigned long *addr)
: : RLONG_ADDR(addr), "Ir" (nr) : "memory");
}
}
+#define arch_change_bit arch_change_bit
-/**
- * test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_set_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, c, "Ir", nr);
}
+#define arch_test_and_set_bit arch_test_and_set_bit
-/**
- * test_and_set_bit_lock - Set a bit and return its old value for lock
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This is the same as test_and_set_bit on x86.
- */
static __always_inline bool
-test_and_set_bit_lock(long nr, volatile unsigned long *addr)
+arch_test_and_set_bit_lock(long nr, volatile unsigned long *addr)
{
- return test_and_set_bit(nr, addr);
+ return arch_test_and_set_bit(nr, addr);
}
+#define arch_test_and_set_bit_lock arch_test_and_set_bit_lock
-/**
- * __test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- */
-static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_set_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -241,37 +166,17 @@ static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *
: ADDR, "Ir" (nr) : "memory");
return oldbit;
}
+#define arch___test_and_set_bit arch___test_and_set_bit
-/**
- * test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_clear_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), *addr, c, "Ir", nr);
}
+#define arch_test_and_clear_bit arch_test_and_clear_bit
-/**
- * __test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- *
- * Note: the operation is performed atomically with respect to
- * the local CPU, but not other CPUs. Portable code should not
- * rely on this behaviour.
- * KVM relies on this behaviour on x86 for modifying memory that is also
- * accessed from a hypervisor on the same CPU if running in a VM: don't change
- * this without also updating arch/x86/kernel/kvm.c
- */
-static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_clear_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -281,9 +186,10 @@ static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long
: ADDR, "Ir" (nr) : "memory");
return oldbit;
}
+#define arch___test_and_clear_bit arch___test_and_clear_bit
-/* WARNING: non atomic and it can be reordered! */
-static __always_inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_change_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -294,19 +200,14 @@ static __always_inline bool __test_and_change_bit(long nr, volatile unsigned lon
return oldbit;
}
+#define arch___test_and_change_bit arch___test_and_change_bit
-/**
- * test_and_change_bit - Change a bit and return its old value
- * @nr: Bit to change
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_change_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr);
}
+#define arch_test_and_change_bit arch_test_and_change_bit
static __always_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
{
@@ -326,16 +227,7 @@ static __always_inline bool variable_test_bit(long nr, volatile const unsigned l
return oldbit;
}
-#if 0 /* Fool kernel-doc since it doesn't do macros yet */
-/**
- * test_bit - Determine whether a bit is set
- * @nr: bit number to test
- * @addr: Address to start counting from
- */
-static bool test_bit(int nr, const volatile unsigned long *addr);
-#endif
-
-#define test_bit(nr, addr) \
+#define arch_test_bit(nr, addr) \
(__builtin_constant_p((nr)) \
? constant_test_bit((nr), (addr)) \
: variable_test_bit((nr), (addr)))
@@ -504,6 +396,8 @@ static __always_inline int fls64(__u64 x)
#include <asm-generic/bitops/const_hweight.h>
+#include <asm-generic/bitops-instrumented.h>
+
#include <asm-generic/bitops/le.h>
#include <asm-generic/bitops/ext2-atomic-setbit.h>
diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
new file mode 100644
index 000000000000..b01b0dd93964
--- /dev/null
+++ b/include/asm-generic/bitops-instrumented.h
@@ -0,0 +1,317 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This file provides wrappers with sanitizer instrumentation for bit
+ * operations.
+ *
+ * To use this functionality, an arch's bitops.h file needs to define each of
+ * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
+ * arch___set_bit(), etc.), #define each provided arch_ function, and include
+ * this file after their definitions. For undefined arch_ functions, it is
+ * assumed that they are provided via asm-generic/bitops, which are implicitly
+ * instrumented.
+ */
+#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
+#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
+
+#include <linux/kasan-checks.h>
+
+#if defined(arch_set_bit)
+/**
+ * set_bit - Atomically set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * This function is atomic and may not be reordered. See __set_bit()
+ * if you do not require the atomic guarantees.
+ *
+ * Note: there are no guarantees that this function will not be reordered
+ * on non x86 architectures, so if you are writing portable code,
+ * make sure not to rely on its reordering guarantees.
+ *
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_set_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___set_bit)
+/**
+ * __set_bit - Set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * Unlike set_bit(), this function is non-atomic and may be reordered.
+ * If it's called on the same region of memory simultaneously, the effect
+ * may be that only one operation succeeds.
+ */
+static inline void __set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___set_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_clear_bit)
+/**
+ * clear_bit - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * clear_bit() is atomic and may not be reordered. However, it does
+ * not contain a memory barrier, so if it is used for locking purposes,
+ * you should call smp_mb__before_atomic() and/or smp_mb__after_atomic()
+ * in order to ensure changes are visible on other processors.
+ */
+static inline void clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_clear_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___clear_bit)
+/**
+ * __clear_bit - Clears a bit in memory
+ * @nr: the bit to clear
+ * @addr: the address to start counting from
+ *
+ * Unlike clear_bit(), this function is non-atomic and may be reordered.
+ * If it's called on the same region of memory simultaneously, the effect
+ * may be that only one operation succeeds.
+ */
+static inline void __clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___clear_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_clear_bit_unlock)
+/**
+ * clear_bit_unlock - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * clear_bit_unlock() is atomic and implies release semantics before the memory
+ * operation. It can be used for an unlock.
+ */
+static inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_clear_bit_unlock(nr, addr);
+}
+#endif
+
+#if defined(arch___clear_bit_unlock)
+/**
+ * __clear_bit_unlock - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * __clear_bit_unlock() is non-atomic and implies release semantics before the
+ * memory operation. It can be used for an unlock if no other CPUs can
+ * concurrently modify other bits in the word.
+ */
+static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___clear_bit_unlock(nr, addr);
+}
+#endif
+
+#if defined(arch_change_bit)
+/**
+ * change_bit - Toggle a bit in memory
+ * @nr: Bit to change
+ * @addr: Address to start counting from
+ *
+ * change_bit() is atomic and may not be reordered.
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_change_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___change_bit)
+/**
+ * __change_bit - Toggle a bit in memory
+ * @nr: the bit to change
+ * @addr: the address to start counting from
+ *
+ * Unlike change_bit(), this function is non-atomic and may be reordered.
+ * If it's called on the same region of memory simultaneously, the effect
+ * may be that only one operation succeeds.
+ */
+static inline void __change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___change_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_test_and_set_bit)
+/**
+ * test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It also implies a memory barrier.
+ */
+static inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_set_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___test_and_set_bit)
+/**
+ * __test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic and can be reordered.
+ * If two examples of this operation race, one can appear to succeed
+ * but actually fail. You must protect multiple accesses with a lock.
+ */
+static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_set_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_test_and_set_bit_lock)
+/**
+ * test_and_set_bit_lock - Set a bit and return its old value, for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is atomic and provides acquire barrier semantics if
+ * the returned value is 0.
+ * It can be used to implement bit locks.
+ */
+static inline bool test_and_set_bit_lock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_set_bit_lock(nr, addr);
+}
+#endif
+
+#if defined(arch_test_and_clear_bit)
+/**
+ * test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It also implies a memory barrier.
+ */
+static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_clear_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___test_and_clear_bit)
+/**
+ * __test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic and can be reordered.
+ * If two examples of this operation race, one can appear to succeed
+ * but actually fail. You must protect multiple accesses with a lock.
+ *
+ * Note: the operation is performed atomically with respect to
+ * the local CPU, but not other CPUs. Portable code should not
+ * rely on this behaviour.
+ * KVM relies on this behaviour on x86 for modifying memory that is also
+ * accessed from a hypervisor on the same CPU if running in a VM: don't change
+ * this without also updating arch/x86/kernel/kvm.c
+ */
+static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_clear_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_test_and_change_bit)
+/**
+ * test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It also implies a memory barrier.
+ */
+static inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_change_bit(nr, addr);
+}
+#endif
+
+#if defined(arch___test_and_change_bit)
+/**
+ * __test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic and can be reordered.
+ * If two examples of this operation race, one can appear to succeed
+ * but actually fail. You must protect multiple accesses with a lock.
+ */
+static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_change_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_test_bit)
+/**
+ * test_bit - Determine whether a bit is set
+ * @nr: bit number to test
+ * @addr: Address to start counting from
+ */
+static inline bool test_bit(long nr, const volatile unsigned long *addr)
+{
+ kasan_check_read(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_bit(nr, addr);
+}
+#endif
+
+#if defined(arch_clear_bit_unlock_is_negative_byte)
+/**
+ * clear_bit_unlock_is_negative_byte - Clear a bit in memory and test if bottom
+ * byte is negative, for unlock.
+ * @nr: the bit to clear
+ * @addr: the address to start counting from
+ *
+ * This is a bit of a one-trick-pony for the filemap code, which clears
+ * PG_locked and tests PG_waiters,
+ */
+static inline bool
+clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_clear_bit_unlock_is_negative_byte(nr, addr);
+}
+/* Let everybody know we have it. */
+#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
+#endif
+
+#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_H */
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* [PATCH 2/3] x86: Move CPU feature test out of uaccess region
From: Marco Elver @ 2019-05-29 14:15 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland
Cc: corbet, tglx, mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190529141500.193390-1-elver@google.com>
This patch is a pre-requisite for enabling KASAN bitops instrumentation:
moves boot_cpu_has feature test out of the uaccess region, as
boot_cpu_has uses test_bit. With instrumentation, the KASAN check would
otherwise be flagged by objtool.
This approach is preferred over adding the explicit kasan_check_*
functions to the uaccess whitelist of objtool, as the case here appears
to be the only one.
Signed-off-by: Marco Elver <elver@google.com>
---
v1:
* This patch replaces patch: 'tools/objtool: add kasan_check_* to
uaccess whitelist'
---
arch/x86/ia32/ia32_signal.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 629d1ee05599..12264e3c9c43 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -333,6 +333,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
void __user *restorer;
int err = 0;
void __user *fpstate = NULL;
+ bool has_xsave;
/* __copy_to_user optimizes that into a single 8 byte store */
static const struct {
@@ -352,13 +353,19 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
if (!access_ok(frame, sizeof(*frame)))
return -EFAULT;
+ /*
+ * Move non-uaccess accesses out of uaccess region if not strictly
+ * required; this also helps avoid objtool flagging these accesses with
+ * instrumentation enabled.
+ */
+ has_xsave = boot_cpu_has(X86_FEATURE_XSAVE);
put_user_try {
put_user_ex(sig, &frame->sig);
put_user_ex(ptr_to_compat(&frame->info), &frame->pinfo);
put_user_ex(ptr_to_compat(&frame->uc), &frame->puc);
/* Create the ucontext. */
- if (boot_cpu_has(X86_FEATURE_XSAVE))
+ if (has_xsave)
put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
else
put_user_ex(0, &frame->uc.uc_flags);
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* [PATCH v2 1/3] lib/test_kasan: Add bitops tests
From: Marco Elver @ 2019-05-29 14:14 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland
Cc: corbet, tglx, mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190529141500.193390-1-elver@google.com>
This adds bitops tests to the test_kasan module. In a follow-up patch,
support for bitops instrumentation will be added.
Signed-off-by: Marco Elver <elver@google.com>
---
Changes in v2:
* Use BITS_PER_LONG.
* Use heap allocated memory for test, as newer compilers (correctly)
warn on OOB stack access.
---
lib/test_kasan.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 72 insertions(+), 3 deletions(-)
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 7de2702621dc..6562df0ca30d 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -11,16 +11,17 @@
#define pr_fmt(fmt) "kasan test: %s " fmt, __func__
+#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/kasan.h>
#include <linux/kernel.h>
-#include <linux/mman.h>
#include <linux/mm.h>
+#include <linux/mman.h>
+#include <linux/module.h>
#include <linux/printk.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/uaccess.h>
-#include <linux/module.h>
-#include <linux/kasan.h>
/*
* Note: test functions are marked noinline so that their names appear in
@@ -623,6 +624,73 @@ static noinline void __init kasan_strings(void)
strnlen(ptr, 1);
}
+static noinline void __init kasan_bitops(void)
+{
+ long *bits = kmalloc(sizeof(long), GFP_KERNEL | __GFP_ZERO);
+ if (!bits)
+ return;
+
+ pr_info("within-bounds in set_bit");
+ set_bit(0, bits);
+
+ pr_info("within-bounds in set_bit");
+ set_bit(BITS_PER_LONG - 1, bits);
+
+ pr_info("out-of-bounds in set_bit\n");
+ set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __set_bit\n");
+ __set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in clear_bit\n");
+ clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __clear_bit\n");
+ __clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in clear_bit_unlock\n");
+ clear_bit_unlock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __clear_bit_unlock\n");
+ __clear_bit_unlock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in change_bit\n");
+ change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __change_bit\n");
+ __change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_and_set_bit\n");
+ test_and_set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __test_and_set_bit\n");
+ __test_and_set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_and_set_bit_lock\n");
+ test_and_set_bit_lock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_and_clear_bit\n");
+ test_and_clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __test_and_clear_bit\n");
+ __test_and_clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_and_change_bit\n");
+ test_and_change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __test_and_change_bit\n");
+ __test_and_change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in test_bit\n");
+ (void)test_bit(BITS_PER_LONG, bits);
+
+#if defined(clear_bit_unlock_is_negative_byte)
+ pr_info("out-of-bounds in clear_bit_unlock_is_negative_byte\n");
+ clear_bit_unlock_is_negative_byte(BITS_PER_LONG, bits);
+#endif
+ kfree(bits);
+}
+
static int __init kmalloc_tests_init(void)
{
/*
@@ -664,6 +732,7 @@ static int __init kmalloc_tests_init(void)
kasan_memchr();
kasan_memcmp();
kasan_strings();
+ kasan_bitops();
kasan_restore_multi_shot(multishot);
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply related
* [PATCH v2 0/3] Bitops instrumentation for KASAN
From: Marco Elver @ 2019-05-29 14:14 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland
Cc: corbet, tglx, mingo, bp, hpa, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
The previous version of this patch series and discussion can be found
here: https://lkml.org/lkml/2019/5/28/769
The most significant change is the change of the instrumented access
size to cover the entire word of a bit.
Marco Elver (3):
lib/test_kasan: Add bitops tests
x86: Move CPU feature test out of uaccess region
asm-generic, x86: Add bitops instrumentation for KASAN
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/ia32/ia32_signal.c | 9 +-
arch/x86/include/asm/bitops.h | 210 ++++----------
include/asm-generic/bitops-instrumented.h | 317 ++++++++++++++++++++++
lib/test_kasan.c | 75 ++++-
5 files changed, 450 insertions(+), 163 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
--
2.22.0.rc1.257.g3120a18244-goog
^ permalink raw reply
* Re: [PATCH 6/8] docs/gpu: fix a documentation build break in i915.rst
From: Jonathan Corbet @ 2019-05-29 13:50 UTC (permalink / raw)
To: Daniel Vetter
Cc: Linux Doc Mailing List, Linux Kernel Mailing List, Jani Nikula,
Markus Heiser, Mauro Carvalho Chehab, Oleksandr Natalenko
In-Reply-To: <CAKMK7uFVP6o5jU_cEPshYXwWN39ohybid52yBj567dGBiejzTg@mail.gmail.com>
On Wed, 29 May 2019 08:54:16 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Documentation/gpu/i915.rst is not included in the TOC tree, but newer
> > versions of sphinx parse it anyway. That leads to this hard build failure:
>
> It is included I think: Documentation/gpu/index.rst -> drivers.rst ->
> i915.rst. With that corrected A-b: me.
>
> btw this patch didn't go to intel-gfx and all i915 maintainers, I
> think per get_maintainers.pl it should have. Just asking since I had a
> few patches of my own where get_maintainers.pl didn't seem to do the
> right thing somehow.
It is included, just a level down and I wasn't paying attention.
In any case, this patch needs to be dropped; the kerneldoc comment
changes I sent (and Jani acked) are the better fix for this problem.
Thanks,
jon
^ permalink raw reply
* [PATCH v2 3/3] ima: show rules with IMA_INMASK correctly
From: Roberto Sassu @ 2019-05-29 13:30 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190529133035.28724-1-roberto.sassu@huawei.com>
Show the '^' character when a policy rule has flag IMA_INMASK.
Fixes: 80eae209d63ac ("IMA: allow reading back the current IMA policy")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/ima/ima_policy.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e0cc323f948f..ae4034f041c4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1146,10 +1146,10 @@ enum {
};
static const char *const mask_tokens[] = {
- "MAY_EXEC",
- "MAY_WRITE",
- "MAY_READ",
- "MAY_APPEND"
+ "^MAY_EXEC",
+ "^MAY_WRITE",
+ "^MAY_READ",
+ "^MAY_APPEND"
};
#define __ima_hook_stringify(str) (#str),
@@ -1209,6 +1209,7 @@ int ima_policy_show(struct seq_file *m, void *v)
struct ima_rule_entry *entry = v;
int i;
char tbuf[64] = {0,};
+ int offset = 0;
rcu_read_lock();
@@ -1232,15 +1233,17 @@ int ima_policy_show(struct seq_file *m, void *v)
if (entry->flags & IMA_FUNC)
policy_func_show(m, entry->func);
- if (entry->flags & IMA_MASK) {
+ if ((entry->flags & IMA_MASK) || (entry->flags & IMA_INMASK)) {
+ if (entry->flags & IMA_MASK)
+ offset = 1;
if (entry->mask & MAY_EXEC)
- seq_printf(m, pt(Opt_mask), mt(mask_exec));
+ seq_printf(m, pt(Opt_mask), mt(mask_exec) + offset);
if (entry->mask & MAY_WRITE)
- seq_printf(m, pt(Opt_mask), mt(mask_write));
+ seq_printf(m, pt(Opt_mask), mt(mask_write) + offset);
if (entry->mask & MAY_READ)
- seq_printf(m, pt(Opt_mask), mt(mask_read));
+ seq_printf(m, pt(Opt_mask), mt(mask_read) + offset);
if (entry->mask & MAY_APPEND)
- seq_printf(m, pt(Opt_mask), mt(mask_append));
+ seq_printf(m, pt(Opt_mask), mt(mask_append) + offset);
seq_puts(m, " ");
}
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/3] ima: don't ignore INTEGRITY_UNKNOWN EVM status
From: Roberto Sassu @ 2019-05-29 13:30 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190529133035.28724-1-roberto.sassu@huawei.com>
Currently, ima_appraise_measurement() ignores the EVM status when
evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid
security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG,
ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM
status. The problem is that the EVM status is overwritten with the
appraisal status.
This patch mitigates the issue by selecting signature verification as the
only method allowed for appraisal when EVM is not initialized. Since the
new behavior might break user space, it must be turned on by adding the
'-evm' suffix to the value of the ima_appraise= kernel option.
Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
Documentation/admin-guide/kernel-parameters.txt | 3 ++-
security/integrity/ima/ima_appraise.c | 8 ++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f6664b2e2..d84a2e612b93 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1585,7 +1585,8 @@
Set number of hash buckets for inode cache.
ima_appraise= [IMA] appraise integrity measurements
- Format: { "off" | "enforce" | "fix" | "log" }
+ Format: { "off" | "enforce" | "fix" | "log" |
+ "enforce-evm" | "log-evm" }
default: "enforce"
ima_appraise_tcb [IMA] Deprecated. Use ima_policy= instead.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5fb7127bbe68..afef06e10fb9 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -18,6 +18,7 @@
#include "ima.h"
+static bool ima_appraise_req_evm __ro_after_init;
static int __init default_appraise_setup(char *str)
{
#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
@@ -28,6 +29,9 @@ static int __init default_appraise_setup(char *str)
else if (strncmp(str, "fix", 3) == 0)
ima_appraise = IMA_APPRAISE_FIX;
#endif
+ if (strcmp(str, "enforce-evm") == 0 ||
+ strcmp(str, "log-evm") == 0)
+ ima_appraise_req_evm = true;
return 1;
}
@@ -245,7 +249,11 @@ int ima_appraise_measurement(enum ima_hooks func,
switch (status) {
case INTEGRITY_PASS:
case INTEGRITY_PASS_IMMUTABLE:
+ break;
case INTEGRITY_UNKNOWN:
+ if (ima_appraise_req_evm &&
+ xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+ goto out;
break;
case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */
case INTEGRITY_NOLABEL: /* No security.evm xattr. */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/3] evm: check hash algorithm passed to init_desc()
From: Roberto Sassu @ 2019-05-29 13:30 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, Roberto Sassu, stable
In-Reply-To: <20190529133035.28724-1-roberto.sassu@huawei.com>
This patch prevents memory access beyond the evm_tfm array by checking the
validity of the index (hash algorithm) passed to init_desc(). The hash
algorithm can be arbitrarily set if the security.ima xattr type is not
EVM_XATTR_HMAC.
Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Cc: stable@vger.kernel.org
---
security/integrity/evm/evm_crypto.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index e11564eb645b..82a38e801ee4 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
tfm = &hmac_tfm;
algo = evm_hmac;
} else {
+ if (hash_algo >= HASH_ALGO__LAST)
+ return ERR_PTR(-EINVAL);
+
tfm = &evm_tfm[hash_algo];
algo = hash_algo_name[hash_algo];
}
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/3] ima/evm fixes for v5.2
From: Roberto Sassu @ 2019-05-29 13:30 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-doc, linux-kernel,
silviu.vlasceanu, Roberto Sassu
Changelog
v1:
- remove patch 2/4 (evm: reset status in evm_inode_post_setattr()); file
attributes cannot be set if the signature is portable and immutable
- patch 3/4: add __ro_after_init to ima_appraise_req_evm variable
declaration
- patch 3/4: remove ima_appraise_req_evm kernel option and introduce
'enforce-evm' and 'log-evm' as possible values for ima_appraise=
- remove patch 4/4 (ima: only audit failed appraisal verifications)
- add new patch (ima: show rules with IMA_INMASK correctly)
Roberto Sassu (3):
evm: check hash algorithm passed to init_desc()
ima: don't ignore INTEGRITY_UNKNOWN EVM status
ima: show rules with IMA_INMASK correctly
.../admin-guide/kernel-parameters.txt | 3 ++-
security/integrity/evm/evm_crypto.c | 3 +++
security/integrity/ima/ima_appraise.c | 8 +++++++
security/integrity/ima/ima_policy.c | 21 +++++++++++--------
4 files changed, 25 insertions(+), 10 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Mark Rutland @ 2019-05-29 13:26 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Peter Zijlstra, Marco Elver, Andrey Ryabinin, Alexander Potapenko,
Andrey Konovalov, Jonathan Corbet, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers,
Arnd Bergmann, Josh Poimboeuf, open list:DOCUMENTATION, LKML,
linux-arch, kasan-dev
In-Reply-To: <CACT4Y+aVB3jK_M0-2D_QTq=nncVXTsNp77kjSwBwjqn-3hAJmA@mail.gmail.com>
On Wed, May 29, 2019 at 12:57:15PM +0200, Dmitry Vyukov wrote:
> On Wed, May 29, 2019 at 12:30 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, May 29, 2019 at 12:16:31PM +0200, Marco Elver wrote:
> > > On Wed, 29 May 2019 at 12:01, Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > On Wed, May 29, 2019 at 11:20:17AM +0200, Marco Elver wrote:
> > > > > For the default, we decided to err on the conservative side for now,
> > > > > since it seems that e.g. x86 operates only on the byte the bit is on.
> > > >
> > > > This is not correct, see for instance set_bit():
> > > >
> > > > static __always_inline void
> > > > set_bit(long nr, volatile unsigned long *addr)
> > > > {
> > > > if (IS_IMMEDIATE(nr)) {
> > > > asm volatile(LOCK_PREFIX "orb %1,%0"
> > > > : CONST_MASK_ADDR(nr, addr)
> > > > : "iq" ((u8)CONST_MASK(nr))
> > > > : "memory");
> > > > } else {
> > > > asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0"
> > > > : : RLONG_ADDR(addr), "Ir" (nr) : "memory");
> > > > }
> > > > }
> > > >
> > > > That results in:
> > > >
> > > > LOCK BTSQ nr, (addr)
> > > >
> > > > when @nr is not an immediate.
> > >
> > > Thanks for the clarification. Given that arm64 already instruments
> > > bitops access to whole words, and x86 may also do so for some bitops,
> > > it seems fine to instrument word-sized accesses by default. Is that
> > > reasonable?
> >
> > Eminently -- the API is defined such; for bonus points KASAN should also
> > do alignment checks on atomic ops. Future hardware will #AC on unaligned
> > [*] LOCK prefix instructions.
> >
> > (*) not entirely accurate, it will only trap when crossing a line.
> > https://lkml.kernel.org/r/1556134382-58814-1-git-send-email-fenghua.yu@intel.com
>
> Interesting. Does an address passed to bitops also should be aligned,
> or alignment is supposed to be handled by bitops themselves?
>
> This probably should be done as a separate config as not related to
> KASAN per se. But obviously via the same
> {atomicops,bitops}-instrumented.h hooks which will make it
> significantly easier.
Makes sense to me -- that should be easy to hack into gen_param_check()
in gen-atomic-instrumented.sh, something like:
----
diff --git a/scripts/atomic/gen-atomic-instrumented.sh b/scripts/atomic/gen-atomic-instrumented.sh
index e09812372b17..2f6b8f521e57 100755
--- a/scripts/atomic/gen-atomic-instrumented.sh
+++ b/scripts/atomic/gen-atomic-instrumented.sh
@@ -21,6 +21,13 @@ gen_param_check()
[ ${type#c} != ${type} ] && rw="read"
printf "\tkasan_check_${rw}(${name}, sizeof(*${name}));\n"
+
+ [ "${type#c}" = "v" ] || return
+
+cat <<EOF
+ if (IS_ENABLED(CONFIG_PETERZ))
+ WARN_ON(!IS_ALIGNED(${name}, sizeof(*${name})));
+EOF
}
#gen_param_check(arg...)
----
On arm64 our atomic instructions always perform an alignment check, so
we'd only miss if an atomic op bailed out after a plain READ_ONCE() of
an unaligned atomic variable.
Thanks,
Mark.
^ permalink raw reply related
* Re: [PATCH 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Peter Zijlstra @ 2019-05-29 12:01 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrey Ryabinin, Marco Elver, Mark Rutland, Alexander Potapenko,
Andrey Konovalov, Jonathan Corbet, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers,
Arnd Bergmann, Josh Poimboeuf, open list:DOCUMENTATION, LKML,
linux-arch, kasan-dev
In-Reply-To: <CACT4Y+ZDmqqM6YW72Q-=kAurta5ctscLT5p=nQJ5y=82yVMq=w@mail.gmail.com>
On Wed, May 29, 2019 at 01:29:51PM +0200, Dmitry Vyukov wrote:
> Thanks. I've filed https://bugzilla.kernel.org/show_bug.cgi?id=203751
> for checking alignment with all the points and references, so that
> it's not lost.
Thanks!
^ permalink raw reply
* Re: [PATCH 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Peter Zijlstra @ 2019-05-29 12:01 UTC (permalink / raw)
To: David Laight
Cc: 'Dmitry Vyukov', Marco Elver, Mark Rutland,
Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
Jonathan Corbet, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, the arch/x86 maintainers, Arnd Bergmann,
Josh Poimboeuf, open list:DOCUMENTATION, LKML, linux-arch,
kasan-dev
In-Reply-To: <a0157a8d778a48b7ba3935f3e6840d30@AcuMS.aculab.com>
On Wed, May 29, 2019 at 11:20:56AM +0000, David Laight wrote:
> From: Dmitry Vyukov
> > Sent: 29 May 2019 11:57
> > Interesting. Does an address passed to bitops also should be aligned,
> > or alignment is supposed to be handled by bitops themselves?
>
> The bitops are defined on 'long []' and it is expected to be aligned.
> Any code that casts the argument is likely to be broken on big-endian.
> I did a quick grep a few weeks ago and found some very dubious code.
> Not all the casts seemed to be on code that was LE only (although
> I didn't try to find out what the casts were from).
>
> The alignment trap on x86 could be avoided by only ever requesting 32bit
> cycles - and assuming the buffer is always 32bit aligned (eg int []).
> But on BE passing an 'int []' is just so wrong ....
Right, but as argued elsewhere, I feel we should clean up the dubious
code instead of enabling it.
^ permalink raw reply
* Re: [PATCH 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Dmitry Vyukov @ 2019-05-29 11:29 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Peter Zijlstra, Marco Elver, Mark Rutland, Alexander Potapenko,
Andrey Konovalov, Jonathan Corbet, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers,
Arnd Bergmann, Josh Poimboeuf, open list:DOCUMENTATION, LKML,
linux-arch, kasan-dev
In-Reply-To: <377465ba-3b31-31e7-0f9d-e0a5ab911ca4@virtuozzo.com>
On Wed, May 29, 2019 at 1:23 PM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> On 5/29/19 1:57 PM, Dmitry Vyukov wrote:
> > On Wed, May 29, 2019 at 12:30 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >>
> >> On Wed, May 29, 2019 at 12:16:31PM +0200, Marco Elver wrote:
> >>> On Wed, 29 May 2019 at 12:01, Peter Zijlstra <peterz@infradead.org> wrote:
> >>>>
> >>>> On Wed, May 29, 2019 at 11:20:17AM +0200, Marco Elver wrote:
> >>>>> For the default, we decided to err on the conservative side for now,
> >>>>> since it seems that e.g. x86 operates only on the byte the bit is on.
> >>>>
> >>>> This is not correct, see for instance set_bit():
> >>>>
> >>>> static __always_inline void
> >>>> set_bit(long nr, volatile unsigned long *addr)
> >>>> {
> >>>> if (IS_IMMEDIATE(nr)) {
> >>>> asm volatile(LOCK_PREFIX "orb %1,%0"
> >>>> : CONST_MASK_ADDR(nr, addr)
> >>>> : "iq" ((u8)CONST_MASK(nr))
> >>>> : "memory");
> >>>> } else {
> >>>> asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0"
> >>>> : : RLONG_ADDR(addr), "Ir" (nr) : "memory");
> >>>> }
> >>>> }
> >>>>
> >>>> That results in:
> >>>>
> >>>> LOCK BTSQ nr, (addr)
> >>>>
> >>>> when @nr is not an immediate.
> >>>
> >>> Thanks for the clarification. Given that arm64 already instruments
> >>> bitops access to whole words, and x86 may also do so for some bitops,
> >>> it seems fine to instrument word-sized accesses by default. Is that
> >>> reasonable?
> >>
> >> Eminently -- the API is defined such; for bonus points KASAN should also
> >> do alignment checks on atomic ops. Future hardware will #AC on unaligned
> >> [*] LOCK prefix instructions.
> >>
> >> (*) not entirely accurate, it will only trap when crossing a line.
> >> https://lkml.kernel.org/r/1556134382-58814-1-git-send-email-fenghua.yu@intel.com
> >
> > Interesting. Does an address passed to bitops also should be aligned,
> > or alignment is supposed to be handled by bitops themselves?
> >
>
> It should be aligned. This even documented in Documentation/core-api/atomic_ops.rst:
>
> Native atomic bit operations are defined to operate on objects aligned
> to the size of an "unsigned long" C data type, and are least of that
> size. The endianness of the bits within each "unsigned long" are the
> native endianness of the cpu.
>
>
> > This probably should be done as a separate config as not related to
> > KASAN per se. But obviously via the same
> > {atomicops,bitops}-instrumented.h hooks which will make it
> > significantly easier.
> >
>
> Agreed.
Thanks. I've filed https://bugzilla.kernel.org/show_bug.cgi?id=203751
for checking alignment with all the points and references, so that
it's not lost.
^ permalink raw reply
* Re: [PATCH 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Andrey Ryabinin @ 2019-05-29 11:23 UTC (permalink / raw)
To: Dmitry Vyukov, Peter Zijlstra
Cc: Marco Elver, Mark Rutland, Alexander Potapenko, Andrey Konovalov,
Jonathan Corbet, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, the arch/x86 maintainers, Arnd Bergmann,
Josh Poimboeuf, open list:DOCUMENTATION, LKML, linux-arch,
kasan-dev
In-Reply-To: <CACT4Y+aVB3jK_M0-2D_QTq=nncVXTsNp77kjSwBwjqn-3hAJmA@mail.gmail.com>
On 5/29/19 1:57 PM, Dmitry Vyukov wrote:
> On Wed, May 29, 2019 at 12:30 PM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Wed, May 29, 2019 at 12:16:31PM +0200, Marco Elver wrote:
>>> On Wed, 29 May 2019 at 12:01, Peter Zijlstra <peterz@infradead.org> wrote:
>>>>
>>>> On Wed, May 29, 2019 at 11:20:17AM +0200, Marco Elver wrote:
>>>>> For the default, we decided to err on the conservative side for now,
>>>>> since it seems that e.g. x86 operates only on the byte the bit is on.
>>>>
>>>> This is not correct, see for instance set_bit():
>>>>
>>>> static __always_inline void
>>>> set_bit(long nr, volatile unsigned long *addr)
>>>> {
>>>> if (IS_IMMEDIATE(nr)) {
>>>> asm volatile(LOCK_PREFIX "orb %1,%0"
>>>> : CONST_MASK_ADDR(nr, addr)
>>>> : "iq" ((u8)CONST_MASK(nr))
>>>> : "memory");
>>>> } else {
>>>> asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0"
>>>> : : RLONG_ADDR(addr), "Ir" (nr) : "memory");
>>>> }
>>>> }
>>>>
>>>> That results in:
>>>>
>>>> LOCK BTSQ nr, (addr)
>>>>
>>>> when @nr is not an immediate.
>>>
>>> Thanks for the clarification. Given that arm64 already instruments
>>> bitops access to whole words, and x86 may also do so for some bitops,
>>> it seems fine to instrument word-sized accesses by default. Is that
>>> reasonable?
>>
>> Eminently -- the API is defined such; for bonus points KASAN should also
>> do alignment checks on atomic ops. Future hardware will #AC on unaligned
>> [*] LOCK prefix instructions.
>>
>> (*) not entirely accurate, it will only trap when crossing a line.
>> https://lkml.kernel.org/r/1556134382-58814-1-git-send-email-fenghua.yu@intel.com
>
> Interesting. Does an address passed to bitops also should be aligned,
> or alignment is supposed to be handled by bitops themselves?
>
It should be aligned. This even documented in Documentation/core-api/atomic_ops.rst:
Native atomic bit operations are defined to operate on objects aligned
to the size of an "unsigned long" C data type, and are least of that
size. The endianness of the bits within each "unsigned long" are the
native endianness of the cpu.
> This probably should be done as a separate config as not related to
> KASAN per se. But obviously via the same
> {atomicops,bitops}-instrumented.h hooks which will make it
> significantly easier.
>
Agreed.
^ permalink raw reply
* RE: [PATCH 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: David Laight @ 2019-05-29 11:20 UTC (permalink / raw)
To: 'Dmitry Vyukov', Peter Zijlstra
Cc: Marco Elver, Mark Rutland, Andrey Ryabinin, Alexander Potapenko,
Andrey Konovalov, Jonathan Corbet, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers,
Arnd Bergmann, Josh Poimboeuf, open list:DOCUMENTATION, LKML,
linux-arch, kasan-dev
In-Reply-To: <CACT4Y+aVB3jK_M0-2D_QTq=nncVXTsNp77kjSwBwjqn-3hAJmA@mail.gmail.com>
From: Dmitry Vyukov
> Sent: 29 May 2019 11:57
> On Wed, May 29, 2019 at 12:30 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, May 29, 2019 at 12:16:31PM +0200, Marco Elver wrote:
> > > On Wed, 29 May 2019 at 12:01, Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > On Wed, May 29, 2019 at 11:20:17AM +0200, Marco Elver wrote:
> > > > > For the default, we decided to err on the conservative side for now,
> > > > > since it seems that e.g. x86 operates only on the byte the bit is on.
> > > >
> > > > This is not correct, see for instance set_bit():
> > > >
> > > > static __always_inline void
> > > > set_bit(long nr, volatile unsigned long *addr)
> > > > {
> > > > if (IS_IMMEDIATE(nr)) {
> > > > asm volatile(LOCK_PREFIX "orb %1,%0"
> > > > : CONST_MASK_ADDR(nr, addr)
> > > > : "iq" ((u8)CONST_MASK(nr))
> > > > : "memory");
> > > > } else {
> > > > asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0"
> > > > : : RLONG_ADDR(addr), "Ir" (nr) : "memory");
> > > > }
> > > > }
> > > >
> > > > That results in:
> > > >
> > > > LOCK BTSQ nr, (addr)
> > > >
> > > > when @nr is not an immediate.
> > >
> > > Thanks for the clarification. Given that arm64 already instruments
> > > bitops access to whole words, and x86 may also do so for some bitops,
> > > it seems fine to instrument word-sized accesses by default. Is that
> > > reasonable?
> >
> > Eminently -- the API is defined such; for bonus points KASAN should also
> > do alignment checks on atomic ops. Future hardware will #AC on unaligned
> > [*] LOCK prefix instructions.
> >
> > (*) not entirely accurate, it will only trap when crossing a line.
> > https://lkml.kernel.org/r/1556134382-58814-1-git-send-email-fenghua.yu@intel.com
>
> Interesting. Does an address passed to bitops also should be aligned,
> or alignment is supposed to be handled by bitops themselves?
The bitops are defined on 'long []' and it is expected to be aligned.
Any code that casts the argument is likely to be broken on big-endian.
I did a quick grep a few weeks ago and found some very dubious code.
Not all the casts seemed to be on code that was LE only (although
I didn't try to find out what the casts were from).
The alignment trap on x86 could be avoided by only ever requesting 32bit
cycles - and assuming the buffer is always 32bit aligned (eg int []).
But on BE passing an 'int []' is just so wrong ....
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Dmitry Vyukov @ 2019-05-29 10:57 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Marco Elver, Mark Rutland, Andrey Ryabinin, Alexander Potapenko,
Andrey Konovalov, Jonathan Corbet, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, the arch/x86 maintainers,
Arnd Bergmann, Josh Poimboeuf, open list:DOCUMENTATION, LKML,
linux-arch, kasan-dev
In-Reply-To: <20190529103010.GP2623@hirez.programming.kicks-ass.net>
On Wed, May 29, 2019 at 12:30 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, May 29, 2019 at 12:16:31PM +0200, Marco Elver wrote:
> > On Wed, 29 May 2019 at 12:01, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Wed, May 29, 2019 at 11:20:17AM +0200, Marco Elver wrote:
> > > > For the default, we decided to err on the conservative side for now,
> > > > since it seems that e.g. x86 operates only on the byte the bit is on.
> > >
> > > This is not correct, see for instance set_bit():
> > >
> > > static __always_inline void
> > > set_bit(long nr, volatile unsigned long *addr)
> > > {
> > > if (IS_IMMEDIATE(nr)) {
> > > asm volatile(LOCK_PREFIX "orb %1,%0"
> > > : CONST_MASK_ADDR(nr, addr)
> > > : "iq" ((u8)CONST_MASK(nr))
> > > : "memory");
> > > } else {
> > > asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0"
> > > : : RLONG_ADDR(addr), "Ir" (nr) : "memory");
> > > }
> > > }
> > >
> > > That results in:
> > >
> > > LOCK BTSQ nr, (addr)
> > >
> > > when @nr is not an immediate.
> >
> > Thanks for the clarification. Given that arm64 already instruments
> > bitops access to whole words, and x86 may also do so for some bitops,
> > it seems fine to instrument word-sized accesses by default. Is that
> > reasonable?
>
> Eminently -- the API is defined such; for bonus points KASAN should also
> do alignment checks on atomic ops. Future hardware will #AC on unaligned
> [*] LOCK prefix instructions.
>
> (*) not entirely accurate, it will only trap when crossing a line.
> https://lkml.kernel.org/r/1556134382-58814-1-git-send-email-fenghua.yu@intel.com
Interesting. Does an address passed to bitops also should be aligned,
or alignment is supposed to be handled by bitops themselves?
This probably should be done as a separate config as not related to
KASAN per se. But obviously via the same
{atomicops,bitops}-instrumented.h hooks which will make it
significantly easier.
^ 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