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); }
/**
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
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.
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
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
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.
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
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.
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
linux-stable-mirror@lists.linaro.org
-
Bart Van Assche -
Bart Van Assche
-
Greg KH -
gregkh＠linuxfoundation.org
-
Jason Gunthorpe