linux-kernel - Re: [PATCH V3 09/18] coresight: tmc: allocating memory when needed

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]

Message-ID: <571DEF5F.5060109@arm.com>
Date:	Mon, 25 Apr 2016 11:20:15 +0100
From:	Suzuki K Poulose <Suzuki.Poulose@....com>
To:	Mathieu Poirier <mathieu.poirier@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 09/18] coresight: tmc: allocating memory when needed

On 22/04/16 18:14, Mathieu Poirier wrote:
>   static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
>   {
> +	bool used = false;
> +	char *buf = NULL;
>   	unsigned long flags;
>   	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> +	 /* This shouldn't be happening */
> +	if (WARN_ON(mode != CS_MODE_SYSFS))
> +		return -EINVAL;
> +
> +	/*
> +	 * If a buffer is already allocated *keep holding* the lock and
> +	 * jump to the fast path.  Otherwise release the lock and allocate
> +	 * memory to work with.
> +	 */
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (drvdata->buf)
> +		goto fast_path;
> +
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	/* Allocating the memory here while outside of the spinlock */
> +	buf = kzalloc(drvdata->size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Let's try again */
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +fast_path:

nit: As I mentioned in the previous series, could we not avoid the goto in the middle of the
function,by doing :

	if (!drvdata->buf) {
		unlock();
		buf = alloc();
		if (!buf) return -ENOMEM;
		lock();
	}


>   	if (drvdata->reading) {
>   		spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		/*
> +		 * Free allocated memory outside of the spinlock.  There is
> +		 * no need to assert the validity of 'buf' since calling
> +		 * kfree(NULL) is safe.
> +		 */
> +		kfree(buf);
>   		return -EBUSY;
>   	}

It would be good to unify the exit points as we do similar steps.
	if (drvdata->reading) {
		rc = -EBUSY;
		goto out;
	}
>
> +	/*
> +	 * If drvdata::buf isn't NULL, memory was allocated for a previous
> +	 * trace run but wasn't read.  If so simply zero-out the memory.
> +	 * Otherwise use the memory allocated above.
> +	 *
> +	 * The memory is freed when users read the buffer using the
> +	 * /dev/xyz.{etf|etb} interface.  See tmc_read_unprepare_etf() for
> +	 * details.
> +	 */
> +	if (drvdata->buf) {
> +		memset(drvdata->buf, 0, drvdata->size);
> +	} else {
> +		used = true;
> +		drvdata->buf = buf;
> +	}
> +
>   	tmc_etb_enable_hw(drvdata);
>   	drvdata->enable = true;

out:

>   	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> +	/* Free memory outside the spinlock if need be */
> +	if (!used && buf)
> +		kfree(buf);
> +
	if (!rc)
	   	dev_info(drvdata->dev, "TMC-ETB/ETF enabled\n");
    	return rc


> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 3483d139a4ac..474c70c089f3 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c

>   static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
>   {
> +	bool used = false;
>   	unsigned long flags;
> +	void __iomem *vaddr = NULL;
> +	dma_addr_t paddr;
>   	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> +	 /* This shouldn't be happening */
> +	if (WARN_ON(mode != CS_MODE_SYSFS))
> +		return -EINVAL;
> +
> +	/*
> +	 * If a buffer is already allocated *keep holding* the lock and
> +	 * jump to the fast path.  Otherwise release the lock and allocate
> +	 * memory to work with.
> +	 */
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (drvdata->vaddr)
> +		goto fast_path;
> +
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	/*
> +	 * Contiguous  memory can't be allocated while a spinlock is held.
> +	 * As such allocate memory here and free it if a buffer has already
> +	 * been allocated (from a previous session).
> +	 */
> +	vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
> +				   &paddr, GFP_KERNEL);
> +	if (!vaddr)
> +		return -ENOMEM;
> +
> +	/* Let's try again */
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
> +fast_path:

nit: Same as above.

I am not too particular about the above changes, but would be good to have them
reader friendly.

Either way,

Reviewed-by: Suzuki K Poulose <suzuki.poulose@....com>

Suzuki