Re: [PATCH v2 05/14] media: v4l2-subdev: Refactor returning routes

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sakari

On Wed, Feb 11, 2026 at 11:09:11AM +0200, Sakari Ailus wrote:
> Refactor returning the routes by adding a new function that essentially
> does a memcopy and sets the number of the number of the routes in the
> routing table.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 40 +++++++++++++++------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b2105c34478e..8a433d074587 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -629,6 +629,19 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
>  			     v4l2_subdev_get_unlocked_active_state(sd);
>  }
>
> +static void subdev_copy_fix_routes(struct v4l2_subdev_routing *routing,

or just subdev_copy_routes()

> +				   const struct v4l2_subdev_route *src,
> +				   u32 copy_routes, u32 num_routes)
> +{
> +	struct v4l2_subdev_route *routes =
> +		(struct v4l2_subdev_route *)(uintptr_t)routing->routes;
> +
> +	for (u32 i = 0; i < copy_routes; i++)

Shouldn't this copy the min(copy_routes, num_routes) as the callers
used to do ?

Even better, shouldn't we just accept one 'dest' and one 'source'
v4l2_subdev_routing arguments and make sure we don't copy more than
min(dest->num_routes, source->num_routes) ?

Having the number of routes being passed in by the caller opens the
door for memory errors.


> +		routes[i] = src[i];
> +
> +	routing->num_routes = num_routes;
> +}
> +
>  static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  			    struct v4l2_subdev_state *state)
>  {
> @@ -1000,7 +1013,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>
>  	case VIDIOC_SUBDEV_G_ROUTING: {
>  		struct v4l2_subdev_routing *routing = arg;
> -		struct v4l2_subdev_krouting *krouting;
>
>  		if (!v4l2_subdev_enable_streams_api)
>  			return -ENOIOCTLCMD;
> @@ -1013,13 +1025,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>
>  		memset(routing->reserved, 0, sizeof(routing->reserved));
>
> -		krouting = &state->routing;
> -
> -		memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
> -		       krouting->routes,
> -		       min(krouting->num_routes, routing->len_routes) *
> -		       sizeof(*krouting->routes));
> -		routing->num_routes = krouting->num_routes;
> +		subdev_copy_fix_routes(routing, state->routing.routes,
> +				       routing->len_routes,
> +				       state->routing.num_routes);
>
>  		return 0;
>  	}
> @@ -1090,11 +1098,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  		 * the routing table.
>  		 */
>  		if (!v4l2_subdev_has_op(sd, pad, set_routing)) {
> -			memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
> -			       state->routing.routes,
> -			       min(state->routing.num_routes, routing->len_routes) *
> -			       sizeof(*state->routing.routes));
> -			routing->num_routes = state->routing.num_routes;
> +			subdev_copy_fix_routes(routing, state->routing.routes,
> +					       routing->len_routes,
> +					       state->routing.num_routes);
>
>  			return 0;
>  		}
> @@ -1108,11 +1114,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  		if (rval < 0)
>  			return rval;
>
> -		memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
> -		       state->routing.routes,
> -		       min(state->routing.num_routes, routing->len_routes) *
> -		       sizeof(*state->routing.routes));
> -		routing->num_routes = state->routing.num_routes;
> +		subdev_copy_fix_routes(routing, state->routing.routes,
> +				       routing->len_routes,
> +				       state->routing.num_routes);
>
>  		return 0;
>  	}
> --
> 2.47.3
>
>