| 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
| ||
|
Date: Mon, 13 Jun 2022 09:13:34 +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 08/18] libata: Queue ATA internal commands as
requests
On 13/06/2022 08:22, Damien Le Moal wrote:
> On 6/9/22 19:29, John Garry wrote:
>> Follow the normal path for requests and queue through the block layer.
>>
>> We hold the qc pointer in the scmd host scribble, which is less than
>> ideal. In future we can hold the ata_queued_cmd in the scmd priv_data,
>> allowing for easy lookup.
>>
>> We need to use the shost sdev to queue these requests. That is because we
>> initially do not have the target disk scsi_device allocated yet when
>> queueing internal commands. This is less than ideal, and makes it hard for
>> libsas to know when queueing an internal command whether it is a ATA
>> internal command or not.
>>
>> Also make ata_exec_internal_sg() static - there are no users outside
>> libata-core.c
>>
>> Signed-off-by: John Garry <john.garry@...wei.com>
>> ---
>> drivers/ata/libata-core.c | 137 +++++++++++++++++++++-----------------
>> drivers/ata/libata-sata.c | 5 +-
>> drivers/ata/libata.h | 4 --
>> 3 files changed, 80 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 40e816419f48..6b1aaeccb253 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -1438,9 +1438,18 @@ unsigned long ata_id_xfermask(const u16 *id)
>> }
>> EXPORT_SYMBOL_GPL(ata_id_xfermask);
>>
>> -static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
>> +void ata_qc_complete_internal(struct ata_queued_cmd *qc)
>> {
>> - struct completion *waiting = qc->private_data;
>> + struct scsi_cmnd *scmd = qc->scsicmd;
>> +
>> + scsi_done(scmd);
>> +}
>> +
>> +static void ata_internal_end_rq(struct request *rq, blk_status_t error)
>> +{
>> + struct completion *waiting = rq->end_io_data;
>> +
>> + rq->end_io_data = (void *)(uintptr_t)error;
>>
>> complete(waiting);
>> }
>> @@ -1467,52 +1476,74 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
>> * RETURNS:
>> * Zero on success, AC_ERR_* mask on failure
>> */
>> -unsigned ata_exec_internal_sg(struct ata_device *dev,
>> +static unsigned ata_exec_internal_sg(struct ata_device *dev,
>> struct ata_taskfile *tf, const u8 *cdb,
>> - int dma_dir, struct scatterlist *sgl,
>> - unsigned int n_elem, unsigned long timeout)
>> + int dma_dir, void *buf, unsigned int buflen,
>> + unsigned long timeout)
>> {
>> struct ata_link *link = dev->link;
>> struct ata_port *ap = link->ap;
>> + struct Scsi_Host *scsi_host = ap->scsi_host;
>> + struct scsi_device *sdev = scsi_host->sdev;
>> u8 command = tf->command;
>> int auto_timeout = 0;
>> struct ata_queued_cmd *qc;
>> - unsigned int preempted_tag;
>> - u32 preempted_sactive;
>> - u64 preempted_qc_active;
>> - int preempted_nr_active_links;
>> DECLARE_COMPLETION_ONSTACK(wait);
>> unsigned long flags;
>> unsigned int err_mask;
>> + struct scsi_cmnd *scmd;
>> + struct request *req;
>> int rc;
>>
>> - spin_lock_irqsave(ap->lock, flags);
>> + /*
>> + * We only support a single reserved command, so this guarantees
>> + * serialization. However the code already assumed that (we are
>> + * serialized here per-port).
>> + */
>> + req = scsi_alloc_request(sdev->request_queue,
>> + dma_dir == DMA_TO_DEVICE ?
>> + REQ_OP_DRV_OUT : REQ_OP_DRV_IN,
>> + BLK_MQ_REQ_RESERVED);
>> + if (IS_ERR(req))
>> + return AC_ERR_OTHER;
>>
>> - /* no internal command while frozen */
>> - if (ap->pflags & ATA_PFLAG_FROZEN) {
>> - spin_unlock_irqrestore(ap->lock, flags);
>> - return AC_ERR_SYSTEM;
>> +
>> + if (!timeout) {
>> + if (ata_probe_timeout)
>> + timeout = ata_probe_timeout * 1000;
>> + else {
>> + timeout = ata_internal_cmd_timeout(dev, command);
>> + auto_timeout = 1;
>> + }
>> }
>>
>> - /* initialize internal qc */
>> + scmd = blk_mq_rq_to_pdu(req);
>> + scmd->allowed = 0;
>> + req->timeout = timeout;
>> + //TODO: Hook up timeout handler
>> + req->rq_flags |= RQF_QUIET;
>> + scmd->device = sdev;
>> qc = __ata_qc_from_tag(ap, ATA_TAG_INTERNAL);
>>
>> + /* Do this until we can hold ata_queued_cmd in the SCMD priv data */
>> + scmd->host_scribble = (unsigned char *)qc;
>> +
>> + if (buflen) {
>> + int ret = blk_rq_map_kern(sdev->request_queue, req,
>> + buf, buflen, GFP_NOIO);
>> + if (ret) {
>> + blk_mq_free_request(req);
>> + return AC_ERR_OTHER;
>> + }
>> + }
>> +
>> qc->tag = ATA_TAG_INTERNAL;
>> qc->hw_tag = 0;
>> - qc->scsicmd = NULL;
>> + qc->scsicmd = scmd;
>> qc->ap = ap;
>> qc->dev = dev;
>> ata_qc_reinit(qc);
>>
>> - preempted_tag = link->active_tag;
>> - preempted_sactive = link->sactive;
>> - preempted_qc_active = ap->qc_active;
>> - preempted_nr_active_links = ap->nr_active_links;
>> - link->active_tag = ATA_TAG_POISON;
>> - link->sactive = 0;
>> - ap->qc_active = 0;
>> - ap->nr_active_links = 0;
>> -
>> /* prepare & issue qc */
>> qc->tf = *tf;
>> if (cdb)
>> @@ -1525,32 +1556,14 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
>>
>> qc->flags |= ATA_QCFLAG_RESULT_TF;
>> qc->dma_dir = dma_dir;
>> - if (dma_dir != DMA_NONE) {
>> - unsigned int i, buflen = 0;
>> - struct scatterlist *sg;
>> -
>> - for_each_sg(sgl, sg, n_elem, i)
>> - buflen += sg->length;
>> -
>> - ata_sg_init(qc, sgl, n_elem);
>> - qc->nbytes = buflen;
>> - }
>>
>> - qc->private_data = &wait;
>> + qc->private_data = ap;
>> qc->complete_fn = ata_qc_complete_internal;
>>
>> - ata_qc_issue(qc);
>> -
>> - spin_unlock_irqrestore(ap->lock, flags);
>> + req->end_io_data = &wait;
>> + req->end_io = ata_internal_end_rq;
>>
>> - if (!timeout) {
>> - if (ata_probe_timeout)
>> - timeout = ata_probe_timeout * 1000;
>> - else {
>> - timeout = ata_internal_cmd_timeout(dev, command);
>> - auto_timeout = 1;
>> - }
>> - }
Hi Damien,
>> + blk_execute_rq_nowait(req, true);
>
> How do you get guarantee that this req ends up being issued with
> ATA_TAG_INTERNAL as the tag ?
We don't, but it's not important, as we don't use the request tag index
in libata to infer that a request is an ATA_TAG_INTERNAL request. Nobody
should assume that the tag index for the ATA_TAG_INTERNAL req is 32. We
have other methods to check that, like using scsi_is_reserved_cmd() and
continuing to use __ata_qc_from_tag(ap, ATA_TAG_INTERNAL) to get the ATA
internal qc.
But it is a good question, as if we start to put the ata_queued_cmd
structure in the SCSI cmnd priv data - as suggested by Hannes - then
__ata_qc_from_tag(ap, ATA_TAG_INTERNAL) may need to be reworked to
handle that.
> Because you have the reserved commands *in
> addition to can queue* ? I can see how that works if can_queue is indeed
> 32, but what if the user changes the max qd ? That breaks, no ?
Thanks,
john
>
>>
>> if (ap->ops->error_handler)
>> ata_eh_release(ap);
>> @@ -1610,13 +1623,15 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
>> err_mask = qc->err_mask;
>>
>> ata_qc_free(qc);
>> - link->active_tag = preempted_tag;
>> - link->sactive = preempted_sactive;
>> - ap->qc_active = preempted_qc_active;
>> - ap->nr_active_links = preempted_nr_active_links;
>> + link->active_tag = link->preempted_tag;
>> + link->sactive = link->preempted_sactive;
>> + ap->qc_active = ap->preempted_qc_active;
>> + ap->nr_active_links = ap->preempted_nr_active_links;
>>
>> spin_unlock_irqrestore(ap->lock, flags);
>>
>> + blk_mq_free_request(req);
>> +
>> if ((err_mask & AC_ERR_TIMEOUT) && auto_timeout)
>> ata_internal_cmd_timed_out(dev, command);
>>
>> @@ -1647,18 +1662,20 @@ unsigned ata_exec_internal(struct ata_device *dev,
>> int dma_dir, void *buf, unsigned int buflen,
>> unsigned long timeout)
>> {
>> - struct scatterlist *psg = NULL, sg;
>> - unsigned int n_elem = 0;
>> + /* buf may not be aligned, so copy to/from an aligned buffer */
>> + void *tmpbuf = kmemdup(buf, buflen, GFP_KERNEL);
>> + unsigned res;
>>
>> - if (dma_dir != DMA_NONE) {
>> - WARN_ON(!buf);
>> - sg_init_one(&sg, buf, buflen);
>> - psg = &sg;
>> - n_elem++;
>> - }
>> + if (!tmpbuf)
>> + return AC_ERR_OTHER;
>>
>> - return ata_exec_internal_sg(dev, tf, cdb, dma_dir, psg, n_elem,
>> + res = ata_exec_internal_sg(dev, tf, cdb, dma_dir, tmpbuf, buflen,
>> timeout);
>> +
>> + memcpy(buf, tmpbuf, buflen);
>> + kfree(tmpbuf);
>> +
>> + return res;
>> }
>>
>> /**
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index 7a5fe41aa5ae..3cecc45d54ab 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -1258,9 +1258,10 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
>> {
>> int rc = 0;
>>
>> - if (likely(ata_dev_enabled(ap->link.device)))
>> + if (likely(ata_dev_enabled(ap->link.device)) ||
>> + scsi_is_reserved_cmd(cmd)) {
>> rc = __ata_scsi_queuecmd(cmd, ap->link.device);
>> - else {
>> + } else {
>> cmd->result = (DID_BAD_TARGET << 16);
>> scsi_done(cmd);
>> }
>> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
>> index 926a7f41303d..1446a482835d 100644
>> --- a/drivers/ata/libata.h
>> +++ b/drivers/ata/libata.h
>> @@ -53,10 +53,6 @@ extern unsigned ata_exec_internal(struct ata_device *dev,
>> struct ata_taskfile *tf, const u8 *cdb,
>> int dma_dir, void *buf, unsigned int buflen,
>> unsigned long timeout);
>> -extern unsigned ata_exec_internal_sg(struct ata_device *dev,
>> - struct ata_taskfile *tf, const u8 *cdb,
>> - int dma_dir, struct scatterlist *sg,
>> - unsigned int n_elem, unsigned long timeout);
>> extern int ata_wait_ready(struct ata_link *link, unsigned long deadline,
>> int (*check_ready)(struct ata_link *link));
>> extern int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>
>
Powered by blists - more mailing lists