Enabling HARDENED_USER_COPY causes measurable regressions in the
networking performances, up to 8% under UDP flood.
A generic distro may want to enable HARDENED_USER_COPY in their default
kernel config, but at the same time, such distro may want to be able to
avoid the performance penalties in with the default configuration and
enable the stricter check on a per-boot basis.
This change adds a config variable and a boot parameter to conditionally
enable HARDENED_USER_COPY at boot time, and switch HUC to off if
HUC_DEFAULT_OFF is set.
Signed-off-by: Chris von Recklinghausen <[email protected]>
---
.../admin-guide/kernel-parameters.rst | 2 ++
.../admin-guide/kernel-parameters.txt | 3 ++
include/linux/thread_info.h | 7 +++++
mm/usercopy.c | 28 +++++++++++++++++++
security/Kconfig | 10 +++++++
5 files changed, 50 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index b8d0bc07ed0a..c3035038e3ae 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -100,6 +100,8 @@ parameter is applicable::
FB The frame buffer device is enabled.
FTRACE Function tracing enabled.
GCOV GCOV profiling is enabled.
+ HUC Hardened usercopy is enabled
+ HUCF Hardened usercopy disabled at boot
HW Appropriate hardware is enabled.
IA-64 IA-64 architecture is enabled.
IMA Integrity measurement architecture is enabled.
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index efc7aa7a0670..cd3354bc14d3 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -816,6 +816,9 @@
disable= [IPV6]
See Documentation/networking/ipv6.txt.
+ enable_hardened_usercopy [HUC,HUCF]
+ Enable hardened usercopy checks
+
disable_radix [PPC]
Disable RADIX MMU mode on POWER9
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 8d8821b3689a..140a36cc1c2c 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -109,12 +109,19 @@ static inline int arch_within_stack_frames(const void * const stack,
#endif
#ifdef CONFIG_HARDENED_USERCOPY
+#include <linux/jump_label.h>
+
+DECLARE_STATIC_KEY_FALSE(bypass_usercopy_checks);
+
extern void __check_object_size(const void *ptr, unsigned long n,
bool to_user);
static __always_inline void check_object_size(const void *ptr, unsigned long n,
bool to_user)
{
+ if (static_branch_likely(&bypass_usercopy_checks))
+ return;
+
if (!__builtin_constant_p(n))
__check_object_size(ptr, n, to_user);
}
diff --git a/mm/usercopy.c b/mm/usercopy.c
index e9e9325f7638..ce3996da1b2e 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -279,3 +279,31 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
check_kernel_text_object((const unsigned long)ptr, n, to_user);
}
EXPORT_SYMBOL(__check_object_size);
+
+DEFINE_STATIC_KEY_FALSE(bypass_usercopy_checks);
+EXPORT_SYMBOL(bypass_usercopy_checks);
+
+#ifdef CONFIG_HUC_DEFAULT_OFF
+#define HUC_DEFAULT false
+#else
+#define HUC_DEFAULT true
+#endif
+
+static bool enable_huc_atboot = HUC_DEFAULT;
+
+static int __init parse_enable_usercopy(char *str)
+{
+ enable_huc_atboot = true;
+ return 1;
+}
+
+static int __init set_enable_usercopy(void)
+{
+ if (enable_huc_atboot == false)
+ static_branch_enable(&bypass_usercopy_checks);
+ return 1;
+}
+
+__setup("enable_hardened_usercopy", parse_enable_usercopy);
+
+late_initcall(set_enable_usercopy);
diff --git a/security/Kconfig b/security/Kconfig
index c4302067a3ad..a6173897b85c 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
been removed. This config is intended to be used only while
trying to find such users.
+config HUC_DEFAULT_OFF
+ bool "allow CONFIG_HARDENED_USERCOPY to be configured but disabled"
+ depends on HARDENED_USERCOPY
+ help
+ When CONFIG_HARDENED_USERCOPY is enabled, disable its
+ functionality unless it is enabled via at boot time
+ via the "enable_hardened_usercopy" boot parameter. This allows
+ the functionality of hardened usercopy to be present but not
+ impact performance unless it is needed.
+
config FORTIFY_SOURCE
bool "Harden common str/mem functions against buffer overflows"
depends on ARCH_HAS_FORTIFY_SOURCE
--
2.17.0
Add correct address for linux-mm
On 06/25/2018 11:08 AM, Chris von Recklinghausen wrote:
> Enabling HARDENED_USER_COPY causes measurable regressions in the
> networking performances, up to 8% under UDP flood.
>
> A generic distro may want to enable HARDENED_USER_COPY in their default
> kernel config, but at the same time, such distro may want to be able to
> avoid the performance penalties in with the default configuration and
> enable the stricter check on a per-boot basis.
>
> This change adds a config variable and a boot parameter to conditionally
> enable HARDENED_USER_COPY at boot time, and switch HUC to off if
> HUC_DEFAULT_OFF is set.
>
> Signed-off-by: Chris von Recklinghausen <[email protected]>
> ---
> .../admin-guide/kernel-parameters.rst | 2 ++
> .../admin-guide/kernel-parameters.txt | 3 ++
> include/linux/thread_info.h | 7 +++++
> mm/usercopy.c | 28 +++++++++++++++++++
> security/Kconfig | 10 +++++++
> 5 files changed, 50 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
> index b8d0bc07ed0a..c3035038e3ae 100644
> --- a/Documentation/admin-guide/kernel-parameters.rst
> +++ b/Documentation/admin-guide/kernel-parameters.rst
> @@ -100,6 +100,8 @@ parameter is applicable::
> FB The frame buffer device is enabled.
> FTRACE Function tracing enabled.
> GCOV GCOV profiling is enabled.
> + HUC Hardened usercopy is enabled
> + HUCF Hardened usercopy disabled at boot
> HW Appropriate hardware is enabled.
> IA-64 IA-64 architecture is enabled.
> IMA Integrity measurement architecture is enabled.
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index efc7aa7a0670..cd3354bc14d3 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -816,6 +816,9 @@
> disable= [IPV6]
> See Documentation/networking/ipv6.txt.
>
> + enable_hardened_usercopy [HUC,HUCF]
> + Enable hardened usercopy checks
> +
> disable_radix [PPC]
> Disable RADIX MMU mode on POWER9
>
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 8d8821b3689a..140a36cc1c2c 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -109,12 +109,19 @@ static inline int arch_within_stack_frames(const void * const stack,
> #endif
>
> #ifdef CONFIG_HARDENED_USERCOPY
> +#include <linux/jump_label.h>
> +
> +DECLARE_STATIC_KEY_FALSE(bypass_usercopy_checks);
> +
> extern void __check_object_size(const void *ptr, unsigned long n,
> bool to_user);
>
> static __always_inline void check_object_size(const void *ptr, unsigned long n,
> bool to_user)
> {
> + if (static_branch_likely(&bypass_usercopy_checks))
> + return;
> +
> if (!__builtin_constant_p(n))
> __check_object_size(ptr, n, to_user);
> }
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index e9e9325f7638..ce3996da1b2e 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -279,3 +279,31 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
> check_kernel_text_object((const unsigned long)ptr, n, to_user);
> }
> EXPORT_SYMBOL(__check_object_size);
> +
> +DEFINE_STATIC_KEY_FALSE(bypass_usercopy_checks);
> +EXPORT_SYMBOL(bypass_usercopy_checks);
> +
> +#ifdef CONFIG_HUC_DEFAULT_OFF
> +#define HUC_DEFAULT false
> +#else
> +#define HUC_DEFAULT true
> +#endif
> +
> +static bool enable_huc_atboot = HUC_DEFAULT;
> +
> +static int __init parse_enable_usercopy(char *str)
> +{
> + enable_huc_atboot = true;
> + return 1;
> +}
> +
> +static int __init set_enable_usercopy(void)
> +{
> + if (enable_huc_atboot == false)
> + static_branch_enable(&bypass_usercopy_checks);
> + return 1;
> +}
> +
> +__setup("enable_hardened_usercopy", parse_enable_usercopy);
> +
> +late_initcall(set_enable_usercopy);
> diff --git a/security/Kconfig b/security/Kconfig
> index c4302067a3ad..a6173897b85c 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
> been removed. This config is intended to be used only while
> trying to find such users.
>
> +config HUC_DEFAULT_OFF
> + bool "allow CONFIG_HARDENED_USERCOPY to be configured but disabled"
> + depends on HARDENED_USERCOPY
> + help
> + When CONFIG_HARDENED_USERCOPY is enabled, disable its
> + functionality unless it is enabled via at boot time
> + via the "enable_hardened_usercopy" boot parameter. This allows
> + the functionality of hardened usercopy to be present but not
> + impact performance unless it is needed.
> +
> config FORTIFY_SOURCE
> bool "Harden common str/mem functions against buffer overflows"
> depends on ARCH_HAS_FORTIFY_SOURCE
On 06/25/2018 08:08 AM, Chris von Recklinghausen wrote:
> Enabling HARDENED_USER_COPY causes measurable regressions in the
> networking performances, up to 8% under UDP flood.
>
> A generic distro may want to enable HARDENED_USER_COPY in their default
> kernel config, but at the same time, such distro may want to be able to
> avoid the performance penalties in with the default configuration and
> enable the stricter check on a per-boot basis.
>
> This change adds a config variable and a boot parameter to conditionally
> enable HARDENED_USER_COPY at boot time, and switch HUC to off if
> HUC_DEFAULT_OFF is set.
>
> Signed-off-by: Chris von Recklinghausen <[email protected]>
> ---
> .../admin-guide/kernel-parameters.rst | 2 ++
> .../admin-guide/kernel-parameters.txt | 3 ++
> include/linux/thread_info.h | 7 +++++
> mm/usercopy.c | 28 +++++++++++++++++++
> security/Kconfig | 10 +++++++
> 5 files changed, 50 insertions(+)
>
Hi,
> diff --git a/security/Kconfig b/security/Kconfig
> index c4302067a3ad..a6173897b85c 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
> been removed. This config is intended to be used only while
> trying to find such users.
>
> +config HUC_DEFAULT_OFF
> + bool "allow CONFIG_HARDENED_USERCOPY to be configured but disabled"
> + depends on HARDENED_USERCOPY
> + help
> + When CONFIG_HARDENED_USERCOPY is enabled, disable its
> + functionality unless it is enabled via at boot time
it is enabled at boot time
> + via the "enable_hardened_usercopy" boot parameter. This allows
> + the functionality of hardened usercopy to be present but not
> + impact performance unless it is needed.
> +
> config FORTIFY_SOURCE
> bool "Harden common str/mem functions against buffer overflows"
> depends on ARCH_HAS_FORTIFY_SOURCE
>
--
~Randy
Hi Chris,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-von-Recklinghausen/add-param-that-allows-bootline-control-of-hardened-usercopy/20180625-232835
config: m68k-allyesconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=m68k
All error/warnings (new ones prefixed by >>):
WARNING: unmet direct dependencies detected for PREEMPT_COUNT
Depends on COLDFIRE
Selected by
- DEBUG_ATOMIC_SLEEP && DEBUG_KERNEL
In file included from include/linux/thread_info.h:112:0,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:81,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:5,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
include/linux/jump_label.h: In function 'static_key_count':
>> include/linux/jump_label.h:194:9: error: implicit declaration of function 'atomic_read'; did you mean
return atomic_read(&key->enabled);
^~~~~~~~~~~
__atomic_load
include/linux/jump_label.h: In function 'static_key_slow_inc':
>> include/linux/jump_label.h:221:2: error: implicit declaration of function 'atomic_inc'; did you mean
atomic_inc(&key->enabled);
^~~~~~~~~~
__atomic_load
include/linux/jump_label.h: In function 'static_key_slow_dec':
>> include/linux/jump_label.h:227:2: error: implicit declaration of function 'atomic_dec'; did you mean
atomic_dec(&key->enabled);
^~~~~~~~~~
__atomic_clear
include/linux/jump_label.h: In function 'static_key_enable':
>> include/linux/jump_label.h:254:2: error: implicit declaration of function 'atomic_set'; did you mean
atomic_set(&key->enabled, 1);
^~~~~~~~~~
__atomic_clear
In file included from include/linux/atomic.h:5:0,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
arch/m68k/include/asm/atomic.h: At top level:
>> arch/m68k/include/asm/atomic.h:125:20: warning: conflicting types for 'atomic_inc'
static inline void atomic_inc(atomic_t
^~~~~~~~~~
>> arch/m68k/include/asm/atomic.h:125:20: error: static declaration of 'atomic_inc' follows non-static declaration
In file included from include/linux/thread_info.h:112:0,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:81,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:5,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
include/linux/jump_label.h:221:2: note: previous implicit declaration of 'atomic_inc' was here
atomic_inc(&key->enabled);
^~~~~~~~~~
In file included from include/linux/atomic.h:5:0,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
>> arch/m68k/include/asm/atomic.h:130:20: warning: conflicting types for 'atomic_dec'
static inline void atomic_dec(atomic_t
^~~~~~~~~~
>> arch/m68k/include/asm/atomic.h:130:20: error: static declaration of 'atomic_dec' follows non-static declaration
In file included from include/linux/thread_info.h:112:0,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:81,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:5,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
include/linux/jump_label.h:227:2: note: previous implicit declaration of 'atomic_dec' was here
atomic_dec(&key->enabled);
^~~~~~~~~~
cc1: some warnings being treated as errors
Makefile Module.symvers System.map arch block built-in.a certs crypto drivers firmware fs include init ipc kernel lib mm modules.builtin modules.order net scripts security sound source usr virt vmlinux vmlinux.gz vmlinux.o Error 1
Target '__build' not remade because of errors.
Makefile Module.symvers System.map arch block built-in.a certs crypto drivers firmware fs include init ipc kernel lib mm modules.builtin modules.order net scripts security sound source usr virt vmlinux vmlinux.gz vmlinux.o Error 2
Target 'prepare' not remade because of errors.
make: Makefile Module.symvers System.map arch block built-in.a certs crypto drivers firmware fs include init ipc kernel lib mm modules.builtin modules.order net scripts security sound source usr virt vmlinux vmlinux.gz vmlinux.o Error 2
vim +/atomic_read +194 include/linux/jump_label.h
1f69bf9c6 Jason Baron 2016-08-03 191
4c5ea0a9c Paolo Bonzini 2016-06-21 192 static inline int static_key_count(struct static_key *key)
4c5ea0a9c Paolo Bonzini 2016-06-21 193 {
4c5ea0a9c Paolo Bonzini 2016-06-21 @194 return atomic_read(&key->enabled);
4c5ea0a9c Paolo Bonzini 2016-06-21 195 }
4c5ea0a9c Paolo Bonzini 2016-06-21 196
97ce2c88f Jeremy Fitzhardinge 2011-10-12 197 static __always_inline void jump_label_init(void)
97ce2c88f Jeremy Fitzhardinge 2011-10-12 198 {
c4b2c0c5f Hannes Frederic Sowa 2013-10-19 199 static_key_initialized = true;
97ce2c88f Jeremy Fitzhardinge 2011-10-12 200 }
97ce2c88f Jeremy Fitzhardinge 2011-10-12 201
578ae447e Josh Poimboeuf 2018-03-19 202 static inline void jump_label_invalidate_initmem(void) {}
333522447 Josh Poimboeuf 2018-02-20 203
c5905afb0 Ingo Molnar 2012-02-24 204 static __always_inline bool static_key_false(struct static_key *key)
c5905afb0 Ingo Molnar 2012-02-24 205 {
ea5e9539a Mel Gorman 2014-06-04 206 if (unlikely(static_key_count(key) > 0))
c5905afb0 Ingo Molnar 2012-02-24 207 return true;
c5905afb0 Ingo Molnar 2012-02-24 208 return false;
c5905afb0 Ingo Molnar 2012-02-24 209 }
c5905afb0 Ingo Molnar 2012-02-24 210
c5905afb0 Ingo Molnar 2012-02-24 211 static __always_inline bool static_key_true(struct static_key *key)
d430d3d7e Jason Baron 2011-03-16 212 {
ea5e9539a Mel Gorman 2014-06-04 213 if (likely(static_key_count(key) > 0))
d430d3d7e Jason Baron 2011-03-16 214 return true;
d430d3d7e Jason Baron 2011-03-16 215 return false;
d430d3d7e Jason Baron 2011-03-16 216 }
bf5438fca Jason Baron 2010-09-17 217
c5905afb0 Ingo Molnar 2012-02-24 218 static inline void static_key_slow_inc(struct static_key *key)
d430d3d7e Jason Baron 2011-03-16 219 {
5cdda5117 Borislav Petkov 2017-10-18 220 STATIC_KEY_CHECK_USE(key);
d430d3d7e Jason Baron 2011-03-16 @221 atomic_inc(&key->enabled);
d430d3d7e Jason Baron 2011-03-16 222 }
bf5438fca Jason Baron 2010-09-17 223
c5905afb0 Ingo Molnar 2012-02-24 224 static inline void static_key_slow_dec(struct static_key *key)
bf5438fca Jason Baron 2010-09-17 225 {
5cdda5117 Borislav Petkov 2017-10-18 226 STATIC_KEY_CHECK_USE(key);
d430d3d7e Jason Baron 2011-03-16 @227 atomic_dec(&key->enabled);
bf5438fca Jason Baron 2010-09-17 228 }
bf5438fca Jason Baron 2010-09-17 229
ce48c1464 Peter Zijlstra 2018-01-22 230 #define static_key_slow_inc_cpuslocked(key) static_key_slow_inc(key)
ce48c1464 Peter Zijlstra 2018-01-22 231 #define static_key_slow_dec_cpuslocked(key) static_key_slow_dec(key)
ce48c1464 Peter Zijlstra 2018-01-22 232
4c3ef6d79 Jason Baron 2010-09-17 233 static inline int jump_label_text_reserved(void *start, void *end)
4c3ef6d79 Jason Baron 2010-09-17 234 {
4c3ef6d79 Jason Baron 2010-09-17 235 return 0;
4c3ef6d79 Jason Baron 2010-09-17 236 }
4c3ef6d79 Jason Baron 2010-09-17 237
91bad2f8d Jason Baron 2010-10-01 238 static inline void jump_label_lock(void) {}
91bad2f8d Jason Baron 2010-10-01 239 static inline void jump_label_unlock(void) {}
91bad2f8d Jason Baron 2010-10-01 240
d430d3d7e Jason Baron 2011-03-16 241 static inline int jump_label_apply_nops(struct module *mod)
d430d3d7e Jason Baron 2011-03-16 242 {
d430d3d7e Jason Baron 2011-03-16 243 return 0;
d430d3d7e Jason Baron 2011-03-16 244 }
b20295207 Gleb Natapov 2011-11-27 245
e33886b38 Peter Zijlstra 2015-07-24 246 static inline void static_key_enable(struct static_key *key)
e33886b38 Peter Zijlstra 2015-07-24 247 {
5cdda5117 Borislav Petkov 2017-10-18 248 STATIC_KEY_CHECK_USE(key);
e33886b38 Peter Zijlstra 2015-07-24 249
1dbb6704d Paolo Bonzini 2017-08-01 250 if (atomic_read(&key->enabled) != 0) {
1dbb6704d Paolo Bonzini 2017-08-01 251 WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
1dbb6704d Paolo Bonzini 2017-08-01 252 return;
1dbb6704d Paolo Bonzini 2017-08-01 253 }
1dbb6704d Paolo Bonzini 2017-08-01 @254 atomic_set(&key->enabled, 1);
e33886b38 Peter Zijlstra 2015-07-24 255 }
e33886b38 Peter Zijlstra 2015-07-24 256
:::::: The code at line 194 was first introduced by commit
:::::: 4c5ea0a9cd02d6aa8adc86e100b2a4cff8d614ff locking/static_key: Fix concurrent static_key_slow_inc()
:::::: TO: Paolo Bonzini <[email protected]>
:::::: CC: Ingo Molnar <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Mon, Jun 25, 2018 at 8:08 AM, Chris von Recklinghausen
<[email protected]> wrote:
> Enabling HARDENED_USER_COPY causes measurable regressions in the
> networking performances, up to 8% under UDP flood.
Which function is "hot"? i.e. which copy*user() is taking up the time?
Do you have a workload that at can be used to reproduce the problem?
-Kees
--
Kees Cook
Pixel Security
Hi Chris,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-von-Recklinghausen/add-param-that-allows-bootline-control-of-hardened-usercopy/20180625-232835
config: m68k-multi_defconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=m68k
All errors (new ones prefixed by >>):
In file included from include/linux/thread_info.h:112:0,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:81,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:5,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
include/linux/jump_label.h: In function 'static_key_count':
>> include/linux/jump_label.h:194:9: error: implicit declaration of function 'atomic_read'; did you mean '__atomic_load'? [-Werror=implicit-function-declaration]
return atomic_read(&key->enabled);
^~~~~~~~~~~
__atomic_load
include/linux/jump_label.h: In function 'static_key_slow_inc':
>> include/linux/jump_label.h:221:2: error: implicit declaration of function 'atomic_inc'; did you mean '__atomic_load'? [-Werror=implicit-function-declaration]
atomic_inc(&key->enabled);
^~~~~~~~~~
__atomic_load
include/linux/jump_label.h: In function 'static_key_slow_dec':
>> include/linux/jump_label.h:227:2: error: implicit declaration of function 'atomic_dec'; did you mean '__atomic_clear'? [-Werror=implicit-function-declaration]
atomic_dec(&key->enabled);
^~~~~~~~~~
__atomic_clear
include/linux/jump_label.h: In function 'static_key_enable':
>> include/linux/jump_label.h:254:2: error: implicit declaration of function 'atomic_set'; did you mean '__atomic_clear'? [-Werror=implicit-function-declaration]
atomic_set(&key->enabled, 1);
^~~~~~~~~~
__atomic_clear
In file included from include/linux/atomic.h:5:0,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
arch/m68k/include/asm/atomic.h: At top level:
arch/m68k/include/asm/atomic.h:125:20: warning: conflicting types for 'atomic_inc'
static inline void atomic_inc(atomic_t *v)
^~~~~~~~~~
arch/m68k/include/asm/atomic.h:125:20: error: static declaration of 'atomic_inc' follows non-static declaration
In file included from include/linux/thread_info.h:112:0,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:81,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:5,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
include/linux/jump_label.h:221:2: note: previous implicit declaration of 'atomic_inc' was here
atomic_inc(&key->enabled);
^~~~~~~~~~
In file included from include/linux/atomic.h:5:0,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
arch/m68k/include/asm/atomic.h:130:20: warning: conflicting types for 'atomic_dec'
static inline void atomic_dec(atomic_t *v)
^~~~~~~~~~
arch/m68k/include/asm/atomic.h:130:20: error: static declaration of 'atomic_dec' follows non-static declaration
In file included from include/linux/thread_info.h:112:0,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:81,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:5,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:11,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from arch/m68k/kernel/asm-offsets.c:15:
include/linux/jump_label.h:227:2: note: previous implicit declaration of 'atomic_dec' was here
atomic_dec(&key->enabled);
^~~~~~~~~~
cc1: some warnings being treated as errors
make[2]: *** [arch/m68k/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [sub-make] Error 2
vim +194 include/linux/jump_label.h
1f69bf9c6 Jason Baron 2016-08-03 191
4c5ea0a9c Paolo Bonzini 2016-06-21 192 static inline int static_key_count(struct static_key *key)
4c5ea0a9c Paolo Bonzini 2016-06-21 193 {
4c5ea0a9c Paolo Bonzini 2016-06-21 @194 return atomic_read(&key->enabled);
4c5ea0a9c Paolo Bonzini 2016-06-21 195 }
4c5ea0a9c Paolo Bonzini 2016-06-21 196
97ce2c88f Jeremy Fitzhardinge 2011-10-12 197 static __always_inline void jump_label_init(void)
97ce2c88f Jeremy Fitzhardinge 2011-10-12 198 {
c4b2c0c5f Hannes Frederic Sowa 2013-10-19 199 static_key_initialized = true;
97ce2c88f Jeremy Fitzhardinge 2011-10-12 200 }
97ce2c88f Jeremy Fitzhardinge 2011-10-12 201
578ae447e Josh Poimboeuf 2018-03-19 202 static inline void jump_label_invalidate_initmem(void) {}
333522447 Josh Poimboeuf 2018-02-20 203
c5905afb0 Ingo Molnar 2012-02-24 204 static __always_inline bool static_key_false(struct static_key *key)
c5905afb0 Ingo Molnar 2012-02-24 205 {
ea5e9539a Mel Gorman 2014-06-04 206 if (unlikely(static_key_count(key) > 0))
c5905afb0 Ingo Molnar 2012-02-24 207 return true;
c5905afb0 Ingo Molnar 2012-02-24 208 return false;
c5905afb0 Ingo Molnar 2012-02-24 209 }
c5905afb0 Ingo Molnar 2012-02-24 210
c5905afb0 Ingo Molnar 2012-02-24 211 static __always_inline bool static_key_true(struct static_key *key)
d430d3d7e Jason Baron 2011-03-16 212 {
ea5e9539a Mel Gorman 2014-06-04 213 if (likely(static_key_count(key) > 0))
d430d3d7e Jason Baron 2011-03-16 214 return true;
d430d3d7e Jason Baron 2011-03-16 215 return false;
d430d3d7e Jason Baron 2011-03-16 216 }
bf5438fca Jason Baron 2010-09-17 217
c5905afb0 Ingo Molnar 2012-02-24 218 static inline void static_key_slow_inc(struct static_key *key)
d430d3d7e Jason Baron 2011-03-16 219 {
5cdda5117 Borislav Petkov 2017-10-18 220 STATIC_KEY_CHECK_USE(key);
d430d3d7e Jason Baron 2011-03-16 @221 atomic_inc(&key->enabled);
d430d3d7e Jason Baron 2011-03-16 222 }
bf5438fca Jason Baron 2010-09-17 223
c5905afb0 Ingo Molnar 2012-02-24 224 static inline void static_key_slow_dec(struct static_key *key)
bf5438fca Jason Baron 2010-09-17 225 {
5cdda5117 Borislav Petkov 2017-10-18 226 STATIC_KEY_CHECK_USE(key);
d430d3d7e Jason Baron 2011-03-16 @227 atomic_dec(&key->enabled);
bf5438fca Jason Baron 2010-09-17 228 }
bf5438fca Jason Baron 2010-09-17 229
ce48c1464 Peter Zijlstra 2018-01-22 230 #define static_key_slow_inc_cpuslocked(key) static_key_slow_inc(key)
ce48c1464 Peter Zijlstra 2018-01-22 231 #define static_key_slow_dec_cpuslocked(key) static_key_slow_dec(key)
ce48c1464 Peter Zijlstra 2018-01-22 232
4c3ef6d79 Jason Baron 2010-09-17 233 static inline int jump_label_text_reserved(void *start, void *end)
4c3ef6d79 Jason Baron 2010-09-17 234 {
4c3ef6d79 Jason Baron 2010-09-17 235 return 0;
4c3ef6d79 Jason Baron 2010-09-17 236 }
4c3ef6d79 Jason Baron 2010-09-17 237
91bad2f8d Jason Baron 2010-10-01 238 static inline void jump_label_lock(void) {}
91bad2f8d Jason Baron 2010-10-01 239 static inline void jump_label_unlock(void) {}
91bad2f8d Jason Baron 2010-10-01 240
d430d3d7e Jason Baron 2011-03-16 241 static inline int jump_label_apply_nops(struct module *mod)
d430d3d7e Jason Baron 2011-03-16 242 {
d430d3d7e Jason Baron 2011-03-16 243 return 0;
d430d3d7e Jason Baron 2011-03-16 244 }
b20295207 Gleb Natapov 2011-11-27 245
e33886b38 Peter Zijlstra 2015-07-24 246 static inline void static_key_enable(struct static_key *key)
e33886b38 Peter Zijlstra 2015-07-24 247 {
5cdda5117 Borislav Petkov 2017-10-18 248 STATIC_KEY_CHECK_USE(key);
e33886b38 Peter Zijlstra 2015-07-24 249
1dbb6704d Paolo Bonzini 2017-08-01 250 if (atomic_read(&key->enabled) != 0) {
1dbb6704d Paolo Bonzini 2017-08-01 251 WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
1dbb6704d Paolo Bonzini 2017-08-01 252 return;
1dbb6704d Paolo Bonzini 2017-08-01 253 }
1dbb6704d Paolo Bonzini 2017-08-01 @254 atomic_set(&key->enabled, 1);
e33886b38 Peter Zijlstra 2015-07-24 255 }
e33886b38 Peter Zijlstra 2015-07-24 256
:::::: The code at line 194 was first introduced by commit
:::::: 4c5ea0a9cd02d6aa8adc86e100b2a4cff8d614ff locking/static_key: Fix concurrent static_key_slow_inc()
:::::: TO: Paolo Bonzini <[email protected]>
:::::: CC: Ingo Molnar <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 06/25/2018 08:22 AM, Christoph von Recklinghausen wrote:
> Add correct address for linux-mm
>
> On 06/25/2018 11:08 AM, Chris von Recklinghausen wrote:
>> Enabling HARDENED_USER_COPY causes measurable regressions in the
>> networking performances, up to 8% under UDP flood.
>>
>> A generic distro may want to enable HARDENED_USER_COPY in their default
>> kernel config, but at the same time, such distro may want to be able to
>> avoid the performance penalties in with the default configuration and
>> enable the stricter check on a per-boot basis.
>>
>> This change adds a config variable and a boot parameter to conditionally
>> enable HARDENED_USER_COPY at boot time, and switch HUC to off if
>> HUC_DEFAULT_OFF is set.
>>
>> Signed-off-by: Chris von Recklinghausen <[email protected]>
>> ---
>> .../admin-guide/kernel-parameters.rst | 2 ++
>> .../admin-guide/kernel-parameters.txt | 3 ++
>> include/linux/thread_info.h | 7 +++++
>> mm/usercopy.c | 28 +++++++++++++++++++
>> security/Kconfig | 10 +++++++
>> 5 files changed, 50 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
>> index b8d0bc07ed0a..c3035038e3ae 100644
>> --- a/Documentation/admin-guide/kernel-parameters.rst
>> +++ b/Documentation/admin-guide/kernel-parameters.rst
>> @@ -100,6 +100,8 @@ parameter is applicable::
>> FB The frame buffer device is enabled.
>> FTRACE Function tracing enabled.
>> GCOV GCOV profiling is enabled.
>> + HUC Hardened usercopy is enabled
>> + HUCF Hardened usercopy disabled at boot
>> HW Appropriate hardware is enabled.
>> IA-64 IA-64 architecture is enabled.
>> IMA Integrity measurement architecture is enabled.
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index efc7aa7a0670..cd3354bc14d3 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -816,6 +816,9 @@
>> disable= [IPV6]
>> See Documentation/networking/ipv6.txt.
>>
>> + enable_hardened_usercopy [HUC,HUCF]
>> + Enable hardened usercopy checks
>> +
>> disable_radix [PPC]
>> Disable RADIX MMU mode on POWER9
>>
>> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
>> index 8d8821b3689a..140a36cc1c2c 100644
>> --- a/include/linux/thread_info.h
>> +++ b/include/linux/thread_info.h
>> @@ -109,12 +109,19 @@ static inline int arch_within_stack_frames(const void * const stack,
>> #endif
>>
>> #ifdef CONFIG_HARDENED_USERCOPY
>> +#include <linux/jump_label.h>
>> +
>> +DECLARE_STATIC_KEY_FALSE(bypass_usercopy_checks);
>> +
>> extern void __check_object_size(const void *ptr, unsigned long n,
>> bool to_user);
>>
>> static __always_inline void check_object_size(const void *ptr, unsigned long n,
>> bool to_user)
>> {
>> + if (static_branch_likely(&bypass_usercopy_checks))
>> + return;
>> +
>> if (!__builtin_constant_p(n))
>> __check_object_size(ptr, n, to_user);
>> }
>> diff --git a/mm/usercopy.c b/mm/usercopy.c
>> index e9e9325f7638..ce3996da1b2e 100644
>> --- a/mm/usercopy.c
>> +++ b/mm/usercopy.c
>> @@ -279,3 +279,31 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
>> check_kernel_text_object((const unsigned long)ptr, n, to_user);
>> }
>> EXPORT_SYMBOL(__check_object_size);
>> +
>> +DEFINE_STATIC_KEY_FALSE(bypass_usercopy_checks);
>> +EXPORT_SYMBOL(bypass_usercopy_checks);
>> +
>> +#ifdef CONFIG_HUC_DEFAULT_OFF
>> +#define HUC_DEFAULT false
>> +#else
>> +#define HUC_DEFAULT true
>> +#endif
>> +
>> +static bool enable_huc_atboot = HUC_DEFAULT;
>> +
>> +static int __init parse_enable_usercopy(char *str)
>> +{
>> + enable_huc_atboot = true;
>> + return 1;
>> +}
>> +
>> +static int __init set_enable_usercopy(void)
>> +{
>> + if (enable_huc_atboot == false)
>> + static_branch_enable(&bypass_usercopy_checks);
>> + return 1;
>> +}
>> +
>> +__setup("enable_hardened_usercopy", parse_enable_usercopy);
>> +
>> +late_initcall(set_enable_usercopy);
>> diff --git a/security/Kconfig b/security/Kconfig
>> index c4302067a3ad..a6173897b85c 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
>> been removed. This config is intended to be used only while
>> trying to find such users.
>>
>> +config HUC_DEFAULT_OFF
>> + bool "allow CONFIG_HARDENED_USERCOPY to be configured but disabled"
>> + depends on HARDENED_USERCOPY
>> + help
>> + When CONFIG_HARDENED_USERCOPY is enabled, disable its
>> + functionality unless it is enabled via at boot time
>> + via the "enable_hardened_usercopy" boot parameter. This allows
>> + the functionality of hardened usercopy to be present but not
>> + impact performance unless it is needed.
>> +
>> config FORTIFY_SOURCE
>> bool "Harden common str/mem functions against buffer overflows"
>> depends on ARCH_HAS_FORTIFY_SOURCE
>
>
This seems a bit backwards, I'd much rather see hardened user copy
default to on with the basic config option and then just have a command
line option to turn it off.
Thanks,
Laura
On 06/25/2018 03:44 PM, Laura Abbott wrote:
> On 06/25/2018 08:22 AM, Christoph von Recklinghausen wrote:
>> Add correct address for linux-mm
>>
>> On 06/25/2018 11:08 AM, Chris von Recklinghausen wrote:
>>> Enabling HARDENED_USER_COPY causes measurable regressions in the
>>> networking performances, up to 8% under UDP flood.
>>>
>>> A generic distro may want to enable HARDENED_USER_COPY in their default
>>> kernel config, but at the same time, such distro may want to be able to
>>> avoid the performance penalties in with the default configuration and
>>> enable the stricter check on a per-boot basis.
>>>
>>> This change adds a config variable and a boot parameter to
>>> conditionally
>>> enable HARDENED_USER_COPY at boot time, and switch HUC to off if
>>> HUC_DEFAULT_OFF is set.
>>>
>>> Signed-off-by: Chris von Recklinghausen <[email protected]>
>>> ---
>>> .../admin-guide/kernel-parameters.rst | 2 ++
>>> .../admin-guide/kernel-parameters.txt | 3 ++
>>> include/linux/thread_info.h | 7 +++++
>>> mm/usercopy.c | 28
>>> +++++++++++++++++++
>>> security/Kconfig | 10 +++++++
>>> 5 files changed, 50 insertions(+)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.rst
>>> b/Documentation/admin-guide/kernel-parameters.rst
>>> index b8d0bc07ed0a..c3035038e3ae 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.rst
>>> +++ b/Documentation/admin-guide/kernel-parameters.rst
>>> @@ -100,6 +100,8 @@ parameter is applicable::
>>> FB The frame buffer device is enabled.
>>> FTRACE Function tracing enabled.
>>> GCOV GCOV profiling is enabled.
>>> + HUC Hardened usercopy is enabled
>>> + HUCF Hardened usercopy disabled at boot
>>> HW Appropriate hardware is enabled.
>>> IA-64 IA-64 architecture is enabled.
>>> IMA Integrity measurement architecture is enabled.
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>>> b/Documentation/admin-guide/kernel-parameters.txt
>>> index efc7aa7a0670..cd3354bc14d3 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -816,6 +816,9 @@
>>> disable= [IPV6]
>>> See Documentation/networking/ipv6.txt.
>>> + enable_hardened_usercopy [HUC,HUCF]
>>> + Enable hardened usercopy checks
>>> +
>>> disable_radix [PPC]
>>> Disable RADIX MMU mode on POWER9
>>> diff --git a/include/linux/thread_info.h
>>> b/include/linux/thread_info.h
>>> index 8d8821b3689a..140a36cc1c2c 100644
>>> --- a/include/linux/thread_info.h
>>> +++ b/include/linux/thread_info.h
>>> @@ -109,12 +109,19 @@ static inline int
>>> arch_within_stack_frames(const void * const stack,
>>> #endif
>>> #ifdef CONFIG_HARDENED_USERCOPY
>>> +#include <linux/jump_label.h>
>>> +
>>> +DECLARE_STATIC_KEY_FALSE(bypass_usercopy_checks);
>>> +
>>> extern void __check_object_size(const void *ptr, unsigned long n,
>>> bool to_user);
>>> static __always_inline void check_object_size(const void *ptr,
>>> unsigned long n,
>>> bool to_user)
>>> {
>>> + if (static_branch_likely(&bypass_usercopy_checks))
>>> + return;
>>> +
>>> if (!__builtin_constant_p(n))
>>> __check_object_size(ptr, n, to_user);
>>> }
>>> diff --git a/mm/usercopy.c b/mm/usercopy.c
>>> index e9e9325f7638..ce3996da1b2e 100644
>>> --- a/mm/usercopy.c
>>> +++ b/mm/usercopy.c
>>> @@ -279,3 +279,31 @@ void __check_object_size(const void *ptr,
>>> unsigned long n, bool to_user)
>>> check_kernel_text_object((const unsigned long)ptr, n, to_user);
>>> }
>>> EXPORT_SYMBOL(__check_object_size);
>>> +
>>> +DEFINE_STATIC_KEY_FALSE(bypass_usercopy_checks);
>>> +EXPORT_SYMBOL(bypass_usercopy_checks);
>>> +
>>> +#ifdef CONFIG_HUC_DEFAULT_OFF
>>> +#define HUC_DEFAULT false
>>> +#else
>>> +#define HUC_DEFAULT true
>>> +#endif
>>> +
>>> +static bool enable_huc_atboot = HUC_DEFAULT;
>>> +
>>> +static int __init parse_enable_usercopy(char *str)
>>> +{
>>> + enable_huc_atboot = true;
>>> + return 1;
>>> +}
>>> +
>>> +static int __init set_enable_usercopy(void)
>>> +{
>>> + if (enable_huc_atboot == false)
>>> + static_branch_enable(&bypass_usercopy_checks);
>>> + return 1;
>>> +}
>>> +
>>> +__setup("enable_hardened_usercopy", parse_enable_usercopy);
>>> +
>>> +late_initcall(set_enable_usercopy);
>>> diff --git a/security/Kconfig b/security/Kconfig
>>> index c4302067a3ad..a6173897b85c 100644
>>> --- a/security/Kconfig
>>> +++ b/security/Kconfig
>>> @@ -189,6 +189,16 @@ config HARDENED_USERCOPY_PAGESPAN
>>> been removed. This config is intended to be used only while
>>> trying to find such users.
>>> +config HUC_DEFAULT_OFF
>>> + bool "allow CONFIG_HARDENED_USERCOPY to be configured but
>>> disabled"
>>> + depends on HARDENED_USERCOPY
>>> + help
>>> + When CONFIG_HARDENED_USERCOPY is enabled, disable its
>>> + functionality unless it is enabled via at boot time
>>> + via the "enable_hardened_usercopy" boot parameter. This allows
>>> + the functionality of hardened usercopy to be present but not
>>> + impact performance unless it is needed.
>>> +
>>> config FORTIFY_SOURCE
>>> bool "Harden common str/mem functions against buffer overflows"
>>> depends on ARCH_HAS_FORTIFY_SOURCE
>>
>>
>
> This seems a bit backwards, I'd much rather see hardened user copy
> default to on with the basic config option and then just have a command
> line option to turn it off.
>
> Thanks,
> Laura
I have a small set of customers that want CONFIG_HARDENED_USERCOPY
enabled, and a large number of customers who would be impacted by its
default behavior (before my change). The desire was to have the smaller
number of users need to change their boot lines to get the behavior they
wanted. Adding CONFIG_HUC_DEFAULT_OFF was an attempt to preserve the
default behavior of existing users of CONFIG_HARDENED_USERCOPY (default
enabled) and allowing that to coexist with the desires of the greater
number of my customers (default disabled).
If folks think that it's better to have it enabled by default and the
command line option to turn it off I can do that (it is simpler). Does
anyone else have opinions one way or the other?
Thanks,
Chris
On Mon, Jun 25, 2018 at 3:29 PM, Christoph von Recklinghausen
<[email protected]> wrote:
> I have a small set of customers that want CONFIG_HARDENED_USERCOPY
> enabled, and a large number of customers who would be impacted by its
> default behavior (before my change). The desire was to have the smaller
> number of users need to change their boot lines to get the behavior they
> wanted. Adding CONFIG_HUC_DEFAULT_OFF was an attempt to preserve the
> default behavior of existing users of CONFIG_HARDENED_USERCOPY (default
> enabled) and allowing that to coexist with the desires of the greater
> number of my customers (default disabled).
>
> If folks think that it's better to have it enabled by default and the
> command line option to turn it off I can do that (it is simpler). Does
> anyone else have opinions one way or the other?
I would prefer to isolate the actual problem case, and fix it if
possible. (i.e. try to make the copy fixed-length, etc) Barring that,
yes, a kernel command line to disable the protection would be okay.
Note that the test needs to be inside __check_object_size() otherwise
the inline optimization with __builtin_constant_p() gets broken and
makes everyone slower. :)
-Kees
--
Kees Cook
Pixel Security
On 06/25/2018 06:35 PM, Kees Cook wrote:
> On Mon, Jun 25, 2018 at 3:29 PM, Christoph von Recklinghausen
> <[email protected]> wrote:
>> I have a small set of customers that want CONFIG_HARDENED_USERCOPY
>> enabled, and a large number of customers who would be impacted by its
>> default behavior (before my change). The desire was to have the smaller
>> number of users need to change their boot lines to get the behavior they
>> wanted. Adding CONFIG_HUC_DEFAULT_OFF was an attempt to preserve the
>> default behavior of existing users of CONFIG_HARDENED_USERCOPY (default
>> enabled) and allowing that to coexist with the desires of the greater
>> number of my customers (default disabled).
>>
>> If folks think that it's better to have it enabled by default and the
>> command line option to turn it off I can do that (it is simpler). Does
>> anyone else have opinions one way or the other?
> I would prefer to isolate the actual problem case, and fix it if
> possible. (i.e. try to make the copy fixed-length, etc) Barring that,
> yes, a kernel command line to disable the protection would be okay.
>
> Note that the test needs to be inside __check_object_size() otherwise
> the inline optimization with __builtin_constant_p() gets broken and
> makes everyone slower. :)
>
> -Kees
>
Thanks Kees,
I'll make that change and retest.
Chris
Hi,
On Mon, 25 Jun 2018 11:21:38 -0700 Kees Cook <[email protected]> wrote:
> On Mon, Jun 25, 2018 at 8:08 AM, Chris von Recklinghausen
> <[email protected]> wrote:
> > Enabling HARDENED_USER_COPY causes measurable regressions in the
> > networking performances, up to 8% under UDP flood.
>
> Which function is "hot"? i.e. which copy*user() is taking up the time?
> Do you have a workload that at can be used to reproduce the problem?
I'm running an a small packet UDP flood using pktgen vs. an host b2b
connected. On the receiver side the UDP packets are processed by a
simple user space process that just read and drop them:
https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c
Not very useful from a functional PoV, it helps mostly pin-pointing
bottle-neck in the networking stack.
When running a kernel with CONFIG_HARDENED_USERCOPY=y, I see a 5-8%
regression in the receive tput, compared to the same kernel without
such option.
With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).
Cheers,
Paolo
On Tue, Jun 26, 2018 at 2:48 AM, Paolo Abeni <[email protected]> wrote:
> Hi,
>
> On Mon, 25 Jun 2018 11:21:38 -0700 Kees Cook <[email protected]> wrote:
>> On Mon, Jun 25, 2018 at 8:08 AM, Chris von Recklinghausen
>> <[email protected]> wrote:
>> > Enabling HARDENED_USER_COPY causes measurable regressions in the
>> > networking performances, up to 8% under UDP flood.
>>
>> Which function is "hot"? i.e. which copy*user() is taking up the time?
>> Do you have a workload that at can be used to reproduce the problem?
>
> I'm running an a small packet UDP flood using pktgen vs. an host b2b
> connected. On the receiver side the UDP packets are processed by a
> simple user space process that just read and drop them:
>
> https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c
>
> Not very useful from a functional PoV, it helps mostly pin-pointing
> bottle-neck in the networking stack.
Cool; thanks for the pointer!
> When running a kernel with CONFIG_HARDENED_USERCOPY=y, I see a 5-8%
> regression in the receive tput, compared to the same kernel without
> such option.
>
> With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
> cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).
Are you able to see which network functions are making the
__check_object_size() calls?
-Kees
--
Kees Cook
Pixel Security
[hopefully fixed the 'mm' recipient]
On Tue, 2018-06-26 at 09:54 -0700, Kees Cook wrote:
> On Tue, Jun 26, 2018 at 2:48 AM, Paolo Abeni <[email protected]> wrote:
> > With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
> > cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).
>
> Are you able to see which network functions are making the
> __check_object_size() calls?
The call-chain is:
__GI___libc_recvfrom
entry_SYSCALL_64_after_hwframe
do_syscall_64
__x64_sys_recvfrom
__sys_recvfrom
inet_recvmsg
udp_recvmsg
__check_object_size
udp_recvmsg() actually calls copy_to_iter() (inlined) and the latters
calls check_copy_size() (again, inlined).
Cheers,
Paolo