Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
Re: [patch 2 of 3] iscsid(8): avoid double free & race during cleanup
To:
Jack Burton <jack@saosce.com.au>
Cc:
tech@openbsd.org
Date:
Thu, 16 Jan 2025 21:27:34 +0100

Download raw body.

Thread
On Thu, Jan 16, 2025 at 01:37:13PM +1030, Jack Burton wrote:
> On Wed, 15 Jan 2025 14:46:02 +0100
> Claudio Jeker <cjeker@diehard.n-r-g.com> 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