| Message ID | 20201007145326.56850-2-mlombard@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fix race conditions with task aborts | expand |
On 2020-10-07 07:53, Maurizio Lombardi wrote: > A potential race condition may occur in iscsit_unmap_cmd() if the > __iscsit_free_cmd() function is called by two different threads. > > This patch adds a spinlock to serialize the calls to > iscsit_unmap_cmd() > > Signed-off-by: Maurizio Lombardi <mlombard@redhat.com> > --- > drivers/target/iscsi/iscsi_target_login.c | 1 + > drivers/target/iscsi/iscsi_target_util.c | 5 ++++- > include/target/iscsi/iscsi_target_core.h | 1 + > 3 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c > index 893d1b406c29..e16ceee87bba 100644 > --- a/drivers/target/iscsi/iscsi_target_login.c > +++ b/drivers/target/iscsi/iscsi_target_login.c > @@ -1110,6 +1110,7 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np) > spin_lock_init(&conn->nopin_timer_lock); > spin_lock_init(&conn->response_queue_lock); > spin_lock_init(&conn->state_lock); > + spin_lock_init(&conn->unmap_cmd_lock); > > timer_setup(&conn->nopin_response_timer, > iscsit_handle_nopin_response_timeout, 0); > diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c > index 45ba07c6ec27..3082f5bde9fa 100644 > --- a/drivers/target/iscsi/iscsi_target_util.c > +++ b/drivers/target/iscsi/iscsi_target_util.c > @@ -755,8 +755,11 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool check_queues) > iscsit_remove_cmd_from_response_queue(cmd, conn); > } > > - if (conn && conn->conn_transport->iscsit_unmap_cmd) > + if (conn && conn->conn_transport->iscsit_unmap_cmd) { > + spin_lock(&conn->unmap_cmd_lock); > conn->conn_transport->iscsit_unmap_cmd(conn, cmd); > + spin_unlock(&conn->unmap_cmd_lock); > + } > } This looks weird to me. Shouldn't the iSCSI target code make sure that __iscsit_free_cmd() is called once per command instead of allowing concurrent calls of that function and serializing iscsit_unmap_cmd() calls? Thanks, Bart.
Dne 08. 10. 20 v 4:15 Bart Van Assche napsal(a): > On 2020-10-07 07:53, Maurizio Lombardi wrote: >> A potential race condition may occur in iscsit_unmap_cmd() if the >> __iscsit_free_cmd() function is called by two different threads. >> >> This patch adds a spinlock to serialize the calls to >> iscsit_unmap_cmd() >> >> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com> >> --- >> drivers/target/iscsi/iscsi_target_login.c | 1 + >> drivers/target/iscsi/iscsi_target_util.c | 5 ++++- >> include/target/iscsi/iscsi_target_core.h | 1 + >> 3 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c >> index 893d1b406c29..e16ceee87bba 100644 >> --- a/drivers/target/iscsi/iscsi_target_login.c >> +++ b/drivers/target/iscsi/iscsi_target_login.c >> @@ -1110,6 +1110,7 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np) >> spin_lock_init(&conn->nopin_timer_lock); >> spin_lock_init(&conn->response_queue_lock); >> spin_lock_init(&conn->state_lock); >> + spin_lock_init(&conn->unmap_cmd_lock); >> >> timer_setup(&conn->nopin_response_timer, >> iscsit_handle_nopin_response_timeout, 0); >> diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c >> index 45ba07c6ec27..3082f5bde9fa 100644 >> --- a/drivers/target/iscsi/iscsi_target_util.c >> +++ b/drivers/target/iscsi/iscsi_target_util.c >> @@ -755,8 +755,11 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool check_queues) >> iscsit_remove_cmd_from_response_queue(cmd, conn); >> } >> >> - if (conn && conn->conn_transport->iscsit_unmap_cmd) >> + if (conn && conn->conn_transport->iscsit_unmap_cmd) { >> + spin_lock(&conn->unmap_cmd_lock); >> conn->conn_transport->iscsit_unmap_cmd(conn, cmd); >> + spin_unlock(&conn->unmap_cmd_lock); >> + } >> } > > This looks weird to me. Shouldn't the iSCSI target code make sure that > __iscsit_free_cmd() is called once per command instead of allowing concurrent > calls of that function and serializing iscsit_unmap_cmd() calls? __iscsit_free_cmd() doesn't actually "free" the command. If you look at the code, you will notice that the only thing that it does is to stop some timers, remove the command from the immediate and response queues and call the unmap callback. In some cases, the iscsit_free_cmd() calls __iscsit_free_cmd() twice against the same command (yes, it's safe). It should also be safe to execute __iscsit_free_cmd() even after a concurrent call to iscsit_free_cmd() has been completed because the abort timer holds a reference against the command. Additionally, all the functions called by __iscsit_free_cmd() are thread-safe, with the exception of the unmap callback that may crash the kernel if the cxgbit offload driver is used. Maurizio
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 893d1b406c29..e16ceee87bba 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -1110,6 +1110,7 @@ static struct iscsi_conn *iscsit_alloc_conn(struct iscsi_np *np) spin_lock_init(&conn->nopin_timer_lock); spin_lock_init(&conn->response_queue_lock); spin_lock_init(&conn->state_lock); + spin_lock_init(&conn->unmap_cmd_lock); timer_setup(&conn->nopin_response_timer, iscsit_handle_nopin_response_timeout, 0); diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c index 45ba07c6ec27..3082f5bde9fa 100644 --- a/drivers/target/iscsi/iscsi_target_util.c +++ b/drivers/target/iscsi/iscsi_target_util.c @@ -755,8 +755,11 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool check_queues) iscsit_remove_cmd_from_response_queue(cmd, conn); } - if (conn && conn->conn_transport->iscsit_unmap_cmd) + if (conn && conn->conn_transport->iscsit_unmap_cmd) { + spin_lock(&conn->unmap_cmd_lock); conn->conn_transport->iscsit_unmap_cmd(conn, cmd); + spin_unlock(&conn->unmap_cmd_lock); + } } void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown) diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h index 1eccb2ac7d02..ae7ac0134c8c 100644 --- a/include/target/iscsi/iscsi_target_core.h +++ b/include/target/iscsi/iscsi_target_core.h @@ -575,6 +575,7 @@ struct iscsi_conn { spinlock_t nopin_timer_lock; spinlock_t response_queue_lock; spinlock_t state_lock; + spinlock_t unmap_cmd_lock; /* libcrypto RX and TX contexts for crc32c */ struct ahash_request *conn_rx_hash; struct ahash_request *conn_tx_hash;
A potential race condition may occur in iscsit_unmap_cmd() if the __iscsit_free_cmd() function is called by two different threads. This patch adds a spinlock to serialize the calls to iscsit_unmap_cmd() Signed-off-by: Maurizio Lombardi <mlombard@redhat.com> --- drivers/target/iscsi/iscsi_target_login.c | 1 + drivers/target/iscsi/iscsi_target_util.c | 5 ++++- include/target/iscsi/iscsi_target_core.h | 1 + 3 files changed, 6 insertions(+), 1 deletion(-)