On Wed, 2013-09-11 at 09:43 +0300, Dan Carpenter wrote:
> On Tue, Sep 10, 2013 at 03:51:50PM -0700, Nicholas A. Bellinger wrote:
> > diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> > index 2572ba7..6c17295 100644
> > --- a/drivers/target/target_core_sbc.c
> > +++ b/drivers/target/target_core_sbc.c
> > @@ -504,12 +504,15 @@ sbc_compare_and_write(struct se_cmd *cmd)
> > * comparision using SGLs at cmd->t_bidi_data_sg..
> > */
> > rc = down_interruptible(&dev->caw_sem);
> > - if ((rc != 0) || signal_pending(current))
> > + if ((rc != 0) || signal_pending(current)) {
> > + cmd->transport_complete_callback = NULL;
> > return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>
> I think this needs to be separate:
>
> if (rc) {
> cmd->transport_complete_callback = NULL;
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
> if (signal_pending(current)) {
> up(&dev->caw_sem);
> cmd->transport_complete_callback = NULL;
> return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> }
>
After double checking what kernel/semaphore.c:down_common() does, I
don't believe that up() should be called here for either case.
Namely, consider when sem->count == 0 enters __down_interruptible() ->
down_common():
static inline int __sched __down_common(struct semaphore *sem, long state,
long timeout)
{
struct task_struct *task = current;
struct semaphore_waiter waiter;
list_add_tail(&waiter.list, &sem->wait_list);
waiter.task = task;
waiter.up = false;
for (;;) {
if (signal_pending_state(state, task))
goto interrupted;
if (unlikely(timeout <= 0))
goto timed_out;
__set_task_state(task, state);
raw_spin_unlock_irq(&sem->lock);
timeout = schedule_timeout(timeout);
raw_spin_lock_irq(&sem->lock);
if (waiter.up)
return 0;
}
timed_out:
list_del(&waiter.list);
return -ETIME;
interrupted:
list_del(&waiter.list);
return -EINTR;
}
and when a signal causes down_common() to return -EINTR above, the
waiter is deleted from the list.
So for the down_interruptible() failure case in sbc_compare_and_write(),
performing an explicit up() after a non zero rc or signal_pending()
would incorrectly increment sem->count after the (failed) waiter was
already removed from the list in down_common().
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
