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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
--
Kind regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
