Re: [Xen-devel] [PATCH 3/4] xen/watchdog: Drop all locked operations on the watchdog_inuse_map

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.19 at 17:17, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 13/05/2019 16:01, Jan Beulich wrote:
>>>>> On 10.05.19 at 20:28, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> All modifications to the watchdog_inuse_map happen with d->watchdog_lock 
>>> held,
>>> so there are no concurrency problems to deal with.
>> But concurrency problems can also occur for readers. While
>> not a problem afaict, dump_domains() actually has such an
>> example (and hence might be worth mentioning here).
> 
> Its only debugging, and would need to take the spinlock anyway to avoid
> a TOCTOU race between watchdog_inuse_map and d->watchdog_timer[i].expires

Right, hence my suggestion to just mention it here.

>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -1068,17 +1068,15 @@ static long domain_watchdog(struct domain *d, 
>>> uint32_t id, uint32_t timeout)
>>>      }
>>>      else /* Allocate the next available timer. */
>>>      {
>>> -        for ( id = 0; id < NR_DOMAIN_WATCHDOG_TIMERS; id++ )
>>> -        {
>>> -            if ( test_and_set_bit(id, &d->watchdog_inuse_map) )
>>> -                continue;
>>> -            break;
>>> -        }
>>> -        if ( id == NR_DOMAIN_WATCHDOG_TIMERS )
>>> +        id = ffs(~d->watchdog_inuse_map) - 1;
>> I'm surprised we have no (universally available) ffz(). I wonder
>> though whether find_first_zero_bit() wouldn't be the better
>> choice here anyway, as the result wouldn't be undefined in
>> case NR_DOMAIN_WATCHDOG_TIMERS grew to 32.
> 
> Sadly no - find_first_zero_bit() takes unsigned long *, so its use here
> a) requires a (void *) cast to compile, and b) is definitely UB.

Oh, right, it works with a pointer. Would you mind adding a
BUILD_BUG_ON() then to exclude the UB case of ffs()? With
that then
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel