| Message ID | 20190221142148.3412-7-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various core and virtual driver fixes | expand |
Hi Hans, Thank you for the patch. On Thu, Feb 21, 2019 at 03:21:47PM +0100, Hans Verkuil wrote: > When the v4l-subdev device node is released it calls the > v4l2_device_release_subdev_node() function which sets sd->devnode > to NULL. > > However, the v4l2_subdev struct may already be released causing this > to write in freed memory. > > Instead just use the regular video_device_release release function > (just calls kfree) and set sd->devnode to NULL right after the > video_unregister_device() call. This seems a bit of a workaround. The devnode can access the subdev in multiple ways, it should really keep a reference to the subdev to ensure it doesn't get freed early. > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/v4l2-core/v4l2-device.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c > index e0ddb9a52bd1..57a7b220fa4d 100644 > --- a/drivers/media/v4l2-core/v4l2-device.c > +++ b/drivers/media/v4l2-core/v4l2-device.c > @@ -216,13 +216,6 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, > } > EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); > > -static void v4l2_device_release_subdev_node(struct video_device *vdev) > -{ > - struct v4l2_subdev *sd = video_get_drvdata(vdev); > - sd->devnode = NULL; > - kfree(vdev); > -} > - > int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) > { > struct video_device *vdev; > @@ -250,7 +243,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) > vdev->dev_parent = sd->dev; > vdev->v4l2_dev = v4l2_dev; > vdev->fops = &v4l2_subdev_fops; > - vdev->release = v4l2_device_release_subdev_node; > + vdev->release = video_device_release; > vdev->ctrl_handler = sd->ctrl_handler; > err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1, > sd->owner); > @@ -319,6 +312,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) > } > #endif > video_unregister_device(sd->devnode); > + sd->devnode = NULL; > if (!sd->owner_v4l2_dev) > module_put(sd->owner); > }
On 2/22/19 12:32 PM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Thu, Feb 21, 2019 at 03:21:47PM +0100, Hans Verkuil wrote: >> When the v4l-subdev device node is released it calls the >> v4l2_device_release_subdev_node() function which sets sd->devnode >> to NULL. >> >> However, the v4l2_subdev struct may already be released causing this >> to write in freed memory. >> >> Instead just use the regular video_device_release release function >> (just calls kfree) and set sd->devnode to NULL right after the >> video_unregister_device() call. > > This seems a bit of a workaround. The devnode can access the subdev in > multiple ways, it should really keep a reference to the subdev to ensure > it doesn't get freed early. It's not the link from the devnode to the subdev (that's done through video_get_drvdata()), it's the link from the subdev to the devnode. As soon as the video device is unregistered sd->devnode should be set to NULL. It is in fact how sd->devnode is used: as a check if the devnode was registered. The only other place where it is used is in v4l2_subdev_notify_event to send an event to the devnode, and after unregistering the video device you no longer want to do that, so setting sd->devnode to NULL after it is unregistered is the right thing to do. FYI, I'll post a v2 of this series soon. Regards, Hans > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/media/v4l2-core/v4l2-device.c | 10 ++-------- >> 1 file changed, 2 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c >> index e0ddb9a52bd1..57a7b220fa4d 100644 >> --- a/drivers/media/v4l2-core/v4l2-device.c >> +++ b/drivers/media/v4l2-core/v4l2-device.c >> @@ -216,13 +216,6 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, >> } >> EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); >> >> -static void v4l2_device_release_subdev_node(struct video_device *vdev) >> -{ >> - struct v4l2_subdev *sd = video_get_drvdata(vdev); >> - sd->devnode = NULL; >> - kfree(vdev); >> -} >> - >> int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) >> { >> struct video_device *vdev; >> @@ -250,7 +243,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) >> vdev->dev_parent = sd->dev; >> vdev->v4l2_dev = v4l2_dev; >> vdev->fops = &v4l2_subdev_fops; >> - vdev->release = v4l2_device_release_subdev_node; >> + vdev->release = video_device_release; >> vdev->ctrl_handler = sd->ctrl_handler; >> err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1, >> sd->owner); >> @@ -319,6 +312,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) >> } >> #endif >> video_unregister_device(sd->devnode); >> + sd->devnode = NULL; >> if (!sd->owner_v4l2_dev) >> module_put(sd->owner); >> } >
On Fri, Mar 01, 2019 at 12:46:52PM +0100, Hans Verkuil wrote: > On 2/22/19 12:32 PM, Laurent Pinchart wrote: > > Hi Hans, > > > > Thank you for the patch. > > > > On Thu, Feb 21, 2019 at 03:21:47PM +0100, Hans Verkuil wrote: > >> When the v4l-subdev device node is released it calls the > >> v4l2_device_release_subdev_node() function which sets sd->devnode > >> to NULL. > >> > >> However, the v4l2_subdev struct may already be released causing this > >> to write in freed memory. > >> > >> Instead just use the regular video_device_release release function > >> (just calls kfree) and set sd->devnode to NULL right after the > >> video_unregister_device() call. > > > > This seems a bit of a workaround. The devnode can access the subdev in > > multiple ways, it should really keep a reference to the subdev to ensure > > it doesn't get freed early. > > It's not the link from the devnode to the subdev (that's done through > video_get_drvdata()), it's the link from the subdev to the devnode. Right, my bad. > As soon as the video device is unregistered sd->devnode should be set > to NULL. It is in fact how sd->devnode is used: as a check if the devnode > was registered. > > The only other place where it is used is in v4l2_subdev_notify_event to > send an event to the devnode, and after unregistering the video device > you no longer want to do that, so setting sd->devnode to NULL after it > is unregistered is the right thing to do. Is there a risk the two function could race each other ? > FYI, I'll post a v2 of this series soon. > > >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> --- > >> drivers/media/v4l2-core/v4l2-device.c | 10 ++-------- > >> 1 file changed, 2 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c > >> index e0ddb9a52bd1..57a7b220fa4d 100644 > >> --- a/drivers/media/v4l2-core/v4l2-device.c > >> +++ b/drivers/media/v4l2-core/v4l2-device.c > >> @@ -216,13 +216,6 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, > >> } > >> EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); > >> > >> -static void v4l2_device_release_subdev_node(struct video_device *vdev) > >> -{ > >> - struct v4l2_subdev *sd = video_get_drvdata(vdev); > >> - sd->devnode = NULL; > >> - kfree(vdev); > >> -} > >> - > >> int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) > >> { > >> struct video_device *vdev; > >> @@ -250,7 +243,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) > >> vdev->dev_parent = sd->dev; > >> vdev->v4l2_dev = v4l2_dev; > >> vdev->fops = &v4l2_subdev_fops; > >> - vdev->release = v4l2_device_release_subdev_node; > >> + vdev->release = video_device_release; > >> vdev->ctrl_handler = sd->ctrl_handler; > >> err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1, > >> sd->owner); > >> @@ -319,6 +312,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) > >> } > >> #endif > >> video_unregister_device(sd->devnode); > >> + sd->devnode = NULL; > >> if (!sd->owner_v4l2_dev) > >> module_put(sd->owner); > >> }
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c index e0ddb9a52bd1..57a7b220fa4d 100644 --- a/drivers/media/v4l2-core/v4l2-device.c +++ b/drivers/media/v4l2-core/v4l2-device.c @@ -216,13 +216,6 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev, } EXPORT_SYMBOL_GPL(v4l2_device_register_subdev); -static void v4l2_device_release_subdev_node(struct video_device *vdev) -{ - struct v4l2_subdev *sd = video_get_drvdata(vdev); - sd->devnode = NULL; - kfree(vdev); -} - int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) { struct video_device *vdev; @@ -250,7 +243,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev) vdev->dev_parent = sd->dev; vdev->v4l2_dev = v4l2_dev; vdev->fops = &v4l2_subdev_fops; - vdev->release = v4l2_device_release_subdev_node; + vdev->release = video_device_release; vdev->ctrl_handler = sd->ctrl_handler; err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1, sd->owner); @@ -319,6 +312,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) } #endif video_unregister_device(sd->devnode); + sd->devnode = NULL; if (!sd->owner_v4l2_dev) module_put(sd->owner); }
When the v4l-subdev device node is released it calls the v4l2_device_release_subdev_node() function which sets sd->devnode to NULL. However, the v4l2_subdev struct may already be released causing this to write in freed memory. Instead just use the regular video_device_release release function (just calls kfree) and set sd->devnode to NULL right after the video_unregister_device() call. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/v4l2-core/v4l2-device.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)