| Message ID | 20210212072642.17520-3-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/25] target: move t_task_cdb initialization | expand |
On 2/12/21 1:26 AM, Mike Christie wrote: > The kref_get_unless_zero use in target_get_sess_cmd was added > in: > > commit 1b4c59b7a1d0 ("target: fix potential race window in > target_sess_cmd_list_waiting()")' > > but it does not seem to do anything. > > I think the original patch might have thought we could have added the > cmd to the sess_wait_list and then target_wait_for_sess_cmds could > do a put before target_get_sess_cmd did it's get. That wouldn't > happen because we do the get first then grab the sess lock and put > it on the list. > > It's also not needed now, because the sess_cmd_list does not exist > anymore and we instead wait on the session cmd_count. > > The other problem with the patch is that several > target_submit_cmd_map_sgls/ target_submit_cmd callers do not handle the > error case properly if it were to ever happen. These drivers think > they have their normal refcount on the cmd and in many cases do a > transport_generic_free_cmd plus target_put_sess_cmd so they would > have fired off the refcount WARN/BUGs. > > So this patch just changes the kref_get_unless_zero to kref_get. > > Signed-off-by: Mike Christie <michael.christie@oracle.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > drivers/target/target_core_transport.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 5c4adde96d5e..b5427e26187b 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -2775,9 +2775,7 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref) > * invocations before se_cmd descriptor release. > */ > if (ack_kref) { > - if (!kref_get_unless_zero(&se_cmd->cmd_kref)) > - return -EINVAL; > - > + kref_get(&se_cmd->cmd_kref); > se_cmd->se_cmd_flags |= SCF_ACK_KREF; > } > > Looks Good. Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 5c4adde96d5e..b5427e26187b 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -2775,9 +2775,7 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref) * invocations before se_cmd descriptor release. */ if (ack_kref) { - if (!kref_get_unless_zero(&se_cmd->cmd_kref)) - return -EINVAL; - + kref_get(&se_cmd->cmd_kref); se_cmd->se_cmd_flags |= SCF_ACK_KREF; }
The kref_get_unless_zero use in target_get_sess_cmd was added in: commit 1b4c59b7a1d0 ("target: fix potential race window in target_sess_cmd_list_waiting()")' but it does not seem to do anything. I think the original patch might have thought we could have added the cmd to the sess_wait_list and then target_wait_for_sess_cmds could do a put before target_get_sess_cmd did it's get. That wouldn't happen because we do the get first then grab the sess lock and put it on the list. It's also not needed now, because the sess_cmd_list does not exist anymore and we instead wait on the session cmd_count. The other problem with the patch is that several target_submit_cmd_map_sgls/ target_submit_cmd callers do not handle the error case properly if it were to ever happen. These drivers think they have their normal refcount on the cmd and in many cases do a transport_generic_free_cmd plus target_put_sess_cmd so they would have fired off the refcount WARN/BUGs. So this patch just changes the kref_get_unless_zero to kref_get. Signed-off-by: Mike Christie <michael.christie@oracle.com> Reviewed-by: Christoph Hellwig <hch@lst.de> --- drivers/target/target_core_transport.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)