Messages in this thread | | | Date | Wed, 22 Apr 2015 11:18:23 -0400 | From | Don Zickus <> | Subject | Re: [PATCH v9 2/3] watchdog: add watchdog_cpumask sysctl to assist nohz |
| |
On Tue, Apr 21, 2015 at 10:07:00AM -0400, Ulrich Obergfell wrote: > > Chris, > > I think it would also be nice to check the plausibility of the user input. > > +int proc_watchdog_cpumask(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + int err; > + > + mutex_lock(&watchdog_proc_mutex); > + err = proc_do_large_bitmap(table, write, buffer, lenp, ppos); > + if (!err && write) { > + /* Remove impossible cpus to keep sysctl output cleaner. */ > + cpumask_and(watchdog_cpumask, watchdog_cpumask, > + cpu_possible_mask); > + > + if (watchdog_enabled && watchdog_thresh) > + smpboot_update_cpumask_percpu_thread(&watchdog_threads, > + watchdog_cpumask); > + } > + mutex_unlock(&watchdog_proc_mutex); > + return err; > +} > > I think the user should only be allowed to specify a mask that is a subset of > tick_nohz_full_mask as only those CPUs don't have a watchdog thread by default. > In other words, the user should not be able to interfere with housekeeping CPUs.
Hi Uli,
I am not sure that is necessary. This was supposed to be a debugging interface for nohz (and possibly other technologies). I think restricting it to just tick_nohz makes it difficult to try out new things or debug certain problems.
Personally, I feel anyone who will use this sys interface will need to do so at their own risk.
Cheers, Don
> > For example, add a plausibility check like so: > > save watchdog_cpumask because proc_do_large_bitmap() is going to change it > > proc_do_large_bitmap() > > // return an error if the user-specified mask includes a housekeeping CPU > if (watchdog_cpumask and 'negated tick_nohz_full_mask') { > restore saved watchdog_cpumask > return -EINVAL > } > > > Regards, > > Uli
| |