linux-kernel - Re: [PATCH v3 1/2] virtio-scsi: first version

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:	Mon, 19 Dec 2011 14:11:42 +0100
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Hannes Reinecke <hare@...e.de>
CC:	linux-kernel@...r.kernel.org,
	"Michael S. Tsirkin" <mst@...hat.com>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Stefan Hajnoczi <stefanha@...ux.vnet.ibm.com>,
	Mike Christie <michaelc@...wisc.edu>
Subject: Re: [PATCH v3 1/2] virtio-scsi: first version

On 12/19/2011 01:22 PM, Hannes Reinecke wrote:
>> +	case VIRTIO_SCSI_S_UNDERRUN:
>> +		set_host_byte(sc, DID_ERROR);
>> +		break;
> Hmm. Not sure this is correct.
> UNDERRUN could be a valid state, eg it is quite common to send
> an INQUIRY with a length of 255 bytes. And only evaluate the
> bytes which are actually send back.

This happens when you send an INQUIRY with a sglist of 255 bytes and an 
ALLOCATION LENGTH of 300 bytes.  The spec says "VIRTIO_SCSI_S_UNDERRUN 
[is returned] if the content of the CDB requires transferring more data 
than is available in the data buffers".

> And, it's not good style to match the same DID_XXX error code to
> several internal errors. I would prefer having a 1:1 match for
> VIRTIO_SCSI_S_XXX and DID_XXX error codes.

There is no DID_XXX value for underrun, but it's quite useful to have it 
diagnosed separately in traces.

>> +static struct scsi_host_template virtscsi_host_template = {
>> +	.module = THIS_MODULE,
>> +	.name = "Virtio SCSI HBA",
>> +	.proc_name = "virtio_scsi",
>> +	.queuecommand = virtscsi_queuecommand,
>> +	.this_id = -1,
>> +
>> +	.can_queue = 1024,
>> +	.dma_boundary = UINT_MAX,
>> +	.use_clustering = ENABLE_CLUSTERING,
>> +};
>> +
> Hmm. Apparently you support more than one LUN, yet no
> ->slave_alloc() callback is present. So how do you scan the bus here?

What does slave_alloc have to do with scanning?  I don't need hostdata, 
so I don't have slave_alloc.

Scanning is here:

         err = scsi_add_host(shost, &vdev->dev);
         if (err)
                 goto scsi_add_host_failed;

         scsi_scan_host(shost);

> Do you actually have a working backend implementation?

Hmm... yes, of course...

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