2018年9月19日(水) 19:35 Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>:
>
> Hi Mita-san,
>
> On Tue, Sep 18, 2018 at 01:03:07AM +0900, Akinobu Mita wrote:
> > The struct video_i2c_data is released when video_unregister_device() is
> > called, but it will still be accessed after calling
> > video_unregister_device().
> >
> > Use devm_kzalloc() and let the memory be automatically released on driver
> > detach.
> >
> > Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
> > Cc: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
> > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Cc: Hans Verkuil <hansverk@xxxxxxxxx>
> > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> > ---
> > drivers/media/i2c/video-i2c.c | 18 +++++-------------
> > 1 file changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 06d29d8..b7a2af9 100644
> > --- a/drivers/media/i2c/video-i2c.c
> > +++ b/drivers/media/i2c/video-i2c.c
> > @@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
> > .vidioc_streamoff = vb2_ioctl_streamoff,
> > };
> >
> > -static void video_i2c_release(struct video_device *vdev)
> > -{
> > - kfree(video_get_drvdata(vdev));
>
> This is actually correct: it ensures that that the device data stays in
> place as long as the device is being accessed. Allocating device data with
> devm_kzalloc() no longer guarantees that, and is not the right thing to do
> for that reason.
I have actually inserted printk() each line in video_i2_remove(). When
rmmod this driver, video_i2c_release() (and also kfree) is called while
executing video_unregister_device(). Because video_unregister_device()
releases the last reference to data->vdev.dev, then v4l2_device_release()
callback executes data->vdev.release.
So use after freeing video_i2c_data actually happened.
In this patch, devm_kzalloc() is called with client->dev (not with vdev->dev).
So the allocated memory is released when the last user of client->dev
is gone (maybe just after video_i2_remove() is finished).
> > -}
> > -
> > static int video_i2c_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > struct video_i2c_data *data;
> > struct v4l2_device *v4l2_dev;
> > struct vb2_queue *queue;
> > - int ret = -ENODEV;
> > + int ret;
> >
> > - data = kzalloc(sizeof(*data), GFP_KERNEL);
> > + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> > if (!data)
> > return -ENOMEM;
> >
> > @@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
> > else if (id)
> > data->chip = &video_i2c_chip[id->driver_data];
> > else
> > - goto error_free_device;
> > + return -ENODEV;
> >
> > data->client = client;
> > v4l2_dev = &data->v4l2_dev;
> > @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
> >
> > ret = v4l2_device_register(&client->dev, v4l2_dev);
> > if (ret < 0)
> > - goto error_free_device;
> > + return ret;
> >
> > mutex_init(&data->lock);
> > mutex_init(&data->queue_lock);
> > @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
> > data->vdev.fops = &video_i2c_fops;
> > data->vdev.lock = &data->lock;
> > data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
> > - data->vdev.release = video_i2c_release;
> > + data->vdev.release = video_device_release_empty;
> > data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
> > V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
> >
> > @@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
> > mutex_destroy(&data->lock);
> > mutex_destroy(&data->queue_lock);
> >
> > -error_free_device:
> > - kfree(data);
> > -
> > return ret;
> > }
> >
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@xxxxxxxxxxxxxxx
