Re: [PATCH] media: vimc: upon streaming, check that the pipeline starts with a source entity

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dafna,

Thanks for the patch. Just small comments below.

On 8/29/19 8:00 AM, Dafna Hirschfeld wrote:
> Userspace can disable links and create pipelines that
> do not start with a source entity. Trying to stream
> from such a pipeline should fail with -EPIPE
> currently this is not handled and cause kernel crash.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
> Hi,
> These are the commands to reproduce the crash:
> media-ctl -d0 -l "5:1->21:0[0]" -v
> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2
> 
> drivers/media/platform/vimc/vimc-streamer.c | 39 +++++++++++++++------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c
> index 048d770e498b..bfc7921c7a8b 100644
> --- a/drivers/media/platform/vimc/vimc-streamer.c
> +++ b/drivers/media/platform/vimc/vimc-streamer.c
> @@ -13,6 +13,19 @@
>  
>  #include "vimc-streamer.h"
>  
> +/**
> + * Check if the entity has only source pads
> + */

please add docs like the other functions in this file.

> +static bool vimc_is_source(struct media_entity *ent)
> +{
> +	int i;

unsigned

Regards,
Helen

> +
> +	for (i = 0; i < ent->num_pads; i++)
> +		if (ent->pads[i].flags & MEDIA_PAD_FL_SINK)
> +			return false;
> +	return true;
> +}
> +
>  /**
>   * vimc_get_source_entity - get the entity connected with the first sink pad
>   *
> @@ -83,14 +96,12 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>  	struct media_entity *entity;
>  	struct video_device *vdev;
>  	struct v4l2_subdev *sd;
> -	int ret = 0;
> +	int ret = -EINVAL;
>  
>  	stream->pipe_size = 0;
>  	while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
> -		if (!ved) {
> -			vimc_streamer_pipeline_terminate(stream);
> -			return -EINVAL;
> -		}
> +		if (!ved)
> +			break;
>  		stream->ved_pipeline[stream->pipe_size++] = ved;
>  
>  		if (is_media_entity_v4l2_subdev(ved->ent)) {
> @@ -99,15 +110,22 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>  			if (ret && ret != -ENOIOCTLCMD) {
>  				pr_err("subdev_call error %s\n",
>  				       ved->ent->name);
> -				vimc_streamer_pipeline_terminate(stream);
> -				return ret;
> +				break;
>  			}
>  		}
>  
>  		entity = vimc_get_source_entity(ved->ent);
> -		/* Check if the end of the pipeline was reached*/
> -		if (!entity)
> +		/* Check if the end of the pipeline was reached */
> +		if (!entity) {
> +			/* the first entity of the pipe should be source only */
> +			if (!vimc_is_source(ved->ent)) {
> +				pr_err("first entity in the pipe '%s' is not a source\n",
> +				       ved->ent->name);
> +				ret = -EPIPE;
> +				break;
> +			}
>  			return 0;
> +		}
>  
>  		/* Get the next device in the pipeline */
>  		if (is_media_entity_v4l2_subdev(entity)) {
> @@ -120,9 +138,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
>  			ved = video_get_drvdata(vdev);
>  		}
>  	}
> -
>  	vimc_streamer_pipeline_terminate(stream);
> -	return -EINVAL;
> +	return ret;
>  }
>  
>  /**
> 



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux