linux-kernel - Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <528D7E8A-B2F6-4EDB-A7C0-E0BF494038D0@xyratex.com>
Date:	Thu, 8 Mar 2012 08:32:09 -0500
From:	Mark Salyzyn <mark_salyzyn@...atex.com>
To:	santosh nayak <santoshprasadnayak@...il.com>
Cc:	Mark Salyzyn <mark_salyzyn@...atex.com>,
	Jack Wang <jack_wang@...sh.com>,
	lindar_liu <lindar_liu@...sh.com>,
	James Bottomley <JBottomley@...allels.com>,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org,
	Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH 1/2] [SCSI] pm8001: Fix bogus interrupt state flag issue.

NAK

In the fragment below, the 'spin_lock_irqsave(&t->task_state_lock, flags);' is 100% legitimate. The change you did was not inert, and result in the IRQ being disabled upon exit should a SAS_TASK_STATE_ABORTED task state condition occur.

I propose (annotated below) you leave the flags automatic variable, but uninitialized. The changes surrounding spin_*lock_irq*(&pm8001_ha->lock, flags) are OK, but revert back the changes surrounding the spin_*lock_irq*(&t->task->state_lock,flags) so that lock would be properly nested.

To be pedantic, and to be more correct, this code should have spawned a work queue task to perform the t->task_done(t) operation in a separate thread rather than inline and precariously unlocked; but that would be beyond the scope of these changes and should be left for future work to decide if it is even necessary. Not sure how such a change would affect performance (using the work queue) ... so I am 'afraid' of pushing such a change in any case given the relatively reliable operation of this driver (and the dynamic nature of the changes Dan is making to the SATA side of things for instance ;-} ).

I am sorry for taking so long to get to the task of reviewing this (and the previous) code. I look forward to your comments.

Sincerely -- Mark Salyzyn

On Feb 26, 2012, at 8:33 AM, santosh nayak wrote:

> @@ -2207,7 +2206,6 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
> static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb)
> {
> 	struct sas_task *t;
> -	unsigned long flags = 0;
MGS>	unsigned long flags;
> 	struct task_status_struct *ts;
> 	struct pm8001_ccb_info *ccb;
> 	struct pm8001_device *pm8001_dev;
> @@ -2287,9 +2285,9 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb)
> 			ts->stat = SAS_QUEUE_FULL;
> 			pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> 			mb();/*ditto*/
> -			spin_unlock_irqrestore(&pm8001_ha->lock, flags);
> +			spin_unlock_irq(&pm8001_ha->lock);
> 			t->task_done(t);
> -			spin_lock_irqsave(&pm8001_ha->lock, flags);
> +			spin_lock_irq(&pm8001_ha->lock);
> 			return;
> 		}
> 		break;
> @@ -2387,31 +2385,31 @@ static void mpi_sata_event(struct pm8001_hba_info *pm8001_ha , void *piomb)
> 		ts->stat = SAS_OPEN_TO;
> 		break;
> 	}
> -	spin_lock_irqsave(&t->task_state_lock, flags);
> +	spin_lock_irq(&t->task_state_lock);
MGS>	spin_lock_irqsave(&t->task_state_lock, flags);
> 	t->task_state_flags &= ~SAS_TASK_STATE_PENDING;
> 	t->task_state_flags &= ~SAS_TASK_AT_INITIATOR;
> 	t->task_state_flags |= SAS_TASK_STATE_DONE;
> 	if (unlikely((t->task_state_flags & SAS_TASK_STATE_ABORTED))) {
> -		spin_unlock_irqrestore(&t->task_state_lock, flags);
> +		spin_unlock_irq(&t->task_state_lock);
MGS>		spin_unlock_irqrestore(&t->task_state_lock, flags);
> 		PM8001_FAIL_DBG(pm8001_ha,
> 			pm8001_printk("task 0x%p done with io_status 0x%x"
> 			" resp 0x%x stat 0x%x but aborted by upper layer!\n",
> 			t, event, ts->resp, ts->stat));
> 		pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> 	} else if (t->uldd_task) {
> -		spin_unlock_irqrestore(&t->task_state_lock, flags);
> +		spin_unlock_irq(&t->task_state_lock);
> 		pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> 		mb();/* ditto */
> -		spin_unlock_irqrestore(&pm8001_ha->lock, flags);
> +		spin_unlock_irq(&pm8001_ha->lock);
> 		t->task_done(t);
> -		spin_lock_irqsave(&pm8001_ha->lock, flags);
> +		spin_lock_irq(&pm8001_ha->lock);
> 	} else if (!t->uldd_task) {
> -		spin_unlock_irqrestore(&t->task_state_lock, flags);
> +		spin_unlock_irq(&t->task_state_lock);
> 		pm8001_ccb_task_free(pm8001_ha, t, ccb, tag);
> 		mb();/*ditto*/
> -		spin_unlock_irqrestore(&pm8001_ha->lock, flags);
> +		spin_unlock_irq(&pm8001_ha->lock);
> 		t->task_done(t);
> -		spin_lock_irqsave(&pm8001_ha->lock, flags);
> +		spin_lock_irq(&pm8001_ha->lock);
> 	}
> }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/