linux-kernel - Re: [RESEND RFC/PATCH 6/8] media: platform: mtk-vcodec: Add Mediatek V4L2 Video Encoder Driver

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: <565F162A.4070106@linaro.org>
Date:	Wed, 2 Dec 2015 16:02:50 +0000
From:	Daniel Thompson <daniel.thompson@...aro.org>
To:	tiffany lin <tiffany.lin@...iatek.com>
Cc:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Daniel Kurtz <djkurtz@...omium.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Hongzhou Yang <hongzhou.yang@...iatek.com>,
	Hans Verkuil <hans.verkuil@...co.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Sakari Ailus <sakari.ailus@....fi>,
	Mikhail Ulyanov <mikhail.ulyanov@...entembedded.com>,
	Fabien Dessenne <fabien.dessenne@...com>,
	Arnd Bergmann <arnd@...db.de>,
	Darren Etheridge <detheridge@...com>,
	Peter Griffin <peter.griffin@...aro.org>,
	Benoit Parrot <bparrot@...com>,
	Andrew-CT Chen (陳智迪) 
	<Andrew-CT.Chen@...iatek.com>,
	Eddie Huang (黃智傑) 
	<eddie.huang@...iatek.com>,
	Yingjoe Chen (陳英洲) 
	<Yingjoe.Chen@...iatek.com>,
	JamesJJ Liao (廖建智) 
	<jamesjj.liao@...iatek.com>,
	Daniel Hsiao (蕭伯剛) 
	<daniel.hsiao@...iatek.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	lkml <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" 
	<linux-mediatek@...ts.infradead.org>,
	PoChun Lin (林柏君) 
	<PoChun.Lin@...iatek.com>
Subject: Re: [RESEND RFC/PATCH 6/8] media: platform: mtk-vcodec: Add Mediatek
 V4L2 Video Encoder Driver

On 02/12/15 13:08, tiffany lin wrote:
>>> We need MTK_STATE_ABORT to inform encoder thread (mtk_venc_worker) that
>>> stop encodeing job from stopped ctx instance.
>>> When user space qbuf, we need to make sure everything is ready to sent
>>> buf to encode.
>>
>> Agree that you need a flag here. In fact currently you have two,
>> MTK_STATE_ABORT and an unused one called aborting.
>>
>> You need to be very careful with these flags though. They are a magnet
>> for data race bugs (especially combined with SMP).
>>
>> For example at present I can't see any locking in the worker code. This
>> means there is nothing to make all those read-modify-write sequences
>> that manage the state atomic (thus risking state corruption).
>>
> We prevent that one function set the flag and others clear the flag.
> So there is no special lock to protect state.

What prevents concurrent access from different calling contexts? It 
looks to me like the work on the work queue may run concurrently with 
the ioctl calls.


> +static void vb2ops_venc_stop_streaming(struct vb2_queue *q)
> +{
> +	struct mtk_vcodec_ctx *ctx = vb2_get_drv_priv(q);
> +	struct v4l2_device *v4l2_dev = &ctx->dev->v4l2_dev;
> +	struct vb2_buffer *src_buf, *dst_buf;
> +	int retry;
> +	int ret;
> +
> +	mtk_v4l2_debug(2, "[%d]-> type=%d", ctx->idx, q->type);
> +
> +	retry = 0;
> +	while ((ctx->state & MTK_STATE_RUNNING) && (retry < 10)) {
> +		mtk_vcodec_clean_ctx_int_flags(ctx);
> +		ctx->state |= MTK_STATE_ABORT;

As a simple example I think the above line can run concurrently with the 
following code near the end of the worker code.

> +	ctx->state &= ~MTK_STATE_RUNNING;
> +	v4l2_m2m_job_finish(ctx->dev->m2m_dev_enc, ctx->m2m_ctx);

If I'm right then the state of the flags can definitely get clobbered 
due to the read-modify-write actions on the state.


Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/