On 2022/3/5 13:26, Tianchen Ding wrote:
On 2022/3/5 02:13, Marco Elver wrote:
On Thu, 3 Mar 2022 at 04:15, Tianchen Ding <dtcccc@xxxxxxxxxxxxxxxxx>
wrote:
If once KFENCE is disabled by:
echo 0 > /sys/module/kfence/parameters/sample_interval
KFENCE could never be re-enabled until next rebooting.
Allow re-enabling it by writing a positive num to sample_interval.
Signed-off-by: Tianchen Ding <dtcccc@xxxxxxxxxxxxxxxxx>
The only problem I see with this is if KFENCE was disabled because of
a KFENCE_WARN_ON(). See below.
---
mm/kfence/core.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 13128fa13062..19eb123c0bba 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -55,6 +55,7 @@ EXPORT_SYMBOL_GPL(kfence_sample_interval); /*
Export for test modules. */
#endif
#define MODULE_PARAM_PREFIX "kfence."
+static int kfence_enable_late(void);
static int param_set_sample_interval(const char *val, const struct
kernel_param *kp)
{
unsigned long num;
@@ -65,10 +66,11 @@ static int param_set_sample_interval(const char
*val, const struct kernel_param
if (!num) /* Using 0 to indicate KFENCE is disabled. */
WRITE_ONCE(kfence_enabled, false);
- else if (!READ_ONCE(kfence_enabled) && system_state !=
SYSTEM_BOOTING)
- return -EINVAL; /* Cannot (re-)enable KFENCE
on-the-fly. */
*((unsigned long *)kp->arg) = num;
+
+ if (num && !READ_ONCE(kfence_enabled) && system_state !=
SYSTEM_BOOTING)
Should probably have an 'old_sample_interval = *((unsigned long
*)kp->arg)' somewhere before, and add a '&& !old_sample_interval',
because if old_sample_interval!=0 then KFENCE was disabled due to a
KFENCE_WARN_ON(). Also in this case, it should return -EINVAL. So you
want a flow like this:
old_sample_interval = ...;
...
if (num && !READ_ONCE(kfence_enabled) && system_state != SYSTEM_BOOTING)
return old_sample_interval ? -EINVAL : kfence_enable_late();
...
Because sample_interval will used by delayed_work, we must put setting
sample_interval before enabling KFENCE.
So the order would be:
old_sample_interval = sample_interval;
sample_interval = num;
if (...) kfence_enable_late();
This may be bypassed after KFENCE_WARN_ON() happens, if we first write
0, and then write 100 to it.
How about this one:
if (ret < 0)
return ret;
+ /* Cannot set sample_interval after KFENCE_WARN_ON(). */
+ if (unlikely(*((unsigned long *)kp->arg) &&
!READ_ONCE(kfence_enabled)))
+ return -EINVAL;
+
if (!num) /* Using 0 to indicate KFENCE is disabled. */
WRITE_ONCE(kfence_enabled, false);
Hmm...
I found KFENCE_WARN_ON() may be called when sample_interval==0. (e.g.,
kfence_guarded_free())
So it's better to add a bool.
diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index ae69b2a113a4..c729be0207e8 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -38,14 +38,17 @@
#define KFENCE_WARN_ON(cond)
\
({ \
const bool __cond = WARN_ON(cond); \
- if (unlikely(__cond)) \
+ if (unlikely(__cond)) { \
WRITE_ONCE(kfence_enabled, false); \
+ disabled_by_warn = true; \
+ } \
__cond; \
})
/* === Data
================================================================= */
static bool kfence_enabled __read_mostly;
+static bool disabled_by_warn __read_mostly;
unsigned long kfence_sample_interval __read_mostly =
CONFIG_KFENCE_SAMPLE_INTERVAL;
EXPORT_SYMBOL_GPL(kfence_sample_interval); /* Export for test modules. */
@@ -70,7 +73,7 @@ static int param_set_sample_interval(const char *val,
const struct kernel_param
*((unsigned long *)kp->arg) = num;
if (num && !READ_ONCE(kfence_enabled) && system_state != SYSTEM_BOOTING)
- return kfence_enable_late();
+ return disabled_by_warn ? -EINVAL : kfence_enable_late();
return 0;
}
