Hi,
Thanks for your advice.
Previously, g_malloc0 was used, but a segmentation
fault occurred during testing. Later, debugging
revealed that a field in struct NvmeReservationStatusExt,
struct NvmeRegisteredCtrlExt regctl_eds[], was a
variable-length array. Therefore, g_malloc was used
to apply after the array length was determined.
Therefore, g_malloc0 was not used here.
On 2024/8/28 14:10, Klaus Jensen wrote:
> On Jul 12 10:36, Changqi Lu wrote:
>> Add reservation acquire, reservation register,
>> reservation release and reservation report commands
>> in the nvme device layer.
>>
>> By introducing these commands, this enables the nvme
>> device to perform reservation-related tasks, including
>> querying keys, querying reservation status, registering
>> reservation keys, initiating and releasing reservations,
>> as well as clearing and preempting reservations held by
>> other keys.
>>
>> These commands are crucial for management and control of
>> shared storage resources in a persistent manner.
>> Signed-off-by: Changqi Lu
>> Signed-off-by: zhenwei pi
>> Acked-by: Klaus Jensen
>> ---
>> hw/nvme/ctrl.c | 318 +++++++++++++++++++++++++++++++++++++++++++
>> hw/nvme/nvme.h | 4 +
>> include/block/nvme.h | 37 +++++
>> 3 files changed, 359 insertions(+)
>>
>
>> +static int nvme_read_reservation_cb(NvmeReadReservation *reservation)
>> +{
>> + int rc;
>> + NvmeReservationStatus *nvme_status;
>> + NvmeRequest *req = reservation->req;
>> + NvmeCtrl *n = req->sq->ctrl;
>> + NvmeResvKeys *keys_info = reservation->keys_info;
>> + int len = sizeof(NvmeReservationStatusHeader) +
>> + sizeof(NvmeRegisteredCtrl) * keys_info->num_keys;
>> +
>> + nvme_status = g_malloc(len);
>> + nvme_status->header.gen = reservation->generation;
>> + nvme_status->header.rtype = block_pr_type_to_nvme(reservation->type);
>> + nvme_status->header.regctl = keys_info->num_keys;
>> + for (int i = 0; i < keys_info->num_keys; i++) {
>> + nvme_status->regctl_ds[i].cntlid = nvme_ctrl(req)->cntlid;
>> + nvme_status->regctl_ds[i].rkey = keys_info->keys[i];
>> + nvme_status->regctl_ds[i].rcsts = keys_info->keys[i] ==
>> + reservation->key ? 1 : 0;
>> + /* hostid is not supported currently */
>> + memset(&nvme_status->regctl_ds[i].hostid, 0, 8);
>> + }
>> +
>> + rc = nvme_c2h(n, (uint8_t *)nvme_status, len, req);
>> + g_free(nvme_status);
>> + return rc;
>> +}
>> +
>> +static int nvme_read_reservation_ext_cb(NvmeReadReservation
*reservation)
>> +{
>> + int rc;
>> + NvmeReservationStatusExt *nvme_status_ext;
>> + NvmeRequest *req = reservation->req;
>> + NvmeCtrl *n = req->sq->ctrl;
>> + NvmeResvKeys *keys_info = reservation->keys_info;
>> + int len = sizeof(NvmeReservationStatusHeader) +
>> + sizeof(uint8_t) * 40 +
>> + sizeof(NvmeRegisteredCtrlExt) * keys_info->num_keys;
>> +
>> + nvme_status_ext = g_malloc(len);
>
> This leaks heap memory due to uninitialized reserved fields in
> NvmeReservationStatusExt. Prefer g_malloc0.
>
> The one above in nvme_read_reservation_cb looks safe, but prefer
> g_malloc0 there anyway.
>
>> + nvme_status_ext->header.gen = cpu_to_be32(reservation->generation);
>> + nvme_status_ext->header.rtype =
block_pr_type_to_nvme(reservation->type);
>> + nvme_status_ext->header.regctl = cpu_to_be16(keys_info->num_keys);
>> +
>> + for (int i = 0; i < keys_info->num_keys; i++) {
>> + uint16_t ctnlid = nvme_ctrl(req)->cntlid;
>> + nvme_status_ext->regctl_eds[i].cntlid = cpu_to_be16(ctnlid);
>> + nvme_status_ext->regctl_eds[i].rkey = cpu_to_be64(keys_info->keys[i]);
>> + nvme_status_ext->regctl_eds[i].rcsts = keys_info->keys[i] ==
>> + reservation->key ? 1 : 0;
>> + /* hostid is not supported currently */
>> + memset(&nvme_status_ext->regctl_eds[i].hostid, 0, 16);
>> + }
>> +
>> + rc = nvme_c2h(n, (uint8_t *)nvme_status_ext, len, req);
>> + g_free(nvme_status_ext);
>> + return rc;
>> +}
