| 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
| ||
|
Message-ID: <9ba97821-9bb8-293b-2d97-a3095c207184@lightnvm.io>
Date: Fri, 7 Sep 2018 17:12:40 +0200
From: Matias Bjørling <mb@...htnvm.io>
To: hans.ml.holmberg@...tronix.com, javier@...igon.com
Cc: igor.j.konopko@...el.com, marcin.dziegielewski@...el.com,
linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
javier@...xlabs.com
Subject: Re: [PATCH 1/3] lightnvm: use internal allocation for chunk log page
On 09/07/2018 02:53 PM, Hans Holmberg wrote:
> On Wed, Aug 29, 2018 at 12:10 PM Javier González <javier@...igon.com> wrote:
>>
>> The lightnvm subsystem provides helpers to retrieve chunk metadata,
>> where the target needs to provide a buffer to store the metadata. An
>> implicit assumption is that this buffer is contiguous and can be used to
>> retrieve the data from the device. If the device exposes too many
>> chunks, then kmalloc might fail, thus failing instance creation.
>>
>> This patch removes this assumption by implementing an internal buffer in
>> the lightnvm subsystem to retrieve chunk metadata. Targets can then
>> use virtual memory allocations. Since this is a target API change, adapt
>> pblk accordingly.
>>
>> Signed-off-by: Javier González <javier@...xlabs.com>
>> ---
>> drivers/lightnvm/pblk-core.c | 4 ++--
>> drivers/lightnvm/pblk-init.c | 2 +-
>> drivers/nvme/host/lightnvm.c | 23 +++++++++++++++--------
>> 3 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>> index fdcbeb920c9e..a311cc29afd8 100644
>> --- a/drivers/lightnvm/pblk-core.c
>> +++ b/drivers/lightnvm/pblk-core.c
>> @@ -120,7 +120,7 @@ static void pblk_end_io_erase(struct nvm_rq *rqd)
>> /*
>> * Get information for all chunks from the device.
>> *
>> - * The caller is responsible for freeing the returned structure
>> + * The caller is responsible for freeing (vmalloc) the returned structure
>> */
>> struct nvm_chk_meta *pblk_get_chunk_meta(struct pblk *pblk)
>> {
>> @@ -134,7 +134,7 @@ struct nvm_chk_meta *pblk_get_chunk_meta(struct pblk *pblk)
>> ppa.ppa = 0;
>>
>> len = geo->all_chunks * sizeof(*meta);
>> - meta = kzalloc(len, GFP_KERNEL);
>> + meta = vzalloc(len);
>> if (!meta)
>> return ERR_PTR(-ENOMEM);
>>
>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>> index e0db6de137d6..a99854439224 100644
>> --- a/drivers/lightnvm/pblk-init.c
>> +++ b/drivers/lightnvm/pblk-init.c
>> @@ -983,7 +983,7 @@ static int pblk_lines_init(struct pblk *pblk)
>>
>> pblk_set_provision(pblk, nr_free_chks);
>>
>> - kfree(chunk_meta);
>> + vfree(chunk_meta);
>> return 0;
>>
>> fail_free_lines:
>> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
>> index 2c96e7fcdcac..5bfa354c5dd5 100644
>> --- a/drivers/nvme/host/lightnvm.c
>> +++ b/drivers/nvme/host/lightnvm.c
>> @@ -579,7 +579,7 @@ static int nvme_nvm_get_chk_meta(struct nvm_dev *ndev,
>> struct nvm_geo *geo = &ndev->geo;
>> struct nvme_ns *ns = ndev->q->queuedata;
>> struct nvme_ctrl *ctrl = ns->ctrl;
>> - struct nvme_nvm_chk_meta *dev_meta = (struct nvme_nvm_chk_meta *)meta;
>> + struct nvme_nvm_chk_meta *dev_meta, *dev_meta_off;
>> struct ppa_addr ppa;
>> size_t left = nchks * sizeof(struct nvme_nvm_chk_meta);
>> size_t log_pos, offset, len;
>> @@ -591,6 +591,10 @@ static int nvme_nvm_get_chk_meta(struct nvm_dev *ndev,
>> */
>> max_len = min_t(unsigned int, ctrl->max_hw_sectors << 9, 256 * 1024);
>>
>> + dev_meta = kmalloc(max_len, GFP_KERNEL);
>> + if (!dev_meta)
>> + return -ENOMEM;
>
> We should free the memory after using it. Pardon the late review, I
> just now discovered the memory leak during testing.
Thanks Hans. No worries, I saw it just when I had ack'ed the patch.
Would you like me to add your Reviewed-by?
>
>> +
>> /* Normalize lba address space to obtain log offset */
>> ppa.ppa = slba;
>> ppa = dev_to_generic_addr(ndev, ppa);
>> @@ -604,6 +608,9 @@ static int nvme_nvm_get_chk_meta(struct nvm_dev *ndev,
>> while (left) {
>> len = min_t(unsigned int, left, max_len);
>>
>> + memset(dev_meta, 0, max_len);
>> + dev_meta_off = dev_meta;
>> +
>> ret = nvme_get_log_ext(ctrl, ns, NVME_NVM_LOG_REPORT_CHUNK,
>> dev_meta, len, offset);
>> if (ret) {
>> @@ -612,15 +619,15 @@ static int nvme_nvm_get_chk_meta(struct nvm_dev *ndev,
>> }
>>
>> for (i = 0; i < len; i += sizeof(struct nvme_nvm_chk_meta)) {
>> - meta->state = dev_meta->state;
>> - meta->type = dev_meta->type;
>> - meta->wi = dev_meta->wi;
>> - meta->slba = le64_to_cpu(dev_meta->slba);
>> - meta->cnlb = le64_to_cpu(dev_meta->cnlb);
>> - meta->wp = le64_to_cpu(dev_meta->wp);
>> + meta->state = dev_meta_off->state;
>> + meta->type = dev_meta_off->type;
>> + meta->wi = dev_meta_off->wi;
>> + meta->slba = le64_to_cpu(dev_meta_off->slba);
>> + meta->cnlb = le64_to_cpu(dev_meta_off->cnlb);
>> + meta->wp = le64_to_cpu(dev_meta_off->wp);
>>
>> meta++;
>> - dev_meta++;
>> + dev_meta_off++;
>> }
>>
>> offset += len;
>> --
>> 2.7.4
>>
Powered by blists - more mailing lists