Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
On Fri, Apr 30, 2021 at 3:32 PM Hans de Goede wrote:
>
> Hi,
>
> On 4/30/21 1:38 PM, Daniel Vetter wrote:
> > On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede wrote:
> >>
> >> Hi,
> >>
> >> On 4/29/21 9:09 PM, Daniel Vetter wrote:
> >>> On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
> Hi,
>
> On 4/29/21 2:04 PM, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
> >> On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
> >>> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
> Userspace could hold open a reference to the connector->kdev device,
> through e.g. holding a sysfs-atrtribute open after
> drm_sysfs_connector_remove() has been called. In this case the
> connector
> could be free-ed while the connector->kdev device's drvdata is still
> pointing to it.
>
> Give drm_connector devices there own device type, which allows
> us to specify our own release function and make
> drm_sysfs_connector_add()
> take a reference on the connector object, and have the new release
> function put the reference when the device is released.
>
> Giving drm_connector devices there own device type, will also allow
> checking if a device is a drm_connector device with a
> "if (device->type == _sysfs_device_connector)" check.
>
> Note that the setting of the name member of the device_type struct
> will
> cause udev events for drm_connector-s to now contain
> DEVTYPE=drm_connector
> as extra info. So this extends the uevent part of the userspace API.
>
> Signed-off-by: Hans de Goede
> >>>
> >>> Are you sure? I thought sysfs is supposed to flush out any pending
> >>> operations (they complete fast) and handle open fd internally?
> >>
> >> Yes, it "should" :)
> >
> > Thanks for confirming my vague memories :-)
> >
> > Hans, pls drop this one.
>
> Please see my earlier reply to your review of this patch, it is
> still needed but for a different reason:
>
> """
> We still need this change though to make sure that the
> "drm/connector: Add drm_connector_find_by_fwnode() function"
> does not end up following a dangling drvdat pointer from one
> if the drm_connector kdev-s.
>
> The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
> a reference on all devices and between getting that reference
> and it calling drm_connector_get() - drm_connector_unregister()
> may run and drop the possibly last reference to the
> drm_connector object, freeing it and leaving the kdev's
> drvdata as a dangling pointer.
> """
>
> This is actually why I added it initially, and while adding it
> I came up with this wrong theory of why it was necessary independently
> of the drm_connector_find_by_fwnode() addition, sorry about that.
> >>>
> >>> Generally that's handled by a kref_get_unless_zero under the protection of
> >>> the lock which protects the weak reference. Which I think is the right
> >>> model here (at a glance at least) since this is a lookup function.
> >>
> >> I'm afraid that things are a bit more complicated here. The idea here
> >> is that we have a subsystem outside of the DRM subsystem which received
> >> a hotplug event for a drm-connector. The only info which this subsystem
> >> has is a reference on the fwnode level (either through device-tree or
> >> to platform-code instantiating software-fwnode-s + links for this).
> >>
> >> So in order to deliver the hotplug event to the connector we need
> >> to lookup the connector by fwnode.
> >>
> >> I've chosen to implement this by iterating over all drm_class
> >> devices with a dev_type of drm_connector using class_dev_iter_init()
> >> and friends. This makes sure that we either get a reference to
> >> the device, or that we skip the device if it is being deleted.
> >>
> >> But this just gives us a reference to the connector->kdev, not
> >> to the connector itself. A pointer to the connector itself is stored
> >> as drvdata inside the device, but without taking a reference as
> >> this patch does, there is no guarantee that that pointer does not
> >> point to possibly free-ed mem.
> >>
> >> We could set drvdata to 0 from drm_sysfs_connector_remove()
> >> Before calling device_unregister(connector->kdev) and then do
> >> something like this inside drm_connector_find_by_fwnode():
> >>
> >> /*
> >> * Lock the device to ensure we either see the drvdata == NULL
> >> * set by drm_sysfs_connector_remove(); or we block the removal
> >> * from continuing until we are done with the device.
> >> */
> >> device_lock(dev);
> >> connector = dev_get_drvdata(dev);
> >> if (connector && connector->fwnode == fwnode) {
> >>
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
p.s. On 4/30/21 1:38 PM, Daniel Vetter wrote: Offtopic: > I'm also not sure why we have to use the kdev stuff here. For other > random objects we need to look up we're building that functionality on > that object. It means you need to keep another list_head around for > that lookup, but that's really not a big cost. E.g. drm_bridge/panel > work like that. So I took a peek at the bridge/panel code and that actually seems to have an issue with removal vs lookup. It is not even just a race, it seems a lookup does not take a reference and there is nothing stopping a user from doing an unbind or rmmod causing the panel to be removed while other code which got a pointer to the panel through of_drm_find_panel() will not be prepared to deal with that pointer all of a sudden no longer being valid. Now this would be a case of the user shooting his-self in the foot (where as connectors can actually dynamically disappear under normal circumstances), but ideally we really should do better here. Is there a TODO list somewhere for issues like this ? Or shall I submit a patch adding a FIXME comment, or is this considered not worth the trouble of fixing it? Regards, Hans ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
Hi,
On 4/30/21 1:38 PM, Daniel Vetter wrote:
> On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede wrote:
>>
>> Hi,
>>
>> On 4/29/21 9:09 PM, Daniel Vetter wrote:
>>> On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
Hi,
On 4/29/21 2:04 PM, Daniel Vetter wrote:
> On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
>> On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
>>> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
Userspace could hold open a reference to the connector->kdev device,
through e.g. holding a sysfs-atrtribute open after
drm_sysfs_connector_remove() has been called. In this case the
connector
could be free-ed while the connector->kdev device's drvdata is still
pointing to it.
Give drm_connector devices there own device type, which allows
us to specify our own release function and make
drm_sysfs_connector_add()
take a reference on the connector object, and have the new release
function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow
checking if a device is a drm_connector device with a
"if (device->type == _sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will
cause udev events for drm_connector-s to now contain
DEVTYPE=drm_connector
as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede
>>>
>>> Are you sure? I thought sysfs is supposed to flush out any pending
>>> operations (they complete fast) and handle open fd internally?
>>
>> Yes, it "should" :)
>
> Thanks for confirming my vague memories :-)
>
> Hans, pls drop this one.
Please see my earlier reply to your review of this patch, it is
still needed but for a different reason:
"""
We still need this change though to make sure that the
"drm/connector: Add drm_connector_find_by_fwnode() function"
does not end up following a dangling drvdat pointer from one
if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
a reference on all devices and between getting that reference
and it calling drm_connector_get() - drm_connector_unregister()
may run and drop the possibly last reference to the
drm_connector object, freeing it and leaving the kdev's
drvdata as a dangling pointer.
"""
This is actually why I added it initially, and while adding it
I came up with this wrong theory of why it was necessary independently
of the drm_connector_find_by_fwnode() addition, sorry about that.
>>>
>>> Generally that's handled by a kref_get_unless_zero under the protection of
>>> the lock which protects the weak reference. Which I think is the right
>>> model here (at a glance at least) since this is a lookup function.
>>
>> I'm afraid that things are a bit more complicated here. The idea here
>> is that we have a subsystem outside of the DRM subsystem which received
>> a hotplug event for a drm-connector. The only info which this subsystem
>> has is a reference on the fwnode level (either through device-tree or
>> to platform-code instantiating software-fwnode-s + links for this).
>>
>> So in order to deliver the hotplug event to the connector we need
>> to lookup the connector by fwnode.
>>
>> I've chosen to implement this by iterating over all drm_class
>> devices with a dev_type of drm_connector using class_dev_iter_init()
>> and friends. This makes sure that we either get a reference to
>> the device, or that we skip the device if it is being deleted.
>>
>> But this just gives us a reference to the connector->kdev, not
>> to the connector itself. A pointer to the connector itself is stored
>> as drvdata inside the device, but without taking a reference as
>> this patch does, there is no guarantee that that pointer does not
>> point to possibly free-ed mem.
>>
>> We could set drvdata to 0 from drm_sysfs_connector_remove()
>> Before calling device_unregister(connector->kdev) and then do
>> something like this inside drm_connector_find_by_fwnode():
>>
>> /*
>> * Lock the device to ensure we either see the drvdata == NULL
>> * set by drm_sysfs_connector_remove(); or we block the removal
>> * from continuing until we are done with the device.
>> */
>> device_lock(dev);
>> connector = dev_get_drvdata(dev);
>> if (connector && connector->fwnode == fwnode) {
>> drm_connector_get(connector);
>> found = connector;
>> }
>> device_unlock(dev);
>
> Yes this is what I mean. Except not a drm_connector_get, but a
> kref_get_unless_zero. The connector might already be on it's way out,
> but the drvdata not yet cleared.
The function we
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
On Fri, Apr 30, 2021 at 1:28 PM Hans de Goede wrote:
>
> Hi,
>
> On 4/29/21 9:09 PM, Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 4/29/21 2:04 PM, Daniel Vetter wrote:
> >>> On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
> >> Userspace could hold open a reference to the connector->kdev device,
> >> through e.g. holding a sysfs-atrtribute open after
> >> drm_sysfs_connector_remove() has been called. In this case the
> >> connector
> >> could be free-ed while the connector->kdev device's drvdata is still
> >> pointing to it.
> >>
> >> Give drm_connector devices there own device type, which allows
> >> us to specify our own release function and make
> >> drm_sysfs_connector_add()
> >> take a reference on the connector object, and have the new release
> >> function put the reference when the device is released.
> >>
> >> Giving drm_connector devices there own device type, will also allow
> >> checking if a device is a drm_connector device with a
> >> "if (device->type == _sysfs_device_connector)" check.
> >>
> >> Note that the setting of the name member of the device_type struct will
> >> cause udev events for drm_connector-s to now contain
> >> DEVTYPE=drm_connector
> >> as extra info. So this extends the uevent part of the userspace API.
> >>
> >> Signed-off-by: Hans de Goede
> >
> > Are you sure? I thought sysfs is supposed to flush out any pending
> > operations (they complete fast) and handle open fd internally?
>
> Yes, it "should" :)
> >>>
> >>> Thanks for confirming my vague memories :-)
> >>>
> >>> Hans, pls drop this one.
> >>
> >> Please see my earlier reply to your review of this patch, it is
> >> still needed but for a different reason:
> >>
> >> """
> >> We still need this change though to make sure that the
> >> "drm/connector: Add drm_connector_find_by_fwnode() function"
> >> does not end up following a dangling drvdat pointer from one
> >> if the drm_connector kdev-s.
> >>
> >> The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
> >> a reference on all devices and between getting that reference
> >> and it calling drm_connector_get() - drm_connector_unregister()
> >> may run and drop the possibly last reference to the
> >> drm_connector object, freeing it and leaving the kdev's
> >> drvdata as a dangling pointer.
> >> """
> >>
> >> This is actually why I added it initially, and while adding it
> >> I came up with this wrong theory of why it was necessary independently
> >> of the drm_connector_find_by_fwnode() addition, sorry about that.
> >
> > Generally that's handled by a kref_get_unless_zero under the protection of
> > the lock which protects the weak reference. Which I think is the right
> > model here (at a glance at least) since this is a lookup function.
>
> I'm afraid that things are a bit more complicated here. The idea here
> is that we have a subsystem outside of the DRM subsystem which received
> a hotplug event for a drm-connector. The only info which this subsystem
> has is a reference on the fwnode level (either through device-tree or
> to platform-code instantiating software-fwnode-s + links for this).
>
> So in order to deliver the hotplug event to the connector we need
> to lookup the connector by fwnode.
>
> I've chosen to implement this by iterating over all drm_class
> devices with a dev_type of drm_connector using class_dev_iter_init()
> and friends. This makes sure that we either get a reference to
> the device, or that we skip the device if it is being deleted.
>
> But this just gives us a reference to the connector->kdev, not
> to the connector itself. A pointer to the connector itself is stored
> as drvdata inside the device, but without taking a reference as
> this patch does, there is no guarantee that that pointer does not
> point to possibly free-ed mem.
>
> We could set drvdata to 0 from drm_sysfs_connector_remove()
> Before calling device_unregister(connector->kdev) and then do
> something like this inside drm_connector_find_by_fwnode():
>
> /*
> * Lock the device to ensure we either see the drvdata == NULL
> * set by drm_sysfs_connector_remove(); or we block the removal
> * from continuing until we are done with the device.
> */
> device_lock(dev);
> connector = dev_get_drvdata(dev);
> if (connector && connector->fwnode == fwnode) {
> drm_connector_get(connector);
> found = connector;
> }
> device_unlock(dev);
Yes this is what I mean. Except not a drm_connector_get, but a
kref_get_unless_zero. The connector might already be on it's way out,
but the drvdata not yet cleared.
> With the device_lock() synchronizing against the device_lock()
> in device_unregister(connector->kdev). So that
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
Hi,
On 4/29/21 9:09 PM, Daniel Vetter wrote:
> On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 4/29/21 2:04 PM, Daniel Vetter wrote:
>>> On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote:
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote:
> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
>> Userspace could hold open a reference to the connector->kdev device,
>> through e.g. holding a sysfs-atrtribute open after
>> drm_sysfs_connector_remove() has been called. In this case the connector
>> could be free-ed while the connector->kdev device's drvdata is still
>> pointing to it.
>>
>> Give drm_connector devices there own device type, which allows
>> us to specify our own release function and make drm_sysfs_connector_add()
>> take a reference on the connector object, and have the new release
>> function put the reference when the device is released.
>>
>> Giving drm_connector devices there own device type, will also allow
>> checking if a device is a drm_connector device with a
>> "if (device->type == _sysfs_device_connector)" check.
>>
>> Note that the setting of the name member of the device_type struct will
>> cause udev events for drm_connector-s to now contain
>> DEVTYPE=drm_connector
>> as extra info. So this extends the uevent part of the userspace API.
>>
>> Signed-off-by: Hans de Goede
>
> Are you sure? I thought sysfs is supposed to flush out any pending
> operations (they complete fast) and handle open fd internally?
Yes, it "should" :)
>>>
>>> Thanks for confirming my vague memories :-)
>>>
>>> Hans, pls drop this one.
>>
>> Please see my earlier reply to your review of this patch, it is
>> still needed but for a different reason:
>>
>> """
>> We still need this change though to make sure that the
>> "drm/connector: Add drm_connector_find_by_fwnode() function"
>> does not end up following a dangling drvdat pointer from one
>> if the drm_connector kdev-s.
>>
>> The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
>> a reference on all devices and between getting that reference
>> and it calling drm_connector_get() - drm_connector_unregister()
>> may run and drop the possibly last reference to the
>> drm_connector object, freeing it and leaving the kdev's
>> drvdata as a dangling pointer.
>> """
>>
>> This is actually why I added it initially, and while adding it
>> I came up with this wrong theory of why it was necessary independently
>> of the drm_connector_find_by_fwnode() addition, sorry about that.
>
> Generally that's handled by a kref_get_unless_zero under the protection of
> the lock which protects the weak reference. Which I think is the right
> model here (at a glance at least) since this is a lookup function.
I'm afraid that things are a bit more complicated here. The idea here
is that we have a subsystem outside of the DRM subsystem which received
a hotplug event for a drm-connector. The only info which this subsystem
has is a reference on the fwnode level (either through device-tree or
to platform-code instantiating software-fwnode-s + links for this).
So in order to deliver the hotplug event to the connector we need
to lookup the connector by fwnode.
I've chosen to implement this by iterating over all drm_class
devices with a dev_type of drm_connector using class_dev_iter_init()
and friends. This makes sure that we either get a reference to
the device, or that we skip the device if it is being deleted.
But this just gives us a reference to the connector->kdev, not
to the connector itself. A pointer to the connector itself is stored
as drvdata inside the device, but without taking a reference as
this patch does, there is no guarantee that that pointer does not
point to possibly free-ed mem.
We could set drvdata to 0 from drm_sysfs_connector_remove()
Before calling device_unregister(connector->kdev) and then do
something like this inside drm_connector_find_by_fwnode():
/*
* Lock the device to ensure we either see the drvdata == NULL
* set by drm_sysfs_connector_remove(); or we block the removal
* from continuing until we are done with the device.
*/
device_lock(dev);
connector = dev_get_drvdata(dev);
if (connector && connector->fwnode == fwnode) {
drm_connector_get(connector);
found = connector;
}
device_unlock(dev);
With the device_lock() synchronizing against the device_lock()
in device_unregister(connector->kdev). So that we either see
drvdata == NULL if we race with unregistering; or we get
a reference on the drm_connector obj before its ref-count can
drop to 0.
There might be places though where we call code take the device_lock
while holding a lock necessary for the drm_connector_get() , so
this approach might lead to an AB BA deadlock. As such I think
my original approach is better (also see below).
> Lookup tables holding full references tends to
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote: > Hi, > > On 4/29/21 2:04 PM, Daniel Vetter wrote: > > On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote: > >> On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote: > >>> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote: > Userspace could hold open a reference to the connector->kdev device, > through e.g. holding a sysfs-atrtribute open after > drm_sysfs_connector_remove() has been called. In this case the connector > could be free-ed while the connector->kdev device's drvdata is still > pointing to it. > > Give drm_connector devices there own device type, which allows > us to specify our own release function and make drm_sysfs_connector_add() > take a reference on the connector object, and have the new release > function put the reference when the device is released. > > Giving drm_connector devices there own device type, will also allow > checking if a device is a drm_connector device with a > "if (device->type == _sysfs_device_connector)" check. > > Note that the setting of the name member of the device_type struct will > cause udev events for drm_connector-s to now contain > DEVTYPE=drm_connector > as extra info. So this extends the uevent part of the userspace API. > > Signed-off-by: Hans de Goede > >>> > >>> Are you sure? I thought sysfs is supposed to flush out any pending > >>> operations (they complete fast) and handle open fd internally? > >> > >> Yes, it "should" :) > > > > Thanks for confirming my vague memories :-) > > > > Hans, pls drop this one. > > Please see my earlier reply to your review of this patch, it is > still needed but for a different reason: > > """ > We still need this change though to make sure that the > "drm/connector: Add drm_connector_find_by_fwnode() function" > does not end up following a dangling drvdat pointer from one > if the drm_connector kdev-s. > > The class_dev_iter_init() in drm_connector_find_by_fwnode() gets > a reference on all devices and between getting that reference > and it calling drm_connector_get() - drm_connector_unregister() > may run and drop the possibly last reference to the > drm_connector object, freeing it and leaving the kdev's > drvdata as a dangling pointer. > """ > > This is actually why I added it initially, and while adding it > I came up with this wrong theory of why it was necessary independently > of the drm_connector_find_by_fwnode() addition, sorry about that. Generally that's handled by a kref_get_unless_zero under the protection of the lock which protects the weak reference. Which I think is the right model here (at a glance at least) since this is a lookup function. Lookup tables holding full references tends to lead to all kinds of bad side effects. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
Hi, On 4/29/21 2:04 PM, Daniel Vetter wrote: > On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote: >> On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote: >>> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote: Userspace could hold open a reference to the connector->kdev device, through e.g. holding a sysfs-atrtribute open after drm_sysfs_connector_remove() has been called. In this case the connector could be free-ed while the connector->kdev device's drvdata is still pointing to it. Give drm_connector devices there own device type, which allows us to specify our own release function and make drm_sysfs_connector_add() take a reference on the connector object, and have the new release function put the reference when the device is released. Giving drm_connector devices there own device type, will also allow checking if a device is a drm_connector device with a "if (device->type == _sysfs_device_connector)" check. Note that the setting of the name member of the device_type struct will cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector as extra info. So this extends the uevent part of the userspace API. Signed-off-by: Hans de Goede >>> >>> Are you sure? I thought sysfs is supposed to flush out any pending >>> operations (they complete fast) and handle open fd internally? >> >> Yes, it "should" :) > > Thanks for confirming my vague memories :-) > > Hans, pls drop this one. Please see my earlier reply to your review of this patch, it is still needed but for a different reason: """ We still need this change though to make sure that the "drm/connector: Add drm_connector_find_by_fwnode() function" does not end up following a dangling drvdat pointer from one if the drm_connector kdev-s. The class_dev_iter_init() in drm_connector_find_by_fwnode() gets a reference on all devices and between getting that reference and it calling drm_connector_get() - drm_connector_unregister() may run and drop the possibly last reference to the drm_connector object, freeing it and leaving the kdev's drvdata as a dangling pointer. """ This is actually why I added it initially, and while adding it I came up with this wrong theory of why it was necessary independently of the drm_connector_find_by_fwnode() addition, sorry about that. Regards, Hans ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
Hi,
On 4/29/21 1:40 PM, Daniel Vetter wrote:
> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
>> Userspace could hold open a reference to the connector->kdev device,
>> through e.g. holding a sysfs-atrtribute open after
>> drm_sysfs_connector_remove() has been called. In this case the connector
>> could be free-ed while the connector->kdev device's drvdata is still
>> pointing to it.
>>
>> Give drm_connector devices there own device type, which allows
>> us to specify our own release function and make drm_sysfs_connector_add()
>> take a reference on the connector object, and have the new release
>> function put the reference when the device is released.
>>
>> Giving drm_connector devices there own device type, will also allow
>> checking if a device is a drm_connector device with a
>> "if (device->type == _sysfs_device_connector)" check.
>>
>> Note that the setting of the name member of the device_type struct will
>> cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector
>> as extra info. So this extends the uevent part of the userspace API.
>>
>> Signed-off-by: Hans de Goede
>
> Are you sure? I thought sysfs is supposed to flush out any pending
> operations (they complete fast) and handle open fd internally?
So I did some digging in fs/kernfs and it looks like you right,
once the file has been removed from sysfs any accesses through an
open fd will fail with -ENODEV, interesting I did not know this.
We still need this change though to make sure that the
"drm/connector: Add drm_connector_find_by_fwnode() function"
does not end up following a dangling drvdat pointer from one
if the drm_connector kdev-s.
The class_dev_iter_init() in drm_connector_find_by_fwnode() gets
a reference on all devices and between getting that reference
and it calling drm_connector_get() - drm_connector_unregister()
may run and drop the possibly last reference to the
drm_connector object, freeing it and leaving the kdev's
drvdata as a dangling pointer.
But I obviously need to rewrite the commit message of this
commit as it currently is completely wrong.
Maybe I should even squash this into the commit adding
drm_connector_find_by_fwnode() ?
Note sure about that though I personally think this is best
kept as a new preparation patch but with a new commit msg.
> Also I'd assume this creates a loop since the connector holds a reference
> on the kdev?
So I was wondering the same thing when working on this code and
I checked. the reference on the kdev is dropped from:
drm_connector_unregister() -> drm_sysfs_connector_remove()
and then happens independent of the reference count on the
connector-drm-obj dropping to 0.
So what this change does is it keeps a reference to the
drm_connector obj as long as someone is keeping a reference
to the connnector->kdev device around after drm_connector_unregister()
but as soon as that kdev device ref is dropped, so will the
drm_connector's obj reference.
I also tested this using a dock with DP MST, which dynamically
adds/removes connectors on plug-in / plug-out of the dock-cable
and I added a printk to the new drm_sysfs_connector_release() this
adds and that printk triggered pretty much immediately on unplug
as expected, releasing the extra drm_connector obj as soon as
drm_connector_unregister() got called.
Regards,
Hans
> -Daniel
>
>> ---
>> drivers/gpu/drm/drm_sysfs.c | 54 +++--
>> 1 file changed, 46 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index f0336c804639..c344c6d5e738 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -50,6 +50,10 @@ static struct device_type drm_sysfs_device_minor = {
>> .name = "drm_minor"
>> };
>>
>> +static struct device_type drm_sysfs_device_connector = {
>> +.name = "drm_connector",
>> +};
>> +
>> struct class *drm_class;
>>
>> static char *drm_devnode(struct device *dev, umode_t *mode)
>> @@ -271,30 +275,64 @@ static const struct attribute_group
>> *connector_dev_groups[] = {
>> NULL
>> };
>>
>> +static void drm_sysfs_connector_release(struct device *dev)
>> +{
>> +struct drm_connector *connector = to_drm_connector(dev);
>> +
>> +drm_connector_put(connector);
>> +kfree(dev);
>> +}
>> +
>> int drm_sysfs_connector_add(struct drm_connector *connector)
>> {
>> struct drm_device *dev = connector->dev;
>> +struct device *kdev;
>> +int r;
>>
>> if (connector->kdev)
>> return 0;
>>
>> -connector->kdev =
>> -device_create_with_groups(drm_class, dev->primary->kdev, 0,
>> - connector, connector_dev_groups,
>> - "card%d-%s", dev->primary->index,
>> - connector->name);
>> +kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
>> +if (!kdev)
>> +return -ENOMEM;
>> +
>> +
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote: > On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote: > > On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote: > > > Userspace could hold open a reference to the connector->kdev device, > > > through e.g. holding a sysfs-atrtribute open after > > > drm_sysfs_connector_remove() has been called. In this case the connector > > > could be free-ed while the connector->kdev device's drvdata is still > > > pointing to it. > > > > > > Give drm_connector devices there own device type, which allows > > > us to specify our own release function and make drm_sysfs_connector_add() > > > take a reference on the connector object, and have the new release > > > function put the reference when the device is released. > > > > > > Giving drm_connector devices there own device type, will also allow > > > checking if a device is a drm_connector device with a > > > "if (device->type == _sysfs_device_connector)" check. > > > > > > Note that the setting of the name member of the device_type struct will > > > cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector > > > as extra info. So this extends the uevent part of the userspace API. > > > > > > Signed-off-by: Hans de Goede > > > > Are you sure? I thought sysfs is supposed to flush out any pending > > operations (they complete fast) and handle open fd internally? > > Yes, it "should" :) Thanks for confirming my vague memories :-) Hans, pls drop this one. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote: > On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote: > > Userspace could hold open a reference to the connector->kdev device, > > through e.g. holding a sysfs-atrtribute open after > > drm_sysfs_connector_remove() has been called. In this case the connector > > could be free-ed while the connector->kdev device's drvdata is still > > pointing to it. > > > > Give drm_connector devices there own device type, which allows > > us to specify our own release function and make drm_sysfs_connector_add() > > take a reference on the connector object, and have the new release > > function put the reference when the device is released. > > > > Giving drm_connector devices there own device type, will also allow > > checking if a device is a drm_connector device with a > > "if (device->type == _sysfs_device_connector)" check. > > > > Note that the setting of the name member of the device_type struct will > > cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector > > as extra info. So this extends the uevent part of the userspace API. > > > > Signed-off-by: Hans de Goede > > Are you sure? I thought sysfs is supposed to flush out any pending > operations (they complete fast) and handle open fd internally? Yes, it "should" :) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote:
> Userspace could hold open a reference to the connector->kdev device,
> through e.g. holding a sysfs-atrtribute open after
> drm_sysfs_connector_remove() has been called. In this case the connector
> could be free-ed while the connector->kdev device's drvdata is still
> pointing to it.
>
> Give drm_connector devices there own device type, which allows
> us to specify our own release function and make drm_sysfs_connector_add()
> take a reference on the connector object, and have the new release
> function put the reference when the device is released.
>
> Giving drm_connector devices there own device type, will also allow
> checking if a device is a drm_connector device with a
> "if (device->type == _sysfs_device_connector)" check.
>
> Note that the setting of the name member of the device_type struct will
> cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector
> as extra info. So this extends the uevent part of the userspace API.
>
> Signed-off-by: Hans de Goede
Are you sure? I thought sysfs is supposed to flush out any pending
operations (they complete fast) and handle open fd internally?
Also I'd assume this creates a loop since the connector holds a reference
on the kdev?
-Daniel
> ---
> drivers/gpu/drm/drm_sysfs.c | 54 +++--
> 1 file changed, 46 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index f0336c804639..c344c6d5e738 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -50,6 +50,10 @@ static struct device_type drm_sysfs_device_minor = {
> .name = "drm_minor"
> };
>
> +static struct device_type drm_sysfs_device_connector = {
> + .name = "drm_connector",
> +};
> +
> struct class *drm_class;
>
> static char *drm_devnode(struct device *dev, umode_t *mode)
> @@ -271,30 +275,64 @@ static const struct attribute_group
> *connector_dev_groups[] = {
> NULL
> };
>
> +static void drm_sysfs_connector_release(struct device *dev)
> +{
> + struct drm_connector *connector = to_drm_connector(dev);
> +
> + drm_connector_put(connector);
> + kfree(dev);
> +}
> +
> int drm_sysfs_connector_add(struct drm_connector *connector)
> {
> struct drm_device *dev = connector->dev;
> + struct device *kdev;
> + int r;
>
> if (connector->kdev)
> return 0;
>
> - connector->kdev =
> - device_create_with_groups(drm_class, dev->primary->kdev, 0,
> - connector, connector_dev_groups,
> - "card%d-%s", dev->primary->index,
> - connector->name);
> + kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
> + if (!kdev)
> + return -ENOMEM;
> +
> + device_initialize(kdev);
> + kdev->class = drm_class;
> + kdev->type = _sysfs_device_connector;
> + kdev->parent = dev->primary->kdev;
> + kdev->groups = connector_dev_groups;
> + kdev->release = drm_sysfs_connector_release;
> + dev_set_drvdata(kdev, connector);
> +
> + r = dev_set_name(kdev, "card%d-%s", dev->primary->index,
> connector->name);
> + if (r)
> + goto err_free;
> +
> DRM_DEBUG("adding \"%s\" to sysfs\n",
> connector->name);
>
> - if (IS_ERR(connector->kdev)) {
> - DRM_ERROR("failed to register connector device: %ld\n",
> PTR_ERR(connector->kdev));
> - return PTR_ERR(connector->kdev);
> + r = device_add(kdev);
> + if (r) {
> + DRM_ERROR("failed to register connector device: %d\n", r);
> + goto err_free;
> }
>
> + /*
> + * Ensure the connector object does not get free-ed if userspace still
> has
> + * references open to the device through e.g. the connector
> sysfs-attributes.
> + */
> + drm_connector_get(connector);
> +
> + connector->kdev = kdev;
> +
> if (connector->ddc)
> return sysfs_create_link(>kdev->kobj,
>>ddc->dev.kobj, "ddc");
> return 0;
> +
> +err_free:
> + put_device(kdev);
> + return r;
> }
>
> void drm_sysfs_connector_remove(struct drm_connector *connector)
> --
> 2.31.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/9] drm/connector: Make the drm_sysfs connector->kdev device hold a reference to the connector
Userspace could hold open a reference to the connector->kdev device,
through e.g. holding a sysfs-atrtribute open after
drm_sysfs_connector_remove() has been called. In this case the connector
could be free-ed while the connector->kdev device's drvdata is still
pointing to it.
Give drm_connector devices there own device type, which allows
us to specify our own release function and make drm_sysfs_connector_add()
take a reference on the connector object, and have the new release
function put the reference when the device is released.
Giving drm_connector devices there own device type, will also allow
checking if a device is a drm_connector device with a
"if (device->type == _sysfs_device_connector)" check.
Note that the setting of the name member of the device_type struct will
cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector
as extra info. So this extends the uevent part of the userspace API.
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_sysfs.c | 54 +++--
1 file changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index f0336c804639..c344c6d5e738 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -50,6 +50,10 @@ static struct device_type drm_sysfs_device_minor = {
.name = "drm_minor"
};
+static struct device_type drm_sysfs_device_connector = {
+ .name = "drm_connector",
+};
+
struct class *drm_class;
static char *drm_devnode(struct device *dev, umode_t *mode)
@@ -271,30 +275,64 @@ static const struct attribute_group
*connector_dev_groups[] = {
NULL
};
+static void drm_sysfs_connector_release(struct device *dev)
+{
+ struct drm_connector *connector = to_drm_connector(dev);
+
+ drm_connector_put(connector);
+ kfree(dev);
+}
+
int drm_sysfs_connector_add(struct drm_connector *connector)
{
struct drm_device *dev = connector->dev;
+ struct device *kdev;
+ int r;
if (connector->kdev)
return 0;
- connector->kdev =
- device_create_with_groups(drm_class, dev->primary->kdev, 0,
- connector, connector_dev_groups,
- "card%d-%s", dev->primary->index,
- connector->name);
+ kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
+ if (!kdev)
+ return -ENOMEM;
+
+ device_initialize(kdev);
+ kdev->class = drm_class;
+ kdev->type = _sysfs_device_connector;
+ kdev->parent = dev->primary->kdev;
+ kdev->groups = connector_dev_groups;
+ kdev->release = drm_sysfs_connector_release;
+ dev_set_drvdata(kdev, connector);
+
+ r = dev_set_name(kdev, "card%d-%s", dev->primary->index,
connector->name);
+ if (r)
+ goto err_free;
+
DRM_DEBUG("adding \"%s\" to sysfs\n",
connector->name);
- if (IS_ERR(connector->kdev)) {
- DRM_ERROR("failed to register connector device: %ld\n",
PTR_ERR(connector->kdev));
- return PTR_ERR(connector->kdev);
+ r = device_add(kdev);
+ if (r) {
+ DRM_ERROR("failed to register connector device: %d\n", r);
+ goto err_free;
}
+ /*
+* Ensure the connector object does not get free-ed if userspace still
has
+* references open to the device through e.g. the connector
sysfs-attributes.
+*/
+ drm_connector_get(connector);
+
+ connector->kdev = kdev;
+
if (connector->ddc)
return sysfs_create_link(>kdev->kobj,
>ddc->dev.kobj, "ddc");
return 0;
+
+err_free:
+ put_device(kdev);
+ return r;
}
void drm_sysfs_connector_remove(struct drm_connector *connector)
--
2.31.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
