linux-kernel - Re: [PATCH 08/20] lightnvm: pblk: sched. metadata on write thread

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

Date:   Wed, 20 Sep 2017 21:28:34 +0300
From:   Rakesh Pandit <rakesh@...era.com>
To:     Javier González <jg@...htnvm.io>
CC:     <mb@...htnvm.io>, <axboe@...com>, <linux-block@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Javier González <javier@...xlabs.com>,
        Matias Bjørling <matias@...xlabs.com>
Subject: Re: [PATCH 08/20] lightnvm: pblk: sched. metadata on write thread

Hi Javier,

one small issue I found for error path while going through changes:

On Mon, Jun 26, 2017 at 11:57:17AM +0200, Javier González wrote:
..
> +static int pblk_lines_alloc_metadata(struct pblk *pblk)
> +{
> +	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
> +	struct pblk_line_meta *lm = &pblk->lm;
> +	int i;
> +
> +	/* smeta is always small enough to fit on a kmalloc memory allocation,
> +	 * emeta depends on the number of LUNs allocated to the pblk instance
> +	 */
> +	l_mg->smeta_alloc_type = PBLK_KMALLOC_META;
> +	for (i = 0; i < PBLK_DATA_LINES; i++) {
> +		l_mg->sline_meta[i] = kmalloc(lm->smeta_len, GFP_KERNEL);
> +		if (!l_mg->sline_meta[i])
> +			goto fail_free_smeta;

For this error path the the goto label at end doesn't free up
resources correctly.  It needs a

while (--index >= 0)...

logic with appropriate adjustment.

> +	}
> +
> +	/* emeta allocates three different buffers for managing metadata with
> +	 * in-memory and in-media layouts
> +	 */
> +	for (i = 0; i < PBLK_DATA_LINES; i++) {
> +		struct pblk_emeta *emeta;
> +
> +		emeta = kmalloc(sizeof(struct pblk_emeta), GFP_KERNEL);
> +		if (!emeta)
> +			goto fail_free_emeta;
> +
> +		if (lm->emeta_len[0] > KMALLOC_MAX_CACHE_SIZE) {
> +			l_mg->emeta_alloc_type = PBLK_VMALLOC_META;
> +
> +			emeta->buf = vmalloc(lm->emeta_len[0]);
> +			if (!emeta->buf) {
> +				kfree(emeta);
> +				goto fail_free_emeta;
> +			}
> +
> +			emeta->nr_entries = lm->emeta_sec[0];
> +			l_mg->eline_meta[i] = emeta;
> +		} else {
> +			l_mg->emeta_alloc_type = PBLK_KMALLOC_META;
> +
> +			emeta->buf = kmalloc(lm->emeta_len[0], GFP_KERNEL);
> +			if (!emeta->buf) {
> +				kfree(emeta);
> +				goto fail_free_emeta;
> +			}
> +
> +			emeta->nr_entries = lm->emeta_sec[0];
> +			l_mg->eline_meta[i] = emeta;
> +		}
> +	}
> +
> +	l_mg->vsc_list = kcalloc(l_mg->nr_lines, sizeof(__le32), GFP_KERNEL);
> +	if (!l_mg->vsc_list)
> +		goto fail_free_emeta;
> +
> +	for (i = 0; i < l_mg->nr_lines; i++)
> +		l_mg->vsc_list[i] = cpu_to_le32(EMPTY_ENTRY);
> +
> +	return 0;
> +
> +fail_free_emeta:
> +	while (--i >= 0) {
> +		vfree(l_mg->eline_meta[i]->buf);

This would need l_mg->emeta_alloc_type check to decide whether
allocation was done with kmalloc or vmalloc.

> +		kfree(&l_mg->eline_meta[i]);
> +	}
> +
> +fail_free_smeta:
> +	for (i = 0; i < PBLK_DATA_LINES; i++)
> +		pblk_mfree(&l_mg->sline_meta[i], l_mg->smeta_alloc_type);
> +
> +	return -ENOMEM;
> +}
> +

Thanks,