| Message ID | 20180621212835.5636-19-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/21/2018 04:28 PM, Matthew Wilcox wrote: > @@ -1163,11 +1157,9 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn, > goto old_sess_out; > if (conn->sess->se_sess) > transport_free_session(conn->sess->se_sess); > - if (conn->sess->session_index != 0) { > - spin_lock_bh(&sess_idr_lock); > - idr_remove(&sess_idr, conn->sess->session_index); > - spin_unlock_bh(&sess_idr_lock); This code looks buggy. We will probably NULL pointer oops before we hit it. It looks like the session_index check was supposed to detect when login fails in the middle of doing login, so that code probably wanted to do: idr_alloc(&sess_idr, NULL, 1, 0, GFP_NOWAIT); The problem is that iscsi_login_zero_tsih_s1 sets conn->sess early in iscsi_login_set_conn_values. If the function fails later like when we alloc the idr it does kfree(sess) and leaves the conn->sess pointer set. iscsi_login_zero_tsih_s1 then returns -Exyz and we then call iscsi_target_login_sess_out and access the freed memory above. So I am not sure what we want to do here for your patch since it is not adding any new bugs. Just merge your patch now and I can send a fix for the above bug over it? > - } > + /* Um, 0 is a valid ID. I suppose we never free it? */ > + if (conn->sess->session_index != 0) > + ida_free(&sess_ida, conn->sess->session_index); > kfree(conn->sess->sess_ops); > kfree(conn->sess); > conn->sess = NULL; > -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/26/2018 11:48 AM, Mike Christie wrote: > On 06/21/2018 04:28 PM, Matthew Wilcox wrote: > >> @@ -1163,11 +1157,9 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn, >> goto old_sess_out; >> if (conn->sess->se_sess) >> transport_free_session(conn->sess->se_sess); >> - if (conn->sess->session_index != 0) { >> - spin_lock_bh(&sess_idr_lock); >> - idr_remove(&sess_idr, conn->sess->session_index); >> - spin_unlock_bh(&sess_idr_lock); > > This code looks buggy. We will probably NULL pointer oops before we hit it. Sorry did not mean null pointer, but some crash due to accessing memory that was freed. > > It looks like the session_index check was supposed to detect when login > fails in the middle of doing login, so that code probably wanted to do: > > idr_alloc(&sess_idr, NULL, 1, 0, GFP_NOWAIT); > > The problem is that iscsi_login_zero_tsih_s1 sets conn->sess early in > iscsi_login_set_conn_values. If the function fails later like when we > alloc the idr it does kfree(sess) and leaves the conn->sess pointer set. > iscsi_login_zero_tsih_s1 then returns -Exyz and we then call > iscsi_target_login_sess_out and access the freed memory above. > > So I am not sure what we want to do here for your patch since it is not > adding any new bugs. Just merge your patch now and I can send a fix for > the above bug over it? > > >> - } >> + /* Um, 0 is a valid ID. I suppose we never free it? */ >> + if (conn->sess->session_index != 0) >> + ida_free(&sess_ida, conn->sess->session_index); >> kfree(conn->sess->sess_ops); >> kfree(conn->sess); >> conn->sess = NULL; >> > -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/26/2018 11:48 AM, Mike Christie wrote: > So I am not sure what we want to do here for your patch since it is not > adding any new bugs. Just merge your patch now and I can send a fix for > the above bug over it? If we want to fix the bug first, then I made the attached patch and I can submit it. From 80c495c3d7f4487c1b6f52f70e8ddc74dcb70794 Mon Sep 17 00:00:00 2001 From: Mike Christie <mchristi@redhat.com> Date: Thu, 26 Jul 2018 12:06:07 -0500 Subject: [PATCH] iscsi target: fix session creation failure handling The problem is that iscsi_login_zero_tsih_s1 sets conn->sess early in iscsi_login_set_conn_values. If the function fails later like when we alloc the idr it does kfree(sess) and leaves the conn->sess pointer set. iscsi_login_zero_tsih_s1 then returns -Exyz and we then call iscsi_target_login_sess_out and access the freed memory. This patch has iscsi_login_zero_tsih_s1 either completely setup the session or completely tear it down, so later in iscsi_target_login_sess_out we can just check for it being set to the connection. --- drivers/target/iscsi/iscsi_target_login.c | 35 ++++++++++++++++++------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 9950178..e20d811 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -348,8 +348,7 @@ static int iscsi_login_zero_tsih_s1( pr_err("idr_alloc() for sess_idr failed\n"); iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, ISCSI_LOGIN_STATUS_NO_RESOURCES); - kfree(sess); - return -ENOMEM; + goto free_sess; } sess->creation_time = get_jiffies_64(); @@ -365,20 +364,28 @@ static int iscsi_login_zero_tsih_s1( ISCSI_LOGIN_STATUS_NO_RESOURCES); pr_err("Unable to allocate memory for" " struct iscsi_sess_ops.\n"); - kfree(sess); - return -ENOMEM; + goto remove_idr; } sess->se_sess = transport_init_session(TARGET_PROT_NORMAL); if (IS_ERR(sess->se_sess)) { iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, ISCSI_LOGIN_STATUS_NO_RESOURCES); - kfree(sess->sess_ops); - kfree(sess); - return -ENOMEM; + goto free_ops; } return 0; + +free_ops: + kfree(sess->sess_ops); +remove_idr: + spin_lock_bh(&sess_idr_lock); + idr_remove(&sess_idr, sess->session_index); + spin_unlock_bh(&sess_idr_lock); +free_sess: + kfree(sess); + conn->sess = NULL; + return -ENOMEM; } static int iscsi_login_zero_tsih_s2( @@ -1161,13 +1168,13 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn, ISCSI_LOGIN_STATUS_INIT_ERR); if (!zero_tsih || !conn->sess) goto old_sess_out; - if (conn->sess->se_sess) - transport_free_session(conn->sess->se_sess); - if (conn->sess->session_index != 0) { - spin_lock_bh(&sess_idr_lock); - idr_remove(&sess_idr, conn->sess->session_index); - spin_unlock_bh(&sess_idr_lock); - } + + transport_free_session(conn->sess->se_sess); + + spin_lock_bh(&sess_idr_lock); + idr_remove(&sess_idr, conn->sess->session_index); + spin_unlock_bh(&sess_idr_lock); + kfree(conn->sess->sess_ops); kfree(conn->sess); conn->sess = NULL;
On Thu, Jul 26, 2018 at 12:13:49PM -0500, Mike Christie wrote: > If we want to fix the bug first, then I made the attached patch and I > can submit it. How about I take it through my tree to minimise the amount of rebasing I'll need to do? I'm already dependent on the nvdimm tree and I'd rather not depend on the scsi tree as well. I'll queue it up in front of my IDA change to maximise its backportability. > >From 80c495c3d7f4487c1b6f52f70e8ddc74dcb70794 Mon Sep 17 00:00:00 2001 > From: Mike Christie <mchristi@redhat.com> > Date: Thu, 26 Jul 2018 12:06:07 -0500 > Subject: [PATCH] iscsi target: fix session creation failure handling > > The problem is that iscsi_login_zero_tsih_s1 sets conn->sess early in > iscsi_login_set_conn_values. If the function fails later like when we > alloc the idr it does kfree(sess) and leaves the conn->sess pointer set. > iscsi_login_zero_tsih_s1 then returns -Exyz and we then call > iscsi_target_login_sess_out and access the freed memory. > > This patch has iscsi_login_zero_tsih_s1 either completely setup the > session or completely tear it down, so later in > iscsi_target_login_sess_out we can just check for it being set to the > connection. > --- > drivers/target/iscsi/iscsi_target_login.c | 35 ++++++++++++++++++------------- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c > index 9950178..e20d811 100644 > --- a/drivers/target/iscsi/iscsi_target_login.c > +++ b/drivers/target/iscsi/iscsi_target_login.c > @@ -348,8 +348,7 @@ static int iscsi_login_zero_tsih_s1( > pr_err("idr_alloc() for sess_idr failed\n"); > iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, > ISCSI_LOGIN_STATUS_NO_RESOURCES); > - kfree(sess); > - return -ENOMEM; > + goto free_sess; > } > > sess->creation_time = get_jiffies_64(); > @@ -365,20 +364,28 @@ static int iscsi_login_zero_tsih_s1( > ISCSI_LOGIN_STATUS_NO_RESOURCES); > pr_err("Unable to allocate memory for" > " struct iscsi_sess_ops.\n"); > - kfree(sess); > - return -ENOMEM; > + goto remove_idr; > } > > sess->se_sess = transport_init_session(TARGET_PROT_NORMAL); > if (IS_ERR(sess->se_sess)) { > iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, > ISCSI_LOGIN_STATUS_NO_RESOURCES); > - kfree(sess->sess_ops); > - kfree(sess); > - return -ENOMEM; > + goto free_ops; > } > > return 0; > + > +free_ops: > + kfree(sess->sess_ops); > +remove_idr: > + spin_lock_bh(&sess_idr_lock); > + idr_remove(&sess_idr, sess->session_index); > + spin_unlock_bh(&sess_idr_lock); > +free_sess: > + kfree(sess); > + conn->sess = NULL; > + return -ENOMEM; > } > > static int iscsi_login_zero_tsih_s2( > @@ -1161,13 +1168,13 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn, > ISCSI_LOGIN_STATUS_INIT_ERR); > if (!zero_tsih || !conn->sess) > goto old_sess_out; > - if (conn->sess->se_sess) > - transport_free_session(conn->sess->se_sess); > - if (conn->sess->session_index != 0) { > - spin_lock_bh(&sess_idr_lock); > - idr_remove(&sess_idr, conn->sess->session_index); > - spin_unlock_bh(&sess_idr_lock); > - } > + > + transport_free_session(conn->sess->se_sess); > + > + spin_lock_bh(&sess_idr_lock); > + idr_remove(&sess_idr, conn->sess->session_index); > + spin_unlock_bh(&sess_idr_lock); > + > kfree(conn->sess->sess_ops); > kfree(conn->sess); > conn->sess = NULL; > -- > 1.8.3.1 > -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/27/2018 02:38 PM, Matthew Wilcox wrote: > On Thu, Jul 26, 2018 at 12:13:49PM -0500, Mike Christie wrote: >> If we want to fix the bug first, then I made the attached patch and I >> can submit it. > > How about I take it through my tree to minimise the amount of rebasing > I'll need to do? I'm already dependent on the nvdimm tree and I'd rather > not depend on the scsi tree as well. I'll queue it up in front of my > IDA change to maximise its backportability. Ccing Martin, because he has been handling target patches. The above plan sounds ok to me. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mike, >> How about I take it through my tree to minimise the amount of rebasing >> I'll need to do? I'm already dependent on the nvdimm tree and I'd rather >> not depend on the scsi tree as well. I'll queue it up in front of my >> IDA change to maximise its backportability. > > Ccing Martin, because he has been handling target patches. That's fine with me. Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
On Mon, Jul 30, 2018 at 10:03:21PM -0400, Martin K. Petersen wrote: > > Mike, > > >> How about I take it through my tree to minimise the amount of rebasing > >> I'll need to do? I'm already dependent on the nvdimm tree and I'd rather > >> not depend on the scsi tree as well. I'll queue it up in front of my > >> IDA change to maximise its backportability. > > > > Ccing Martin, because he has been handling target patches. > > That's fine with me. > > Acked-by: Martin K. Petersen <martin.petersen@oracle.com> Thanks, Martin. Mike, can I have your Signed-off-by: on the original patch? What Fixes: line should this have? As far as I can tell, the bug is present all the way back to its introduction, so: Fixes: e48354ce078c ("iscsi-target: Add iSCSI fabric support for target v4.1") yes? -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2018 01:15 PM, Matthew Wilcox wrote: > On Mon, Jul 30, 2018 at 10:03:21PM -0400, Martin K. Petersen wrote: >> >> Mike, >> >>>> How about I take it through my tree to minimise the amount of rebasing >>>> I'll need to do? I'm already dependent on the nvdimm tree and I'd rather >>>> not depend on the scsi tree as well. I'll queue it up in front of my >>>> IDA change to maximise its backportability. >>> >>> Ccing Martin, because he has been handling target patches. >> >> That's fine with me. >> >> Acked-by: Martin K. Petersen <martin.petersen@oracle.com> > > Thanks, Martin. Mike, can I have your Signed-off-by: on the original patch? Yes, Signed-off-by: Mike Christie <mchristi@redhat.com> > What Fixes: line should this have? As far as I can tell, the bug is present > all the way back to its introduction, so: > > Fixes: e48354ce078c ("iscsi-target: Add iSCSI fabric support for target v4.1") > > yes? The session_index bug was in that patch, but I think it correctly kfreed the session in __iscsi_target_login_thread. I think it was this one that added the early kfree bug: commit 0957627a99604f379d35831897a2aa15272528fc Author: Nicholas Bellinger <nab@linux-iscsi.org> Date: Fri Nov 4 11:36:38 2011 -0700 iscsi-target: Fix sess allocation leak in iscsi_login_zero_tsih_s1 Dan's tools probably just did not catch the iscsi_login_zero_tsih_s1 -> iscsi_login_set_conn_values call that sets conn->sess then later kfree(conn->sess) call in the error handling in __iscsi_target_login_thread. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index 8e223799347a..94bad43c41ff 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -57,9 +57,8 @@ static DEFINE_SPINLOCK(tiqn_lock); static DEFINE_MUTEX(np_lock); static struct idr tiqn_idr; -struct idr sess_idr; +DEFINE_IDA(sess_ida); struct mutex auth_id_lock; -spinlock_t sess_idr_lock; struct iscsit_global *iscsit_global; @@ -700,9 +699,7 @@ static int __init iscsi_target_init_module(void) spin_lock_init(&iscsit_global->ts_bitmap_lock); mutex_init(&auth_id_lock); - spin_lock_init(&sess_idr_lock); idr_init(&tiqn_idr); - idr_init(&sess_idr); ret = target_register_template(&iscsi_ops); if (ret) @@ -4375,10 +4372,7 @@ int iscsit_close_session(struct iscsi_session *sess) pr_debug("Decremented number of active iSCSI Sessions on" " iSCSI TPG: %hu to %u\n", tpg->tpgt, tpg->nsessions); - spin_lock(&sess_idr_lock); - idr_remove(&sess_idr, sess->session_index); - spin_unlock(&sess_idr_lock); - + ida_free(&sess_ida, sess->session_index); kfree(sess->sess_ops); sess->sess_ops = NULL; spin_unlock_bh(&se_tpg->session_lock); diff --git a/drivers/target/iscsi/iscsi_target.h b/drivers/target/iscsi/iscsi_target.h index 42de1843aa40..48bac0acf8c7 100644 --- a/drivers/target/iscsi/iscsi_target.h +++ b/drivers/target/iscsi/iscsi_target.h @@ -55,9 +55,7 @@ extern struct kmem_cache *lio_ooo_cache; extern struct kmem_cache *lio_qr_cache; extern struct kmem_cache *lio_r2t_cache; -extern struct idr sess_idr; +extern struct ida sess_ida; extern struct mutex auth_id_lock; -extern spinlock_t sess_idr_lock; - #endif /*** ISCSI_TARGET_H ***/ diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 99501785cdc1..561d2ad38989 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -336,22 +336,16 @@ static int iscsi_login_zero_tsih_s1( timer_setup(&sess->time2retain_timer, iscsit_handle_time2retain_timeout, 0); - idr_preload(GFP_KERNEL); - spin_lock_bh(&sess_idr_lock); - ret = idr_alloc(&sess_idr, NULL, 0, 0, GFP_NOWAIT); - if (ret >= 0) - sess->session_index = ret; - spin_unlock_bh(&sess_idr_lock); - idr_preload_end(); - + ret = ida_alloc(&sess_ida, GFP_KERNEL); if (ret < 0) { - pr_err("idr_alloc() for sess_idr failed\n"); + pr_err("Session ID allocation failed %d\n", ret); iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, ISCSI_LOGIN_STATUS_NO_RESOURCES); kfree(sess); return -ENOMEM; } + sess->session_index = ret; sess->creation_time = get_jiffies_64(); /* * The FFP CmdSN window values will be allocated from the TPG's @@ -1163,11 +1157,9 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn, goto old_sess_out; if (conn->sess->se_sess) transport_free_session(conn->sess->se_sess); - if (conn->sess->session_index != 0) { - spin_lock_bh(&sess_idr_lock); - idr_remove(&sess_idr, conn->sess->session_index); - spin_unlock_bh(&sess_idr_lock); - } + /* Um, 0 is a valid ID. I suppose we never free it? */ + if (conn->sess->session_index != 0) + ida_free(&sess_ida, conn->sess->session_index); kfree(conn->sess->sess_ops); kfree(conn->sess); conn->sess = NULL;
Since the session is never looked up by ID, we can use the more space-efficient IDA instead of the IDR. I think I found a bug where we never reuse session ID 0, but there may be a good reason for it, so I've merely documented it in this patch. Not reusing session ID 0 doesn't even leak memory. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- drivers/target/iscsi/iscsi_target.c | 10 ++-------- drivers/target/iscsi/iscsi_target.h | 4 +--- drivers/target/iscsi/iscsi_target_login.c | 20 ++++++-------------- 3 files changed, 9 insertions(+), 25 deletions(-)