Re: [PATCH v9 2/3] watchdog: add watchdog_cpumask sysctl to assist nohz

Don Zickus
2015-04-22T15:21:42Z

On Wed, Apr 22, 2015 at 07:02:31AM -0400, Ulrich Obergfell wrote:
>
> Chris,
>
> in https://lkml.org/lkml/2015/4/17/616 you stated:
>
> ">> + alloc_cpumask_var(&watchdog_cpumask_for_smpboot, GFP_KERNEL);
> >
> > alloc_cpumask_var could fail?
>
> Good catch; if I get a failure I'll just return early without trying to
> start the watchdog, since clearly things are too memory-constrained
> to enable that functionality anyway."
>
> Let's assume that (in spite of the memory constraints) the kernel would still
> be able to make progress and get to a point where the system will be usable.
> In this corner case, the following code would leave a NULL pointer behind in
> watchdog_cpumask and in watchdog_cpumask_bits which could subsequently lead
> to a crash.
>
> void __init lockup_detector_init(void)
> {
> set_sample_period();
>
> + if (!alloc_cpumask_var(&watchdog_cpumask, GFP_KERNEL)) {
> + pr_err("Failed to allocate cpumask for watchdog");
> + return;
> + }
> + watchdog_cpumask_bits = cpumask_bits(watchdog_cpumask);
>
> For example, proc_watchdog_cpumask() and the change that your patch introduces
> in watchdog_enable_all_cpus() are not protected against a possible NULL pointer.
> I think the code needs to be made safer.

Or we could just statically allocate it

static DECLARE_BITMAP(watchdog_cpumask, NR_CPUS) __read_mostly;

Cheers,
Don