From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: domain protocol timer unlock
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Fri, 17 May 2024 23:14:02 +0300

> On 17 May 2024, at 23:01, Alexander Bluhm <bluhm@openbsd.org> wrote:
> 
> On Fri, May 17, 2024 at 09:14:31PM +0200, Claudio Jeker wrote:
>>>> -	if (!igmp_timers_are_running)
>>>> +	if (!READ_ONCE(igmp_timers_are_running))
>>>> 		return;
>> 
>> This can't be correct. I know what you want but that is not the right way
>> of doing this check. Which daemon are you removing by using READ_ONCE()
>> here?
> 
> As long as the compiler knows that foo() and bar() don't touch
> variable, the compiler can optimize this code:
> 
> 	if (variable)
> 		foo();
> 	else
> 		bar();
> 
> to this construct:
> 
> 	if (variable)
> 		foo();
> 	if (!variable)
> 		bar();
> 
> If variable is global and accessed by another CPU the following can
> happen:
> 
> CPU 0 sets variable = 1;
> 
> CPU 1 calls foo()
> 
> CPU 0 sets variable = 0;
> 
> CPU 1 calls bar()
> 
> This was never intended in the if/else code and READ_ONCE() or
> volatile prevents it.
> 
> Of course in the code we are talking about it is very unlikely that
> something like this can ever happen.  But I have a very bad feeling
> about accessing a variable from multiple CPU without any protection.
> I would like to have a consistent pattern to prevent MP surprise.
> 
> But I looks that I am the only one who cares about this problem.
> Am I too paranoid for MP?

But there is no else branch. What compiler will optimise here?

igmp_fasttimo(void)
{       
        struct ifnet *ifp;
        
        /*
         * Quick check to see if any work needs to be done, in order
         * to minimize the overhead of fasttimo processing.
         * Variable igmp_timers_are_running is read atomically, but without
         * lock intentionally.  In case it is not set due to MP races, we may
         * miss to check the timers.  Then run the loop at next fast timeout.
         */
        if (!igmp_timers_are_running)
                return;
        
        NET_LOCK();
        
        igmp_timers_are_running = 0;
        TAILQ_FOREACH(ifp, &ifnetlist, if_list)
                igmp_checktimer(ifp);
        
        NET_UNLOCK();
}