linux-kernel - Re: [PATCH RFC v2 04/18] scsi: core: Add support to send reserved commands

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: <04540488-ec07-3bc2-a997-b7f64b0ba606@huawei.com>
Date:   Mon, 13 Jun 2022 10:40:46 +0100
From:   John Garry <john.garry@...wei.com>
To:     Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        <axboe@...nel.dk>, <jejb@...ux.ibm.com>,
        <martin.petersen@...cle.com>, <brking@...ibm.com>, <hare@...e.de>,
        <hch@....de>
CC:     <linux-block@...r.kernel.org>, <linux-ide@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
        <chenxiang66@...ilicon.com>
Subject: Re: [PATCH RFC v2 04/18] scsi: core: Add support to send reserved
 commands

On 13/06/2022 08:03, Damien Le Moal wrote:
>> +	if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) {
>> +		shost_printk(KERN_ERR, shost,
>> +			"nr_reserved_cmds set but no method to queue\n");
>> +		goto fail;
> This would be a driver implementation bug.

It would be a driver bug, but it probably makes the driver utterly 
useless and there is no point in continuing (to try to add). If the 
driver supports reserved commands then they are prob essential to make 
the driver function.

> So what about a WARN() here ?

Maybe but I really do not see a point in continuing

> 
>> +	}
>> +
>>   	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
>>   	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
>>   				   shost->can_queue);
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index f6e53c6d913c..8c8b4c6767d9 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1422,6 +1422,16 @@ static void scsi_complete(struct request *rq)
>>   	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>>   	enum scsi_disposition disposition;
>>   
>> +	if (scsi_is_reserved_cmd(cmd)) {
>> +		struct scsi_device *sdev = cmd->device;
>> +
>> +		scsi_mq_uninit_cmd(cmd);
>> +		scsi_device_unbusy(sdev, cmd);
>> +		__blk_mq_end_request(rq, 0);
>> +
>> +		return;
>> +	}
>> +
>>   	INIT_LIST_HEAD(&cmd->eh_entry);
>>   
>>   	atomic_inc(&cmd->device->iodone_cnt);
>> @@ -1706,6 +1716,28 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>>   
>>   	WARN_ON_ONCE(cmd->budget_token < 0);
>>   
>> +	if (scsi_is_reserved_cmd(cmd)) {
>> +		unsigned char *host_scribble = cmd->host_scribble;
>> +
>> +		if (!(req->rq_flags & RQF_DONTPREP)) {
>> +			ret = scsi_prepare_cmd(req);
>> +			if (ret != BLK_STS_OK) {
>> +
> Stray blank line.

ok

> 
>> +				goto out_dec_host_busy;
>> +			}
> No need for the curly brackets here.

ok

> 
>> +
>> +			req->rq_flags |= RQF_DONTPREP;
>> +		} else {
>> +			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
>> +		}

Thanks,
John