Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 13 Nov 2022 19:15:18 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Stephen Boyd <sboyd@kernel.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Anna-Maria Gleixner <anna-maria@linutronix.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        Julia Lawall <Julia.Lawall@inria.fr>
Subject: Re: [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called
 before freeing timers
Message-ID: <20221113191518.0a3b29b1@rorschach.local.home>
In-Reply-To: <875yfitpdu.ffs@tglx>
References: <20221110064101.429013735@goodmis.org>
        <20221110064147.343514404@goodmis.org>
        <875yfitpdu.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 14 Nov 2022 00:18:21 +0100
Thomas Gleixner <tglx@linutronix.de> wrote:

> > @@ -1285,11 +1281,25 @@ int try_to_del_timer_sync(struct timer_list *timer)
> >  
> >  	if (base->running_timer != timer)
> >  		ret = detach_if_pending(timer, base, true);
> > +	if (free)
> > +		timer->function = NULL;  
> 
> Same problem as in the timer_shutdown() case just more subtle:
> 
> CPU0                           		CPU1
> 
>                                         lock_timer(timer);
>                                         base->running_timer = timer;
> 					fn = timer->function;
> 					unlock_timer(timer);
> 					fn(timer) {
> 
> __try_to_del_timer_sync(timer, free=true)
>     lock_timer(timer);
>     if (base->running_timer != timer)
>        // Not taken
>     if (free)                             mod_timer(timer);
>                                             if (WARN_ON_ONCE(!timer->function))
>                                                return; // not taken
>        timer->function = NULL;
>     unlock_timer(timer);
> 					    lock_timer(timer);
>                                             enqueue_timer(timer);
> 					    unlock_timer(timer);
>                                         }
> 
> 					//timer expires
> 					lock_timer(timer);
> 					fn = timer->function;
> 					unlock_timer(timer);
> 					fn(timer); <--- NULL pointer dereference
> 
> You surely have spent a massive amount of analysis on this!
> 
> Can you please explain how you came up with the brilliant idea of asking
> Linus to pull this post -rc4 without a review from the timer maintainers
> or anyone else who understands concurrency?

I trusted the source of this code:

  https://lore.kernel.org/all/87pmlrkgi3.ffs@tglx/


-- Steve