[media] media-entity: Don't use var length arrays
diff mbox

Message ID 60c3f149566de02ae1d2b857d4b6115ec7a9107e.1442963941.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab Sept. 22, 2015, 11:19 p.m. UTC
The graph traversal algorithm currently uses two variable-length
arrays that are dynamically allocated at the stack:

	drivers/media/media-entity.c:238:17: warning: Variable length array is used.
	drivers/media/media-entity.c:239:17: warning: Variable length array is used.

Those are bad, specially if the number of pads for some entity would
be bigger than a certain amount, as it would cause a Linux stack
overflow.

We could just replace entity->num_pads by a define in order to solve
the above sparse warnings, but that would still prevent having entities
with a big number of pads. So, let's do the right thing and use a
kalloced var to store those ancillary bitmaps, e. g. put them at
struct media_device, and use an arbitrary big number that will work
for all currently known usecases.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Comments

Sakari Ailus Oct. 4, 2015, 4:52 p.m. UTC | #1
Hi Mauro,

Mauro Carvalho Chehab wrote:
> The graph traversal algorithm currently uses two variable-length
> arrays that are dynamically allocated at the stack:
> 
> 	drivers/media/media-entity.c:238:17: warning: Variable length array is used.
> 	drivers/media/media-entity.c:239:17: warning: Variable length array is used.
> 
> Those are bad, specially if the number of pads for some entity would
> be bigger than a certain amount, as it would cause a Linux stack
> overflow.
> 
> We could just replace entity->num_pads by a define in order to solve
> the above sparse warnings, but that would still prevent having entities
> with a big number of pads. So, let's do the right thing and use a
> kalloced var to store those ancillary bitmaps, e. g. put them at
> struct media_device, and use an arbitrary big number that will work
> for all currently known usecases.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 153a46469814..02842d875809 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -235,8 +235,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  	media_entity_graph_walk_start(&graph, entity);
>  
>  	while ((entity = media_entity_graph_walk_next(&graph))) {
> -		DECLARE_BITMAP(active, entity->num_pads);
> -		DECLARE_BITMAP(has_no_links, entity->num_pads);
>  		unsigned int i;
>  
>  		entity->stream_count++;
> @@ -250,8 +248,8 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  		if (!entity->ops || !entity->ops->link_validate)
>  			continue;
>  
> -		bitmap_zero(active, entity->num_pads);
> -		bitmap_fill(has_no_links, entity->num_pads);
> +		bitmap_zero(mdev->active, entity->num_pads);
> +		bitmap_fill(mdev->has_no_links, entity->num_pads);
>  
>  		for (i = 0; i < entity->num_links; i++) {
>  			struct media_link *link = &entity->links[i];
> @@ -259,7 +257,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  						? link->sink : link->source;
>  
>  			/* Mark that a pad is connected by a link. */
> -			bitmap_clear(has_no_links, pad->index, 1);
> +			bitmap_clear(mdev->has_no_links, pad->index, 1);
>  
>  			/*
>  			 * Pads that either do not need to connect or
> @@ -268,7 +266,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  			 */
>  			if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) ||
>  			    link->flags & MEDIA_LNK_FL_ENABLED)
> -				bitmap_set(active, pad->index, 1);
> +				bitmap_set(mdev->active, pad->index, 1);
>  
>  			/*
>  			 * Link validation will only take place for
> @@ -290,15 +288,16 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  		}
>  
>  		/* Either no links or validated links are fine. */
> -		bitmap_or(active, active, has_no_links, entity->num_pads);
> +		bitmap_or(mdev->active, mdev->active, mdev->has_no_links,
> +			  entity->num_pads);
>  
> -		if (!bitmap_full(active, entity->num_pads)) {
> +		if (!bitmap_full(mdev->active, entity->num_pads)) {
>  			ret = -EPIPE;
>  			dev_dbg(entity->parent->dev,
>  				"\"%s\":%u must be connected by an enabled link\n",
>  				entity->name,
>  				(unsigned)find_first_zero_bit(
> -					active, entity->num_pads));
> +					mdev->active, entity->num_pads));
>  			goto error;
>  		}
>  	}
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 6e6db78f1ee2..1ac132af031b 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -32,6 +32,10 @@
>  
>  struct device;
>  
> +/* Maximum number of pads per entity */
> +
> +#define	MEDIA_MAX_PADS	512
> +
>  /**
>   * struct media_device - Media device
>   * @dev:	Parent device
> @@ -78,6 +82,11 @@ struct media_device {
>  
>  	int (*link_notify)(struct media_link *link, u32 flags,
>  			   unsigned int notification);
> +
> +	/* private: ancillary bitmaps used by graph traversal algo */
> +
> +	DECLARE_BITMAP(active, MEDIA_MAX_PADS);
> +	DECLARE_BITMAP(has_no_links, MEDIA_MAX_PADS);
>  };
>  
>  /* Supported link_notify @notification values. */
> 

