| Message ID | 20220303031505.28495-2-dtcccc@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Alloc kfence_pool after system startup | expand |
On Thu, 3 Mar 2022 at 04:15, Tianchen Ding <dtcccc@linux.alibaba.com> 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@linux.alibaba.com> 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(); ... Thanks, -- Marco
On 2022/3/5 02:13, Marco Elver wrote: > On Thu, 3 Mar 2022 at 04:15, Tianchen Ding <dtcccc@linux.alibaba.com> 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@linux.alibaba.com> > > 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
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@linux.alibaba.com> >> 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@linux.alibaba.com> >> >> 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; }
On Sat, 5 Mar 2022 at 07:06, Tianchen Ding <dtcccc@linux.alibaba.com> wrote: [...] > 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. Yes, that's probably safer and easier. Thanks, -- Marco
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) + return kfence_enable_late(); return 0; } @@ -787,6 +789,16 @@ void __init kfence_init(void) (void *)(__kfence_pool + KFENCE_POOL_SIZE)); } +static int kfence_enable_late(void) +{ + if (!__kfence_pool) + return -EINVAL; + + WRITE_ONCE(kfence_enabled, true); + queue_delayed_work(system_unbound_wq, &kfence_timer, 0); + return 0; +} + void kfence_shutdown_cache(struct kmem_cache *s) { unsigned long flags;
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@linux.alibaba.com> --- mm/kfence/core.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)