From: Claudio Jeker Subject: Re: [patch 2 of 3] iscsid(8): avoid double free & race during cleanup To: Jack Burton Cc: tech@openbsd.org Date: Thu, 16 Jan 2025 21:27:34 +0100 On Thu, Jan 16, 2025 at 01:37:13PM +1030, Jack Burton wrote: > On Wed, 15 Jan 2025 14:46:02 +0100 > Claudio Jeker wrote: > > On Tue, Dec 31, 2024 at 06:33:08PM +1030, Jack Burton wrote: > > > Fixing that one unmasked a latent race condition during shutdown. > > > Depending on whether session reestablishment attempts result in any > > > new connections entering the login phase before ISCSI_EXIT_WAIT > > > expires, EITHER conn_free() would get called multiple times on the > > > same connection OR there'd be an outright access after free. > > > > > > To avoid adding extra states to the fsm, simply check exit_rounds > > > (which gets set in shutdown_cb) before freeing connections in > > > sess_do_free(): if it's set then we're about to exit anyway, so it > > > doesn't matter that some connections may get reallocated without > > > freeing the old ones. > > > > Uhm. I don't understand how conn_free() can be called multiple times. > > sess_do_free() calls conn_free() in the loop for each connection in the > TAILQ. Then sess_do_conn_fail() calls conn_free() because > connection state is CONN_FREE. > > I don't really understand why sess_do_conn_fail() is getting called at > all, as after sess_do_free() the current session state should be > SESS_FREE (not SESS_RUNNING) ... but somehow it does get called. > Ah, I see. The problem is that sessev has another pointer to the connection and the sessev is not removed in conn_free(). I need to reread the code to see how we can fix that mess. But what happens is that we have a event queued with a pointer to a connection that was already freed. -- :wq Claudio