I think media_entity_init() should check the number of pads will not
exceed MEDIA_MAX_PADS.

With that change,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Patch
diff mbox

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 153a46469814..02842d875809 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -235,8 +235,6 @@  __must_check int media_entity_pipeline_start(struct media_entity *entity,
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
-		DECLARE_BITMAP(active, entity->num_pads);
-		DECLARE_BITMAP(has_no_links, entity->num_pads);
 		unsigned int i;
 
 		entity->stream_count++;
@@ -250,8 +248,8 @@  __must_check int media_entity_pipeline_start(struct media_entity *entity,
 		if (!entity->ops || !entity->ops->link_validate)
 			continue;
 
-		bitmap_zero(active, entity->num_pads);
-		bitmap_fill(has_no_links, entity->num_pads);
+		bitmap_zero(mdev->active, entity->num_pads);
+		bitmap_fill(mdev->has_no_links, entity->num_pads);
 
 		for (i = 0; i < entity->num_links; i++) {
 			struct media_link *link = &entity->links[i];
@@ -259,7 +257,7 @@  __must_check int media_entity_pipeline_start(struct media_entity *entity,
 						? link->sink : link->source;
 
 			/* Mark that a pad is connected by a link. */
-			bitmap_clear(has_no_links, pad->index, 1);
+			bitmap_clear(mdev->has_no_links, pad->index, 1);
 
 			/*
 			 * Pads that either do not need to connect or
@@ -268,7 +266,7 @@  __must_check int media_entity_pipeline_start(struct media_entity *entity,
 			 */
 			if (!(pad->flags & MEDIA_PAD_FL_MUST_CONNECT) ||
 			    link->flags & MEDIA_LNK_FL_ENABLED)
-				bitmap_set(active, pad->index, 1);
+				bitmap_set(mdev->active, pad->index, 1);
 
 			/*
 			 * Link validation will only take place for
@@ -290,15 +288,16 @@  __must_check int media_entity_pipeline_start(struct media_entity *entity,
 		}
 
 		/* Either no links or validated links are fine. */
-		bitmap_or(active, active, has_no_links, entity->num_pads);
+		bitmap_or(mdev->active, mdev->active, mdev->has_no_links,
+			  entity->num_pads);
 
-		if (!bitmap_full(active, entity->num_pads)) {
+		if (!bitmap_full(mdev->active, entity->num_pads)) {
 			ret = -EPIPE;
 			dev_dbg(entity->parent->dev,
 				"\"%s\":%u must be connected by an enabled link\n",
 				entity->name,
 				(unsigned)find_first_zero_bit(
-					active, entity->num_pads));
+					mdev->active, entity->num_pads));
 			goto error;
 		}
 	}
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 6e6db78f1ee2..1ac132af031b 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -32,6 +32,10 @@ 
 
 struct device;
 
+/* Maximum number of pads per entity */
+
+#define	MEDIA_MAX_PADS	512
+
 /**
  * struct media_device - Media device
  * @dev:	Parent device
@@ -78,6 +82,11 @@  struct media_device {
 
 	int (*link_notify)(struct media_link *link, u32 flags,
 			   unsigned int notification);
+
+	/* private: ancillary bitmaps used by graph traversal algo */
+
+	DECLARE_BITMAP(active, MEDIA_MAX_PADS);
+	DECLARE_BITMAP(has_no_links, MEDIA_MAX_PADS);
 };
 
 /* Supported link_notify @notification values. */