On 04/27/2016 11:56 PM, Shuah Khan wrote:
>>> dev_dbg(mdev->dev, "Media device unregistered\n");
>>> }
>>> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
>>> index 29409f4..9af9ba1 100644
>>> --- a/drivers/media/media-devnode.c
>>> +++ b/drivers/media/media-devnode.c
>>> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file
>>> *filp)
>>> mutex_unlock(&media_devnode_lock);
>>> return -ENXIO;
>>> }
>>> +
>>> + kobject_get(&mdev->kobj);
>>
>> This is not necessary, and if it was it would be prone to race condition as
>> the last reference could be dropped before this line. But assigning the cdev
>> parent makes sure that we always have a reference to the object while the
>> open() callback is running.
>
> I don't see cdev parent kobj get in cdev_get() which does kobject_get()
> on cdev->kobj. Is that enough to get the reference?
>
> cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is
> the reason why I added a get here and put in media_release().
>
The cdev takes the parent reference when created and only drops it once it
is released. So as long as the cdev exists there is a reference to the
parent. While cdev_del() puts one reference to the cdev there is also one
reference for each open file. So as long as there is a open file there is a
reference to the parent as well.
> I can remove the get and put and test. Looks like I am not checking
> kobject_get() return value which isn't good?
kobject_get() can't fail.
>
>>
>>> +
>>> /* and increase the device refcount */
>>> get_device(&mdev->dev);
>>> mutex_unlock(&media_devnode_lock);
>>> /*
>> [...]
>>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
>>> index fe42f08..ba4bdaa 100644
>>> --- a/include/media/media-devnode.h
>>> +++ b/include/media/media-devnode.h
>>> @@ -70,7 +70,9 @@ struct media_file_operations {
>>> * @fops: pointer to struct &media_file_operations with media device ops
>>> * @dev: struct device pointer for the media controller device
>>> * @cdev: struct cdev pointer character device
>>> + * @kobj: struct kobject
>>> * @parent: parent device
>>> + * @media_dev: media device
>>> * @minor: device node minor number
>>> * @flags: flags, combination of the MEDIA_FLAG_* constants
>>> * @release: release callback called at the end of
>>> media_devnode_release()
>>> @@ -87,7 +89,9 @@ struct media_devnode {
>>> /* sysfs */
>>> struct device dev; /* media device */
>>> struct cdev cdev; /* character device */
>>> + struct kobject kobj; /* set as cdev parent kobj */
>>
>> You don't need a extra kobj. Just use the struct dev kobj.
>
> Yeah I can use that as long as I can override the default release
> function with media_devnode_free(). media_devnode should stick around
> until the last app closes /dev/mediaX even if the media_device is no
> longer registered. i.e media_ioctl should be able to check if devnode
> is registered or not. I think I am missing something and don't understand
> how struct dev kobj can be used.
The struct dev that is embedded into th media_devnode as the same live time
as the media_devnode itself. In addition to that struct device is a
reference counted object. This means a structure that embeds struct device
must not be freed until the last reference is dropped.
What you do here is introduce a independent reference counting mechanism for
the same structure. Which means if there is a reference to struct device,
but not to the new kobj you end up with a use-after-free again.
The solution is to only use one reference counting mechanism (the struct
device) and intialize the cdev kobj parent to the device kobj and whenever
you did kobj_{get,put}() replace that with {get,put}_device(). And in the
device release callback free the struct media_devnode.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
