[PATCH 1/3] IB/srpt: Fix srpt_cm_req_recv() error path (1/2)

[PATCH 1/3] IB/srpt: Fix srpt_cm_req_recv() error path (1/2)

[Bart Van Assche]

Once a target session has been allocated, if an error occurs, the
session must be freed. Since it is not safe to call blocking code
from the context of an connection manager callback, trigger target
session release in this case by calling srpt_close_ch().

Fixes: db7683d7deb2 ("IB/srpt: Fix login-related race conditions")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: <stable@vger.kernel.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 538b9013e815..e95d0c1c0652 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2088,7 +2088,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 		struct rdma_conn_param rdma_cm;
 		struct ib_cm_rep_param ib_cm;
 	} *rep_param = NULL;
-	struct srpt_rdma_ch *ch;
+	struct srpt_rdma_ch *ch = NULL;
 	char i_port_id[36];
 	u32 it_iu_len;
 	int i, ret;
@@ -2235,13 +2235,15 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 						TARGET_PROT_NORMAL,
 						i_port_id + 2, ch, NULL);
 	if (IS_ERR_OR_NULL(ch->sess)) {
+		WARN_ON_ONCE(ch->sess == NULL);
 		ret = PTR_ERR(ch->sess);
+		ch->sess = NULL;
 		pr_info("Rejected login for initiator %s: ret = %d.\n",
 			ch->sess_name, ret);
 		rej->reason = cpu_to_be32(ret == -ENOMEM ?
 				SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES :
 				SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
-		goto reject;
+		goto destroy_ib;
 	}
 
 	mutex_lock(&sport->mutex);
@@ -2280,7 +2282,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 		rej->reason = cpu_to_be32(SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
 		pr_err("rejected SRP_LOGIN_REQ because enabling RTR failed (error code = %d)\n",
 		       ret);
-		goto destroy_ib;
+		goto reject;
 	}
 
 	pr_debug("Establish connection sess=%p name=%s ch=%p\n", ch->sess,
@@ -2380,6 +2382,15 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 		ib_send_cm_rej(ib_cm_id, IB_CM_REJ_CONSUMER_DEFINED, NULL, 0,
 			       rej, sizeof(*rej));
 
+	if (ch && ch->sess) {
+		srpt_close_ch(ch);
+		/*
+		 * Tell the caller not to free cm_id since
+		 * srpt_release_channel_work() will do that.
+		 */
+		ret = 0;
+	}
+
 out:
 	kfree(rep_param);
 	kfree(rsp);
-- 
2.18.0

[PATCH 2/3] IB/srpt: Fix srpt_cm_req_recv() error path (2/2)

[Bart Van Assche]

If a login request was received through the RDMA/CM and if an
error occurs during login, clear rdma_cm_id->context instead of
ib_cm_id->context.

Fixes: 63cf1a902c9d ("IB/srpt: Add RDMA/CM support")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: <stable@vger.kernel.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index e95d0c1c0652..325bae29e90d 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2361,8 +2361,11 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_ring,
 			     ch->sport->sdev, ch->rq_size,
 			     ch->max_rsp_size, DMA_TO_DEVICE);
+
 free_ch:
-	if (ib_cm_id)
+	if (rdma_cm_id)
+		rdma_cm_id->context = NULL;
+	else
 		ib_cm_id->context = NULL;
 	kfree(ch);
 	ch = NULL;
-- 
2.18.0

[PATCH 3/3] IB/srpt: Fix a use-after-free

[Bart Van Assche]

Make sure that channel objects continue to exist until the target
core has called the .close_session() callback function. This patch
voids that KASAN sporadically reports the following:

BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c/0x130
Read of size 4 at addr ffff8801534b16e4 by task rmdir/14805
CPU: 16 PID: 14805 Comm: rmdir Not tainted 4.18.0-rc2-dbg+ #5
Call Trace:
dump_stack+0xa4/0xf5
print_address_description+0x6f/0x270
kasan_report+0x241/0x360
__asan_load4+0x78/0x80
do_raw_spin_lock+0x1c/0x130
_raw_spin_lock_irqsave+0x52/0x60
srpt_set_ch_state+0x27/0x70 [ib_srpt]
srpt_disconnect_ch+0x1b/0xc0 [ib_srpt]
srpt_close_session+0xa8/0x260 [ib_srpt]
target_shutdown_sessions+0x170/0x180 [target_core_mod]
core_tpg_del_initiator_node_acl+0xf3/0x200 [target_core_mod]
target_fabric_nacl_base_release+0x25/0x30 [target_core_mod]
config_item_release+0x9c/0x110 [configfs]
config_item_put+0x26/0x30 [configfs]
configfs_rmdir+0x3b8/0x510 [configfs]
vfs_rmdir+0xb3/0x1e0
do_rmdir+0x262/0x2c0
do_syscall_64+0x77/0x230
entry_SYSCALL_64_after_hwframe+0x49/0xbe

Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: <stable@vger.kernel.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 325bae29e90d..705f6a992d82 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 	}
 
 	kref_init(&ch->kref);
