linux-kernel - Re: please fix FUSION (Was: [v3.13][v3.14][Regression]kthread:makekthread

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: <20140324170115.GA30307@redhat.com>
Date:	Mon, 24 Mar 2014 18:01:15 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	James Bottomley <James.Bottomley@...senPartnership.com>
Cc:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	joseph.salisbury@...onical.com, Nagalakshmi.Nandigama@....com,
	Sreekanth.Reddy@....com, rientjes@...gle.com,
	akpm@...ux-foundation.org, torvalds@...ux-foundation.org,
	tj@...nel.org, tglx@...utronix.de, linux-kernel@...r.kernel.org,
	kernel-team@...ts.ubuntu.com, linux-scsi@...r.kernel.org,
	thenzl@...hat.com
Subject: Re: please fix FUSION (Was:
	[v3.13][v3.14][Regression]kthread:makekthread_create()killable)

On 03/22, James Bottomley wrote:
>
> OK, the fix from the SCSI point of view is to make the mpt teardown path
> actually work.  The whole thing looks to be a complete mess because
> there's another place where it will do the wrong thing in
> mptscsih_remove().  You always have to call mpt_detach() otherwise the
> device doesn't get removed from the lists.  In theory this patch fixes
> both bugs in the driver.

Yes, I obviously thought about the same change ;)

But note that mpt_detach() doesn't look correct too. This is almost
off-topic, but still...

At least it needs s/cancel_delayed_work/cancel_delayed_work_sync/ to
sync with mpt_fault_reset_work().

And it is not clear if it is safe to simply destroy ->fw_event_q,
perhaps it can't have the pending works if the caller is teardown
path, and otherwise we rely on mptsas_cleanup_fw_event_q(), I do
not know... but mptsas_cleanup_fw_event_q() needs _sync too I guess,
and obviously mptsas_free_fw_event() should be called unconditionally.

OTOH, mpt_attach() doesn't verify that ->fw_event_q was created
succesfully. And, if mpt_do_ioc_recovery() fails it destroys
->reset_work_q but not ->fw_event_q. Looks like this driver needs
some cleanups.

> --- a/drivers/message/fusion/mptscsih.c
> +++ b/drivers/message/fusion/mptscsih.c
> @@ -1176,10 +1176,14 @@ mptscsih_remove(struct pci_dev *pdev)
>  	MPT_SCSI_HOST		*hd;
>  	int sz1;
>
> +	if (!host)
> +		/* not brought up far enough to do scsi_host_attach() */
                                                   ^^^^^^^^^^^^^^^^
scsi_host_alloc ;)

> +		goto out;
> +

Yes, I think this is correct. mpt_attach() does kzalloc(MPT_ADAPTER) so
we can probably rely on ->sh == NULL if _alloc failed.

Oleg.

--
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/