[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH] block: nvme: correct the nvme queu
From: |
P J P |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH] block: nvme: correct the nvme queue id check |
Date: |
Sun, 23 Oct 2016 10:38:58 +0530 (IST) |
+-- On Sat, 22 Oct 2016, Peter Maydell wrote --+
| Secondly, it's almost the same as this cleanup
| patch from Thomas Huth that's already in qemu-trivial:
| http://patchwork.ozlabs.org/patch/681349/
|
| except that your version is removing the !
| negations from the return value.
|
| Can you explain in a bit more detail what the bug
| is here and why this is the right fix for it?
nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
{
return sqid < n->num_queues && n->sq[sqid] != NULL ? 0 : -1;
}
Both 'nvme_check_sqid' and 'nvme_check_cqid' return zero(0) when the
respective queue id's are valid and -1 when they are invalid. The '!' was
causing it to return invalid QID error when the check function returned zero.
The code seems incosistent quite a bit. The current check returns invalid QID
error for '!cqid' and '!sqid', but these IDs are used as index into 'n->cq'
array, not sure why index zero(0) is invalid. But then I found this in the
NVME specification[0]
"...One entry in each queue is not available for use due
to Head and Tail entry pointer definition."
Not sure if this one entry is at index zero(0). If so, there are other calls
to the 'nvme_check' functions which are not acompanyed with '!cqid' or
'!sqid' checks. Ex.
nvme_process_db
qid = (addr - 0x1000) >> 3;
if (nvme_check_sqid(n, qid)) {
return;
}
nvme_del_sq
if (!nvme_check_cqid(n, sq->cqid)) {
cq = n->cq[sq->cqid];
I haven't check these in further details yet. When 'cqid' is invalid it
returns 'NVME_INVALID_CQID' but when 'sqid' is invalid it returns
'NVME_INVALID_QID'. Not sure why not use 'NVME_INVALID_QID' for both.
[0]
http://www.nvmexpress.org/wp-content/uploads/NVM-Express-1_2-Gold-20141209.pdf
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F