Re: [Qemu-devel] [PATCH] block: nvme: correct the nvme queue id check

[P J P <ppandit@redhat.com>]

+-- 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