linux-kernel - Re: [PATCH] SCSI driver for VMware's virtual HBA

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]

Date:	Thu, 3 Sep 2009 23:18:53 +0200
From:	Rolf Eike Beer <eike-kernel@...tec.de>
To:	akataria@...are.com
Cc:	James Bottomley <James.Bottomley@...e.de>,
	Matthew Wilcox <matthew@....cx>,
	Roland Dreier <rdreier@...co.com>,
	Bart Van Assche <bvanassche@....org>,
	Robert Love <robert.w.love@...el.com>,
	Randy Dunlap <randy.dunlap@...cle.com>,
	Mike Christie <michaelc@...wisc.edu>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	pv-drivers@...are.com
Subject: Re: [PATCH] SCSI driver for VMware's virtual HBA - V3.

Alok Kataria wrote:
> VMware PVSCSI driver - v3.
> 
> +static void pvscsi_write_cmd_desc(const struct pvscsi_adapter *adapter,
> +				  u32 cmd, const void *desc, size_t len)
> +{
> +	const u32 *ptr = desc;
> +	unsigned i;
> +
> +	len /= sizeof(*ptr);
> +	pvscsi_reg_write(adapter, PVSCSI_REG_OFFSET_COMMAND, cmd);
> +	for (i = 0; i < len; i++)

This would limit len to 16 GiB as "unsigned" (looks like "unsigned int" is 
preferred in kernel) is only 32 bit long. Instead i should probably be size_t, 
too.

> +static int __devinit pvscsi_allocate_rings(struct pvscsi_adapter *adapter)
> +{

...

> +	BUG_ON(adapter->ringStatePA & ~PAGE_MASK);
> +	BUG_ON(adapter->reqRingPA   & ~PAGE_MASK);
> +	BUG_ON(adapter->cmpRingPA   & ~PAGE_MASK);

Ok, that took me a moment. How about BUG_ON(!IS_ALIGNED(foo, PAGE_SIZE))?

> +static const char *pvscsi_info(struct Scsi_Host *host)
> +{
> +	struct pvscsi_adapter *adapter = shost_priv(host);
> +	static char buf[512];
> +
> +	sprintf(buf, "VMware PVSCSI storage adapter rev %d, req/cmp/msg rings: "
> +		"%u/%u/%u pages, cmd_per_lun=%u", adapter->rev,
> +		adapter->req_pages, adapter->cmp_pages, adapter->msg_pages,
> +		pvscsi_template.cmd_per_lun);

The buffer looks rather huge here. I'm too lazy to calculate an exact size but 
256 should still be more than enough, no?

> +static void pvscsi_shutdown_intr(struct pvscsi_adapter *adapter)
> +{
> +	if (adapter->irq) {
> +		free_irq(adapter->irq, adapter);
> +		adapter->irq = 0;
> +	}
> +	if (adapter->use_msi) {
> +		pci_disable_msi(adapter->dev);
> +		adapter->use_msi = 0;
> +	}
> +
> +	if (adapter->use_msix) {
> +		pci_disable_msix(adapter->dev);
> +		adapter->use_msix = 0;
> +	}
> +}

If MSI and MSI-X are exclusive the last one could be "else if".

Greetings,

Eike

Download attachment "signature.asc " of type "application/pgp-signature" (199 bytes)