Re: [PATCH 07/23] watchdog: Sync up with runtime change of isolated CPUs

On 4/21/26 11:08 PM, [email protected] wrote:
> Sashiko AI review found 1 potential issue(s):
> - [High] Silent overwrite and clobbering of userspace `watchdog_cpumask` sysctl configuration.
> --
>
> commit c51e1c592377490b0da49dd889417304c34badc2
> Author: Waiman Long <[email protected]>
>
> watchdog: Sync up with runtime change of isolated CPUs
>
> This commit updates the watchdog to exclude isolated CPUs at runtime by
> adding a new watchdog_cpumask_update() helper. It modifies watchdog_cpumask
> and watchdog_allowed_mask when the HK_TYPE_KERNEL_NOISE cpumask is updated.
>
>> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> index 7d675781bc917..f35374abf457c 100644
>> --- a/kernel/watchdog.c
>> +++ b/kernel/watchdog.c
> [ ... ]
>> +/**
>> + * watchdog_cpumask_update - update watchdog_cpumask & watchdog_allowed_mask
>> + * @isol_mask: cpumask of isolated CPUs
>> + *
>> + * Update watchdog_cpumask and watchdog_allowed_mask to be inverse of the
>> + * given isolated cpumask to disable watchdog activities on isolated CPUs.
>> + * It should be called with the affected CPUs in offline state which will be
>> + * brought up online later.
>> + *
>> + * Any changes made in watchdog_cpumask by users via the sysctl parameter will
>> + * be overridden. However, proc_watchdog_update() isn't called. So change will
>> + * only happens on CPUs that will brought up later on to minimize changes to
>> + * the existing watchdog configuration.
>> + */
>> +void watchdog_cpumask_update(struct cpumask *isol_mask)
>> +{
>> +	mutex_lock(&watchdog_mutex);
>> +	cpumask_andnot(&watchdog_cpumask, cpu_possible_mask, isol_mask);
> Does this unintentionally discard the existing sysctl configuration for
> kernel.watchdog_cpumask?

Yes, it does, as documented in the comment above. However, except for 
the transitioning CPUs, the watchdog activities in other untouched CPUs 
will remain unchanged until the next time one of the watchdog sysctl 
parameters is updated. I will certainly welcome any suggestion of a 
better way of handling this kind of conflict. I should update the sysctl 
parameter document to highlight the expected change.


>
> By using cpu_possible_mask as the base, it looks like this unconditionally
> overwrites the entire watchdog_cpumask with all possible CPUs except the
> newly isolated ones.
>
> If an administrator had explicitly disabled watchdogs on CPUs 1-6 using the
> sysctl interface, and later dynamically isolates CPU 7, would this operation
> silently clobber the user API contract and re-enable watchdogs on CPUs 1-6
> when they are next onlined or reconfigured?

As said above, the current watchdog setting of other CPUs will not be 
affected until when another watchdog sysctl parameter write.

Cheers,
Longman