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);
Thanks,
-- Marco
