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:   Wed, 5 Jul 2023 13:36:44 +0300
From:   Leon Romanovsky <leon@kernel.org>
To:     Rock =?utf-8?B?TGko5p2O5a6P5LyfKQ==?= <lihongweizz@inspur.com>
Cc:     "sagi@grimberg.me" <sagi@grimberg.me>,
        "mgurtovoy@nvidia.com" <mgurtovoy@nvidia.com>,
        "jgg@ziepe.ca" <jgg@ziepe.ca>,
        "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] IB/iser: Protect tasks cleanup in case iser connection
 was stopped
Message-ID: <20230705103644.GL6455@unreal>
References: <45c68d6835964dcbae8fbd983696064b@inspur.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <45c68d6835964dcbae8fbd983696064b@inspur.com>
Precedence: bulk

On Wed, Jul 05, 2023 at 09:22:08AM +0000, Rock Li(李宏伟) wrote:
> > > On Tue, Jul 04, 2023 at 08:51:44AM +0800, lihongweizz wrote:
> > > From: Rock Li <lihongweizz@inspur.com>
> > >
> > > We met a crash issue as below:
> > > ...
> > >  #7 [ff61b991f6f63d10] page_fault at ffffffffab80111e
> > >     [exception RIP: iscsi_iser_cleanup_task+13]
> > >     RIP: ffffffffc046c04d RSP: ff61b991f6f63dc0 RFLAGS: 00010246
> > >     RAX: 0000000000000000 RBX: ff4bd0aalf7a5610 RCX: ff61b991f6f63dc8
> > >     RDX: ff61b991f6f63d68 RSI: ff61b991f6f63d58 RDI: ff4bd0aalf6cdc00
> > >     RBP: 0000000000000005 R8: 0000000000000073 R9:
> > 0000000000000005
> > >     R10: 0000000000000000 R11: 00000ccde3e0f5c0 R12:
> > ff4bd08c0e0631f8
> > >     R13: ff4bd0a95ffd3c78 R14: ff4bd0a95ffd3c78 R15: ff4bd0aalf6cdc00
> > >     ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> > >  #8 [ff616991f6f63dc0] __iscsi_put_task at ffffffffc0bd3652 [libiscsi]
> > >  #9 [ff61b991f6f63e00] iscsi_put_task at ffffffffc0bd36e9 [libiscsi]
> > > ...
> > >
> > > After analysing the vmcore, we find that the iser connection was
> > > already stopped before abort handler running. The iser_conn is already
> > > unbindded and released. So we add iser connection validation check
> > > inside cleanup task to fix this corner case.
> > >
> > > Signed-off-by: Rock Li <lihongweizz@inspur.com>
> > > ---
> > >  drivers/infiniband/ulp/iser/iscsi_iser.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c
> > > b/drivers/infiniband/ulp/iser/iscsi_iser.c
> > > index bb9aaff92ca3..35dfbf41fc40 100644
> > > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> > > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> > > @@ -366,7 +366,12 @@ static void iscsi_iser_cleanup_task(struct
> iscsi_task
> > *task)
> > >  	struct iscsi_iser_task *iser_task = task->dd_data;
> > >  	struct iser_tx_desc *tx_desc = &iser_task->desc;
> > >  	struct iser_conn *iser_conn = task->conn->dd_data;
> > > -	struct iser_device *device = iser_conn->ib_conn.device;
> > > +	struct iser_device *device;
> > > +
> > > +	/* stop connection might happens before iser cleanup work */
> > > +	if (!iser_conn)
> > > +		return;
> > 
> > And what prevents from iser_conn being not valid here?
> > For example, in the flow:
> > 1. Start iscsi_iser_cleanup_task
> > 2. Get valid task->conn->dd_data
> > 3. Pass this if (..) check
> > 4. Context switch and release connection 5. iser_conn now points to
> released
> > memory.
> > 
> > Thanks
> 
> Hi Leon,
> Thanks for your reply:) In case iscsi_stop_conn was executed cocurrently or
> after iscsi_iser_cleanup_task, above issue would happen.
> I've confirmed the values in iscsi_cls_conn and iscsi_conn instances from
> vmcore:
> 
> iscsi_stop_conn
>   ...
>   WRITE_ONCE(conn->state, ISCSI_CONN_FAILED);      --- confirmed
>   ...
>   conn->transport->stop_conn => iscsi_iser_conn_stop
>     iscsi_conn_stop
>       ...
>       conn->c_stage = ISCSI_CONN_STOPPED;          --- confirmed
>     conn->dd_data = NULL;                          --- confirmed
> 
> The crash scene tells us that iscsi_stop_conn was executed before
> iscsi_iser_cleanup_task start, the iser_conn instance was already released. 

It is by chance, it will be better to provide a fix which is race free.

Thanks

> 
> > 
> > > +	device = iser_conn->ib_conn.device;
> > >
> > >  	/* DEVICE_REMOVAL event might have already released the device */
> > >  	if (!device)
> > > --
> > > 2.27.0
> > >