linux-kernel - [PATCH v2] nvmet: fix mar and mor off-by-one errors

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] [day] [month] [year] [list]

Message-ID: <B683848E-46D3-403D-91BE-CE0C4B3C5841@wdc.com>
Date:   Wed, 7 Sep 2022 07:04:12 +0000
From:   Dennis Maisenbacher <dennis.maisenbacher@....com>
To:     Chaitanya Kulkarni <chaitanyak@...dia.com>,
        Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>
CC:     Niklas Cassel <Niklas.Cassel@....com>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>,
        Damien Le Moal <Damien.LeMoal@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: [PATCH v2] nvmet: fix mar and mor off-by-one errors

>>>>     if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
>>>>             req->error_loc = offsetof(struct nvme_identify, nsid);
>>>> @@ -130,8 +131,20 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>>>>     zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
>>>>                                     req->ns->blksize_shift;
>>>>     id_zns->lbafe[0].zsze = cpu_to_le64(zsze);
>>>> -   id_zns->mor = cpu_to_le32(bdev_max_open_zones(req->ns->bdev));
>>>> -   id_zns->mar = cpu_to_le32(bdev_max_active_zones(req->ns->bdev));
>>>> +
>>>> +   mor = bdev_max_open_zones(req->ns->bdev);
>>>> +   if (!mor)
>>>> +           mor = U32_MAX;
>>>> +   else
>>>> +           --mor;
>>>> +   id_zns->mor = cpu_to_le32(mor);
>>>> +
>>>> +   mar = bdev_max_active_zones(req->ns->bdev);
>>>> +   if (!mar)
>>>> +           mar = U32_MAX;
>>>> +   else
>>>> +           --mar;
>>>> +   id_zns->mar = cpu_to_le32(mar);
>>>>
>>>
>>> above 14 lines of code can be simplified as in 4-5 lines :-
>>
>> Simplified ? It is much harder to read in my opinion...
>>
>>>
>
>there are two if ... else ... doing identical things on same data
>type u32 and its return type is also same le32,
>if my suggestion is hard to read then common code needs
>to be moved to the helper as it is not clear the need for
>code duplication from commit message.

For my taste the conditional operator would have looked like a good
tradeoff in this case. It condenses the code and verbosely converts a 0 to
U32_MAX. A new helper would have read like the conditional operator.

Christoph already applied the patch. In any case thanks for your comments! 

Dennis