| Message ID | 20200811205939.19550-4-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: Fix asd dynamic allocation | expand |
Hi Laurent, On 11/08/2020 21:59, Laurent Pinchart wrote: > v4l2_async_notifier_add_subdev() requires the asd to be allocated > dynamically, but the rcar-drif driver embeds it in the > rcar_drif_graph_ep structure. This causes memory corruption when the > notifier is destroyed at remove time with v4l2_async_notifier_cleanup(). > > Fix this issue by registering the asd with > v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically > internally. > > Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev") > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/platform/rcar_drif.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c > index 3f1e5cb8b197..f318cd4b8086 100644 > --- a/drivers/media/platform/rcar_drif.c > +++ b/drivers/media/platform/rcar_drif.c > @@ -185,7 +185,6 @@ struct rcar_drif_frame_buf { > /* OF graph endpoint's V4L2 async data */ > struct rcar_drif_graph_ep { > struct v4l2_subdev *subdev; /* Async matched subdev */ > - struct v4l2_async_subdev asd; /* Async sub-device descriptor */ > }; > > /* DMA buffer */ > @@ -1109,12 +1108,6 @@ static int rcar_drif_notify_bound(struct v4l2_async_notifier *notifier, > struct rcar_drif_sdr *sdr = > container_of(notifier, struct rcar_drif_sdr, notifier); > > - if (sdr->ep.asd.match.fwnode != > - of_fwnode_handle(subdev->dev->of_node)) { > - rdrif_err(sdr, "subdev %s cannot bind\n", subdev->name); > - return -EINVAL; > - } > - > v4l2_set_subdev_hostdata(subdev, sdr); > sdr->ep.subdev = subdev; > rdrif_dbg(sdr, "bound asd %s\n", subdev->name); > @@ -1218,7 +1211,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr) > { > struct v4l2_async_notifier *notifier = &sdr->notifier; > struct fwnode_handle *fwnode, *ep; > - int ret; > + struct v4l2_async_subdev *asd; > > v4l2_async_notifier_init(notifier); > > @@ -1237,12 +1230,13 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr) > return -EINVAL; > } > > - sdr->ep.asd.match.fwnode = fwnode; > - sdr->ep.asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > - ret = v4l2_async_notifier_add_subdev(notifier, &sdr->ep.asd); > + asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode, > + sizeof(*asd)); I guess this isn't suffering from the same thing that happened on the max9286 as there is no need for any private data to follow here. So, Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > fwnode_handle_put(fwnode); > + if (IS_ERR(asd)) > + return PTR_ERR(asd); > > - return ret; > + return 0; > } > > /* Check if the given device is the primary bond */ >
diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c index 3f1e5cb8b197..f318cd4b8086 100644 --- a/drivers/media/platform/rcar_drif.c +++ b/drivers/media/platform/rcar_drif.c @@ -185,7 +185,6 @@ struct rcar_drif_frame_buf { /* OF graph endpoint's V4L2 async data */ struct rcar_drif_graph_ep { struct v4l2_subdev *subdev; /* Async matched subdev */ - struct v4l2_async_subdev asd; /* Async sub-device descriptor */ }; /* DMA buffer */ @@ -1109,12 +1108,6 @@ static int rcar_drif_notify_bound(struct v4l2_async_notifier *notifier, struct rcar_drif_sdr *sdr = container_of(notifier, struct rcar_drif_sdr, notifier); - if (sdr->ep.asd.match.fwnode != - of_fwnode_handle(subdev->dev->of_node)) { - rdrif_err(sdr, "subdev %s cannot bind\n", subdev->name); - return -EINVAL; - } - v4l2_set_subdev_hostdata(subdev, sdr); sdr->ep.subdev = subdev; rdrif_dbg(sdr, "bound asd %s\n", subdev->name); @@ -1218,7 +1211,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr) { struct v4l2_async_notifier *notifier = &sdr->notifier; struct fwnode_handle *fwnode, *ep; - int ret; + struct v4l2_async_subdev *asd; v4l2_async_notifier_init(notifier); @@ -1237,12 +1230,13 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr) return -EINVAL; } - sdr->ep.asd.match.fwnode = fwnode; - sdr->ep.asd.match_type = V4L2_ASYNC_MATCH_FWNODE; - ret = v4l2_async_notifier_add_subdev(notifier, &sdr->ep.asd); + asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode, + sizeof(*asd)); fwnode_handle_put(fwnode); + if (IS_ERR(asd)) + return PTR_ERR(asd); - return ret; + return 0; } /* Check if the given device is the primary bond */
v4l2_async_notifier_add_subdev() requires the asd to be allocated dynamically, but the rcar-drif driver embeds it in the rcar_drif_graph_ep structure. This causes memory corruption when the notifier is destroyed at remove time with v4l2_async_notifier_cleanup(). Fix this issue by registering the asd with v4l2_async_notifier_add_fwnode_subdev(), which allocates it dynamically internally. Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev") Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/media/platform/rcar_drif.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)