| Message ID | 1647417702-129883-1-git-send-email-kanie@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi:target:tcmu: make sure dev blocked before resetting ring | expand |
On 3/16/22 17:01, Guixin Liu wrote: > If dev is not blocked when resetting ring, then there could be new > commands coming in after resetting ring, this will make cmd ring broken, > because tcmu can not find tcmu_cmd when tcmu-runner handled these > newcome commands. > > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> > --- > drivers/target/target_core_user.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c > index 7b2a89a..548ad94 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -2333,7 +2333,7 @@ static void tcmu_block_dev(struct tcmu_dev *udev) > mutex_unlock(&udev->cmdr_lock); > } > > -static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > +static int tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > { > struct tcmu_mailbox *mb; > struct tcmu_cmd *cmd; > @@ -2341,6 +2341,12 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > > mutex_lock(&udev->cmdr_lock); > > + if (!test_bit(TCMU_DEV_BIT_BLOCKED, &udev->flags)) { > + pr_err("The dev should be blocked before resetting ring.\n"); This looks like a bug... So I think this should at least be a WARN_ON(). E.g. if (WARN_ON(!test_bit(TCMU_DEV_BIT_BLOCKED, &udev->flags))) { mutex_unlock(&udev->cmdr_lock); return -EINVAL; } But why is the ring reset even allowed without the device being blocked in the first place ? What is the root cause ? > + mutex_unlock(&udev->cmdr_lock); > + return -EINVAL; > + } > + > xa_for_each(&udev->commands, i, cmd) { > pr_debug("removing cmd %u on dev %s from ring %s\n", > cmd->cmd_id, udev->name, > @@ -2396,6 +2402,7 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > run_qfull_queue(udev, false); > > mutex_unlock(&udev->cmdr_lock); > + return 0; > } > > enum { > @@ -2995,7 +3002,10 @@ static ssize_t tcmu_reset_ring_store(struct config_item *item, const char *page, > return -EINVAL; > } > > - tcmu_reset_ring(udev, val); > + ret = tcmu_reset_ring(udev, val); > + if (ret < 0) > + return ret; > + > return count; > } > CONFIGFS_ATTR_WO(tcmu_, reset_ring);
Hi, I don't think we need any fix here. 1) Generally userspace can easily violate the ring protocol. E.g., if userspace uses a currently not existing cmd_id in a cmd response, it forces tcmu into RING_BROKEN state. Resetting the ring while processing cmds is just another way to probably force tcmu into RING_BROKEN. Why should we try to fix just this one single situation? If userspace resets the ring, and after that continues to write responses to the ring for cmds it has read before the reset, we probably have to fix userspace app. 2) Resetting the ring without being blocked is _not_ forbidden. The situation you describe is more about resetting the ring while userspace holds the uio dev open. But even that is not useless, if userspace just stops to process cmds, resets the ring and then re-starts processing the ring, including to read the new head and tail pointers. Bodo On 16.03.22 09:01, Guixin Liu wrote: > If dev is not blocked when resetting ring, then there could be new > commands coming in after resetting ring, this will make cmd ring broken, > because tcmu can not find tcmu_cmd when tcmu-runner handled these > newcome commands. > > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> > --- > drivers/target/target_core_user.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c > index 7b2a89a..548ad94 100644 > --- a/drivers/target/target_core_user.c > +++ b/drivers/target/target_core_user.c > @@ -2333,7 +2333,7 @@ static void tcmu_block_dev(struct tcmu_dev *udev) > mutex_unlock(&udev->cmdr_lock); > } > > -static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > +static int tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > { > struct tcmu_mailbox *mb; > struct tcmu_cmd *cmd; > @@ -2341,6 +2341,12 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > > mutex_lock(&udev->cmdr_lock); > > + if (!test_bit(TCMU_DEV_BIT_BLOCKED, &udev->flags)) { > + pr_err("The dev should be blocked before resetting ring.\n"); > + mutex_unlock(&udev->cmdr_lock); > + return -EINVAL; > + } > + > xa_for_each(&udev->commands, i, cmd) { > pr_debug("removing cmd %u on dev %s from ring %s\n", > cmd->cmd_id, udev->name, > @@ -2396,6 +2402,7 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) > run_qfull_queue(udev, false); > > mutex_unlock(&udev->cmdr_lock); > + return 0; > } > > enum { > @@ -2995,7 +3002,10 @@ static ssize_t tcmu_reset_ring_store(struct config_item *item, const char *page, > return -EINVAL; > } > > - tcmu_reset_ring(udev, val); > + ret = tcmu_reset_ring(udev, val); > + if (ret < 0) > + return ret; > + > return count; > } > CONFIGFS_ATTR_WO(tcmu_, reset_ring);
diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c index 7b2a89a..548ad94 100644 --- a/drivers/target/target_core_user.c +++ b/drivers/target/target_core_user.c @@ -2333,7 +2333,7 @@ static void tcmu_block_dev(struct tcmu_dev *udev) mutex_unlock(&udev->cmdr_lock); } -static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) +static int tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) { struct tcmu_mailbox *mb; struct tcmu_cmd *cmd; @@ -2341,6 +2341,12 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) mutex_lock(&udev->cmdr_lock); + if (!test_bit(TCMU_DEV_BIT_BLOCKED, &udev->flags)) { + pr_err("The dev should be blocked before resetting ring.\n"); + mutex_unlock(&udev->cmdr_lock); + return -EINVAL; + } + xa_for_each(&udev->commands, i, cmd) { pr_debug("removing cmd %u on dev %s from ring %s\n", cmd->cmd_id, udev->name, @@ -2396,6 +2402,7 @@ static void tcmu_reset_ring(struct tcmu_dev *udev, u8 err_level) run_qfull_queue(udev, false); mutex_unlock(&udev->cmdr_lock); + return 0; } enum { @@ -2995,7 +3002,10 @@ static ssize_t tcmu_reset_ring_store(struct config_item *item, const char *page, return -EINVAL; } - tcmu_reset_ring(udev, val); + ret = tcmu_reset_ring(udev, val); + if (ret < 0) + return ret; + return count; } CONFIGFS_ATTR_WO(tcmu_, reset_ring);
If dev is not blocked when resetting ring, then there could be new commands coming in after resetting ring, this will make cmd ring broken, because tcmu can not find tcmu_cmd when tcmu-runner handled these newcome commands. Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> --- drivers/target/target_core_user.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)