linux-kernel - Re: [PATCH] media: Media Controller fix to not let stream

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, 3 Feb 2016 00:53:21 +0200
From:	Sakari Ailus <sakari.ailus@....fi>
To:	Shuah Khan <shuahkh@....samsung.com>
Cc:	mchehab@....samsung.com, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: Media Controller fix to not let stream_count go
 negative

Hi Shuah,

On Sat, Jan 30, 2016 at 01:10:52PM -0700, Shuah Khan wrote:
> Change media_entity_pipeline_stop() to not decrement
> stream_count of an inactive media pipeline. Doing so,
> results in preventing starting the pipeline.
> 
> Signed-off-by: Shuah Khan <shuahkh@....samsung.com>
> ---
>  drivers/media/media-entity.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index e89d85a..f2e4360 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -452,9 +452,12 @@ error:
>  	media_entity_graph_walk_start(graph, entity_err);
>  
>  	while ((entity_err = media_entity_graph_walk_next(graph))) {
> -		entity_err->stream_count--;
> -		if (entity_err->stream_count == 0)
> -			entity_err->pipe = NULL;
> +		/* don't let the stream_count go negative */
> +		if (entity->stream_count > 0) {
> +			entity_err->stream_count--;
> +			if (entity_err->stream_count == 0)
> +				entity_err->pipe = NULL;
> +		}
>  
>  		/*
>  		 * We haven't increased stream_count further than this
> @@ -486,9 +489,12 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>  	media_entity_graph_walk_start(graph, entity);
>  
>  	while ((entity = media_entity_graph_walk_next(graph))) {
> -		entity->stream_count--;
> -		if (entity->stream_count == 0)
> -			entity->pipe = NULL;
> +		/* don't let the stream_count go negative */
> +		if (entity->stream_count > 0) {
> +			entity->stream_count--;
> +			if (entity->stream_count == 0)
> +				entity->pipe = NULL;
> +		}
>  	}
>  
>  	if (!--pipe->streaming_count)

Have you seen issues with a certain driver, for instance?

In the original design the streaming count is really a count --- streaming
starts when count becomes non-zero, and stops when it reaches zero again.

The calls to media_entity_pipeline_start() and media_entity_pipeline_stop()
should thus always be balanced. I'm fine with the patch, but the framework
should shout loud when the count would be decremented when it's zero.

That was some four or more years ago. I have to say I really haven't been
able to see good reasons for making this a count --- rather what's needed is
to mark the entity as busy so that its link configuration isn't touched. The
request API is a completely different matter then.

Such change would require more discussion IMHO.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@....fi	XMPP: sailus@...iisi.org.uk