+	kref_get(&ch->kref);
 	ch->pkey = be16_to_cpu(pkey);
 	ch->nexus = nexus;
 	ch->zw_cqe.done = srpt_zerolength_write_done;
@@ -3212,6 +3213,7 @@ static void srpt_close_session(struct se_session *se_sess)
 	struct srpt_rdma_ch *ch = se_sess->fabric_sess_ptr;
 
 	srpt_disconnect_ch_sync(ch);
+	kref_put(&ch->kref, srpt_free_ch);
 }
 
 /**
-- 
2.18.0

Re: [PATCH 3/3] IB/srpt: Fix a use-after-free

[Jason Gunthorpe]

On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote:
> Make sure that channel objects continue to exist until the target
> core has called the .close_session() callback function. This patch
> voids that KASAN sporadically reports the following:
> 
> BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c/0x130
> Read of size 4 at addr ffff8801534b16e4 by task rmdir/14805
> CPU: 16 PID: 14805 Comm: rmdir Not tainted 4.18.0-rc2-dbg+ #5
> Call Trace:
> dump_stack+0xa4/0xf5
> print_address_description+0x6f/0x270
> kasan_report+0x241/0x360
> __asan_load4+0x78/0x80
> do_raw_spin_lock+0x1c/0x130
> _raw_spin_lock_irqsave+0x52/0x60
> srpt_set_ch_state+0x27/0x70 [ib_srpt]
> srpt_disconnect_ch+0x1b/0xc0 [ib_srpt]
> srpt_close_session+0xa8/0x260 [ib_srpt]
> target_shutdown_sessions+0x170/0x180 [target_core_mod]
> core_tpg_del_initiator_node_acl+0xf3/0x200 [target_core_mod]
> target_fabric_nacl_base_release+0x25/0x30 [target_core_mod]
> config_item_release+0x9c/0x110 [configfs]
> config_item_put+0x26/0x30 [configfs]
> configfs_rmdir+0x3b8/0x510 [configfs]
> vfs_rmdir+0xb3/0x1e0
> do_rmdir+0x262/0x2c0
> do_syscall_64+0x77/0x230
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1")
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: <stable@vger.kernel.org>
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 325bae29e90d..705f6a992d82 100644
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
>  	}
>
>  	kref_init(&ch->kref);
> +	kref_get(&ch->kref);

Oh that is ugly, can you put the 'get' closer to the the place that
stores the ch pointer this kref is protecting? Perhaps it is one of
the list_add's in this function?

Guessing the kref created kref_init should probably belong to target_core's
'se_sess->fabric_sess_ptr' ..

Jason

Re: [PATCH 3/3] IB/srpt: Fix a use-after-free

[Greg KH]

On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote:
> Make sure that channel objects continue to exist until the target
> core has called the .close_session() callback function. This patch
> voids that KASAN sporadically reports the following:
> 
> BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c/0x130
> Read of size 4 at addr ffff8801534b16e4 by task rmdir/14805
> CPU: 16 PID: 14805 Comm: rmdir Not tainted 4.18.0-rc2-dbg+ #5
> Call Trace:
> dump_stack+0xa4/0xf5
> print_address_description+0x6f/0x270
> kasan_report+0x241/0x360
> __asan_load4+0x78/0x80
> do_raw_spin_lock+0x1c/0x130
> _raw_spin_lock_irqsave+0x52/0x60
> srpt_set_ch_state+0x27/0x70 [ib_srpt]
> srpt_disconnect_ch+0x1b/0xc0 [ib_srpt]
> srpt_close_session+0xa8/0x260 [ib_srpt]
> target_shutdown_sessions+0x170/0x180 [target_core_mod]
> core_tpg_del_initiator_node_acl+0xf3/0x200 [target_core_mod]
> target_fabric_nacl_base_release+0x25/0x30 [target_core_mod]
> config_item_release+0x9c/0x110 [configfs]
> config_item_put+0x26/0x30 [configfs]
> configfs_rmdir+0x3b8/0x510 [configfs]
> vfs_rmdir+0xb3/0x1e0
> do_rmdir+0x262/0x2c0
> do_syscall_64+0x77/0x230
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1")
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 325bae29e90d..705f6a992d82 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
>  	}
>  
>  	kref_init(&ch->kref);
> +	kref_get(&ch->kref);

kref_init starts the reference count at at 1, so why do you need to
increment it right away?  That feels like something is "odd" here, why
do you start with 2 references in the same function?

thanks,

greg k-h

Re: [PATCH 3/3] IB/srpt: Fix a use-after-free

[Bart Van Assche]

On Tue, 2018-07-10 at 12:38 -0600, Jason Gunthorpe wrote:
> On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote:
> > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > index 325bae29e90d..705f6a992d82 100644
> > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
> >  	}
> > 
> >  	kref_init(&ch->kref);
> > +	kref_get(&ch->kref);
> 
> Oh that is ugly, can you put the 'get' closer to the the place that
> stores the ch pointer this kref is protecting? Perhaps it is one of
> the list_add's in this function?
> 
> Guessing the kref created kref_init should probably belong to target_core's
> 'se_sess->fabric_sess_ptr' ..

Hello Jason,

Can you clarify your second paragraph? The ib_srpt driver has been implemented
such that sess->fabric_sess_ptr == ch as long as a session exists.

An ib_srpt RDMA channel object (ch in the above code) must stay around as long
as the associated target core session (se_sess) exists and also as long as the
target core has not yet called srpt_close_session(). Hence the initialization of
ch->kref to 2 just before an RDMA channel is registered with the target core.
Hence also the kref_put() calls in srpt_close_session() and
srpt_release_channel_work().

Bart.

Re: [PATCH 3/3] IB/srpt: Fix a use-after-free

[Bart Van Assche]

On Tue, 2018-07-10 at 20:51 +0200, Greg KH wrote:
> On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote:
> > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > index 325bae29e90d..705f6a992d82 100644
> > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
> >  	}
> >  
> >  	kref_init(&ch->kref);
> > +	kref_get(&ch->kref);
> 
> kref_init starts the reference count at at 1, so why do you need to
> increment it right away?  That feels like something is "odd" here, why
> do you start with 2 references in the same function?

Hi Greg,

An ib_srpt RDMA channel object (ch in the above code) must stay around as long
as the associated target core session (se_sess) exists and also as long as the
target core has not yet called srpt_close_session(). Hence the initialization of
ch->kref to 2 just before an RDMA channel is registered with the target core.
Hence also the kref_put() calls in srpt_close_session() and
srpt_release_channel_work(). Please let me know if you need more information.

Thanks,

Bart.

Re: [PATCH 3/3] IB/srpt: Fix a use-after-free

[gregkh]

On Tue, Jul 10, 2018 at 08:23:01PM +0000, Bart Van Assche wrote:
> On Tue, 2018-07-10 at 20:51 +0200, Greg KH wrote:
> > On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote:
> > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > index 325bae29e90d..705f6a992d82 100644
> > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
> > >  	}
> > >  
> > >  	kref_init(&ch->kref);
> > > +	kref_get(&ch->kref);
> > 
> > kref_init starts the reference count at at 1, so why do you need to
> > increment it right away?  That feels like something is "odd" here, why
> > do you start with 2 references in the same function?
> 
> Hi Greg,
> 
> An ib_srpt RDMA channel object (ch in the above code) must stay around as long
> as the associated target core session (se_sess) exists and also as long as the
> target core has not yet called srpt_close_session(). Hence the initialization of
> ch->kref to 2 just before an RDMA channel is registered with the target core.

Shouldn't the registration increment the reference?  Starting out at "2"
feels very "odd", don't you agree?

thanks,

greg k-h

Re: [PATCH 3/3] IB/srpt: Fix a use-after-free

[Bart Van Assche]

On Tue, 2018-07-10 at 22:44 +0200, gregkh@linuxfoundation.org wrote:
> On Tue, Jul 10, 2018 at 08:23:01PM +0000, Bart Van Assche wrote:
> > On Tue, 2018-07-10 at 20:51 +0200, Greg KH wrote:
> > > On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote:
> > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > index 325bae29e90d..705f6a992d82 100644
> > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
> > > >  	}
> > > >  
> > > >  	kref_init(&ch->kref);
> > > > +	kref_get(&ch->kref);
> > > 
> > > kref_init starts the reference count at at 1, so why do you need to
> > > increment it right away?  That feels like something is "odd" here, why
> > > do you start with 2 references in the same function?
> > 
> > An ib_srpt RDMA channel object (ch in the above code) must stay around as long
> > as the associated target core session (se_sess) exists and also as long as the
> > target core has not yet called srpt_close_session(). Hence the initialization of
> > ch->kref to 2 just before an RDMA channel is registered with the target core.
> 
> Shouldn't the registration increment the reference?  Starting out at "2"
> feels very "odd", don't you agree?

Hello Greg,

The code that registers the session with the target core is in another driver
(SCSI target core) and that driver doesn't know about the abstractions maintained
by the ib_srpt driver. That's why the above kref_get() call is in the ib_srpt
driver and not e.g. in the target_alloc_session() function.

BTW, there is more code in the Linux kernel that follows the above pattern.
target_submit_cmd_map_sgls() initializes the se_cmd reference count to two as
follows:
* transport_init_se_cmd() initializes it to one.
* target_get_sess_cmd() increments it from one to two.
This is because two contexts keep a reference to se_cmd data structures,
namely the target core and the target driver. A SCSI target command data
structure (se_cmd) must only be freed after both contexts have finished their
part of the command processing.

Thanks,

Bart.

Re: [PATCH 3/3] IB/srpt: Fix a use-after-free

[Jason Gunthorpe]

On Tue, Jul 10, 2018 at 08:19:20PM +0000, Bart Van Assche wrote:
> On Tue, 2018-07-10 at 12:38 -0600, Jason Gunthorpe wrote:
> > On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote:
> > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > index 325bae29e90d..705f6a992d82 100644
> > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
> > >  	}
> > > 
> > >  	kref_init(&ch->kref);
> > > +	kref_get(&ch->kref);
> > 
> > Oh that is ugly, can you put the 'get' closer to the the place that
> > stores the ch pointer this kref is protecting? Perhaps it is one of
> > the list_add's in this function?
> > 
> > Guessing the kref created kref_init should probably belong to target_core's
> > 'se_sess->fabric_sess_ptr' ..
> 
> Hello Jason,
> 
> Can you clarify your second paragraph? The ib_srpt driver has been implemented
> such that sess->fabric_sess_ptr == ch as long as a session exists.

In my view that means sess->fabric_sess_ptr is the pointer that
'owns' the kref_init's value - ie the kref_init pairs with the
kref_put added in your patch.

The question here, is what kref_put does the 2nd kref_get pair with?
When you identify that, then move the kref_get closer to the
assignment to the 'owning' pointer.

krefs are very tricky, I find it is best to be very rigorous with
them, and place some notes about how they pair and what storage is
'owning' each kref.

eg if you put something in a list, and incr the kref when doing so,
then place the kref_get near the list_add, and the kref_put near the
list_del. That gives somebody else a chance to understand that the
list_head is holding the kref.

And you have to be careful when sharing the stack kref with something
else - eg if a list_add 'moves' a stack kref into a list then another
thread can race and do list_del and put, even though the first thread
is still accessing the memory on its stack. Often times these
'publish' actions must be last in a function, or more krefs are
needed.

This is a really annoying bug class with krefs I find from time to time..

Jason

Re: [PATCH 3/3] IB/srpt: Fix a use-after-free

[gregkh]

On Tue, Jul 10, 2018 at 09:02:51PM +0000, Bart Van Assche wrote:
> On Tue, 2018-07-10 at 22:44 +0200, gregkh@linuxfoundation.org wrote:
> > On Tue, Jul 10, 2018 at 08:23:01PM +0000, Bart Van Assche wrote:
> > > On Tue, 2018-07-10 at 20:51 +0200, Greg KH wrote:
> > > > On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote:
> > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > > index 325bae29e90d..705f6a992d82 100644
> > > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > > > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
> > > > >  	}
> > > > >  
> > > > >  	kref_init(&ch->kref);
> > > > > +	kref_get(&ch->kref);
> > > > 
> > > > kref_init starts the reference count at at 1, so why do you need to
> > > > increment it right away?  That feels like something is "odd" here, why
> > > > do you start with 2 references in the same function?
> > > 
> > > An ib_srpt RDMA channel object (ch in the above code) must stay around as long
> > > as the associated target core session (se_sess) exists and also as long as the
> > > target core has not yet called srpt_close_session(). Hence the initialization of
> > > ch->kref to 2 just before an RDMA channel is registered with the target core.
> > 
> > Shouldn't the registration increment the reference?  Starting out at "2"
> > feels very "odd", don't you agree?
> 
> Hello Greg,
> 
> The code that registers the session with the target core is in another driver
> (SCSI target core) and that driver doesn't know about the abstractions maintained
> by the ib_srpt driver.

Ok, but then that registration shouldn't be dropping a reference either,
right?

> That's why the above kref_get() call is in the ib_srpt
> driver and not e.g. in the target_alloc_session() function.

But I still do not understand why you have 2 on the count here.  Why do
you need that?

> BTW, there is more code in the Linux kernel that follows the above pattern.
> target_submit_cmd_map_sgls() initializes the se_cmd reference count to two as
> follows:
> * transport_init_se_cmd() initializes it to one.
> * target_get_sess_cmd() increments it from one to two.

That is two different areas of the code, right?  That is not "initialize
the count to 2 when we first create it".

> This is because two contexts keep a reference to se_cmd data structures,
> namely the target core and the target driver. A SCSI target command data
> structure (se_cmd) must only be freed after both contexts have finished their
> part of the command processing.

It's your subsytem, I'm just trying to point out that this pattern you
have created is very odd and is probably wrong.  If you all insist it is
correct, that's great, but I did warn you :)

good luck!

greg k-h