| Message ID | 1409313661-20696-2-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 29, 2014 at 02:01:01PM +0200, David Herrmann wrote: > With MST, we now have connector hotplugging, yey! Pretty easy to use in > user-space, but introduces some nasty races: > * If a connector is removed and added again while a compositor is in > background, it will get the same ID again. If the compositor wakes up, > it cannot know whether its the same connector or a new one, thus they > must re-read EDID information, etc. > * possible_clones, possible_crtcs, etc. depend on indices, not IDs. So if > an object is removed and a new one is added, those bitmasks are invalid > and must be refreshed. This currently does not affect connectors, but > only crtcs and encoders, but it's only a matter of time when this will > change. > > The easiest way to protect against this, is to not recylce modeset IDs. > Instead, always allocate a new, higher ID. All ioctls that affect modeset > objects, take IDs. Thus, during hotplug races, those ioctls will simply > fail if invalid IDs are passed. They will no longer silently run on a > newly hotplugged object. > > Furthermore, hotplug-races during state sync can now be easily detected. A > call to GET_RESOURCES returns a list of available IDs atomically. > User-space can now start fetching all those objects via GET_* ioctls. If > any of those fails, they know that the given object was unplugged. Thus, > the "possible_*" bit-fields are invalidated. User-space can now decide > whether to restart the sync all over or wait for the 'change' uevent that > is sent on modeset object modifications (or, well, is supposed to be sent > and probably will be at some point). > > With this in place, modeset object hotplugging should work fine for all > modeset objects in the KMS API. > > CC'ing stable so we can rely on all kernels with MST to not recycle IDs. > > Cc: <stable@vger.kernel.org> # 3.16+ > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> So userspace just needs to cycle through piles of framebuffer objects to make bad things happen? Doesn't sound like a terribly solid plan. I guess we could save this by doing normal id allocations for fbs and monotonically increasing allocations for all other objects. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 97eba56..ab0fe6d 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -286,7 +286,7 @@ static int drm_mode_object_get_reg(struct drm_device *dev, > idr_preload(GFP_KERNEL); > spin_lock_irq(&dev->mode_config.idr_lock); > > - ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT); > + ret = idr_alloc_cyclic(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT); > if (ret >= 0) { > /* > * Set up the object linking under the protection of the idr > -- > 2.1.0 >
Hi On Fri, Aug 29, 2014 at 2:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Aug 29, 2014 at 02:01:01PM +0200, David Herrmann wrote: >> With MST, we now have connector hotplugging, yey! Pretty easy to use in >> user-space, but introduces some nasty races: >> * If a connector is removed and added again while a compositor is in >> background, it will get the same ID again. If the compositor wakes up, >> it cannot know whether its the same connector or a new one, thus they >> must re-read EDID information, etc. >> * possible_clones, possible_crtcs, etc. depend on indices, not IDs. So if >> an object is removed and a new one is added, those bitmasks are invalid >> and must be refreshed. This currently does not affect connectors, but >> only crtcs and encoders, but it's only a matter of time when this will >> change. >> >> The easiest way to protect against this, is to not recylce modeset IDs. >> Instead, always allocate a new, higher ID. All ioctls that affect modeset >> objects, take IDs. Thus, during hotplug races, those ioctls will simply >> fail if invalid IDs are passed. They will no longer silently run on a >> newly hotplugged object. >> >> Furthermore, hotplug-races during state sync can now be easily detected. A >> call to GET_RESOURCES returns a list of available IDs atomically. >> User-space can now start fetching all those objects via GET_* ioctls. If >> any of those fails, they know that the given object was unplugged. Thus, >> the "possible_*" bit-fields are invalidated. User-space can now decide >> whether to restart the sync all over or wait for the 'change' uevent that >> is sent on modeset object modifications (or, well, is supposed to be sent >> and probably will be at some point). >> >> With this in place, modeset object hotplugging should work fine for all >> modeset objects in the KMS API. >> >> CC'ing stable so we can rely on all kernels with MST to not recycle IDs. >> >> Cc: <stable@vger.kernel.org> # 3.16+ >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > > So userspace just needs to cycle through piles of framebuffer objects to > make bad things happen? Doesn't sound like a terribly solid plan. IDs will still get recycled, but only once all IDs got used. So this is "safe". Sure, user-space can create 2 billion framebuffers and destroy them again, thus causing the ID range to overflow and recycle old IDs. Not sure how fast you can run 2 billion syscalls.. If that's a real issue, I'd vote for using the high-range for user-space managed objects, low-range for kernel-space managed objects ([1...INT_MAX] and [INT_MAX+1...UINT_MAX] or so). > I guess we could save this by doing normal id allocations for fbs and > monotonically increasing allocations for all other objects. This doesn't work. A connector with ID #n might get unplugged and another process created a new FB, which will then get ID #n. Sure, I doubt there's a real conflict as ioctls check the type, but it still sounds really ugly to me. Thanks David
Hi On Fri, Aug 29, 2014 at 2:57 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Fri, Aug 29, 2014 at 2:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Fri, Aug 29, 2014 at 02:01:01PM +0200, David Herrmann wrote: >>> With MST, we now have connector hotplugging, yey! Pretty easy to use in >>> user-space, but introduces some nasty races: >>> * If a connector is removed and added again while a compositor is in >>> background, it will get the same ID again. If the compositor wakes up, >>> it cannot know whether its the same connector or a new one, thus they >>> must re-read EDID information, etc. >>> * possible_clones, possible_crtcs, etc. depend on indices, not IDs. So if >>> an object is removed and a new one is added, those bitmasks are invalid >>> and must be refreshed. This currently does not affect connectors, but >>> only crtcs and encoders, but it's only a matter of time when this will >>> change. >>> >>> The easiest way to protect against this, is to not recylce modeset IDs. >>> Instead, always allocate a new, higher ID. All ioctls that affect modeset >>> objects, take IDs. Thus, during hotplug races, those ioctls will simply >>> fail if invalid IDs are passed. They will no longer silently run on a >>> newly hotplugged object. >>> >>> Furthermore, hotplug-races during state sync can now be easily detected. A >>> call to GET_RESOURCES returns a list of available IDs atomically. >>> User-space can now start fetching all those objects via GET_* ioctls. If >>> any of those fails, they know that the given object was unplugged. Thus, >>> the "possible_*" bit-fields are invalidated. User-space can now decide >>> whether to restart the sync all over or wait for the 'change' uevent that >>> is sent on modeset object modifications (or, well, is supposed to be sent >>> and probably will be at some point). >>> >>> With this in place, modeset object hotplugging should work fine for all >>> modeset objects in the KMS API. >>> >>> CC'ing stable so we can rely on all kernels with MST to not recycle IDs. >>> >>> Cc: <stable@vger.kernel.org> # 3.16+ >>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >> >> So userspace just needs to cycle through piles of framebuffer objects to >> make bad things happen? Doesn't sound like a terribly solid plan. > > IDs will still get recycled, but only once all IDs got used. So this is "safe". > > Sure, user-space can create 2 billion framebuffers and destroy them > again, thus causing the ID range to overflow and recycle old IDs. Not > sure how fast you can run 2 billion syscalls.. If that's a real issue, > I'd vote for using the high-range for user-space managed objects, > low-range for kernel-space managed objects ([1...INT_MAX] and > [INT_MAX+1...UINT_MAX] or so). > >> I guess we could save this by doing normal id allocations for fbs and >> monotonically increasing allocations for all other objects. > > This doesn't work. A connector with ID #n might get unplugged and > another process created a new FB, which will then get ID #n. Sure, I > doubt there's a real conflict as ioctls check the type, but it still > sounds really ugly to me. On a second thought: maybe your idea isn't as bad as I thought. I mean, everyone must do type-checking when looking up mode-objects, so it seems safe to rely on that. Thanks David
On Fri, Aug 29, 2014 at 02:57:12PM +0200, David Herrmann wrote: > Hi > > On Fri, Aug 29, 2014 at 2:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Aug 29, 2014 at 02:01:01PM +0200, David Herrmann wrote: > >> With MST, we now have connector hotplugging, yey! Pretty easy to use in > >> user-space, but introduces some nasty races: > >> * If a connector is removed and added again while a compositor is in > >> background, it will get the same ID again. If the compositor wakes up, > >> it cannot know whether its the same connector or a new one, thus they > >> must re-read EDID information, etc. > >> * possible_clones, possible_crtcs, etc. depend on indices, not IDs. So if > >> an object is removed and a new one is added, those bitmasks are invalid > >> and must be refreshed. This currently does not affect connectors, but > >> only crtcs and encoders, but it's only a matter of time when this will > >> change. > >> > >> The easiest way to protect against this, is to not recylce modeset IDs. > >> Instead, always allocate a new, higher ID. All ioctls that affect modeset > >> objects, take IDs. Thus, during hotplug races, those ioctls will simply > >> fail if invalid IDs are passed. They will no longer silently run on a > >> newly hotplugged object. > >> > >> Furthermore, hotplug-races during state sync can now be easily detected. A > >> call to GET_RESOURCES returns a list of available IDs atomically. > >> User-space can now start fetching all those objects via GET_* ioctls. If > >> any of those fails, they know that the given object was unplugged. Thus, > >> the "possible_*" bit-fields are invalidated. User-space can now decide > >> whether to restart the sync all over or wait for the 'change' uevent that > >> is sent on modeset object modifications (or, well, is supposed to be sent > >> and probably will be at some point). > >> > >> With this in place, modeset object hotplugging should work fine for all > >> modeset objects in the KMS API. > >> > >> CC'ing stable so we can rely on all kernels with MST to not recycle IDs. > >> > >> Cc: <stable@vger.kernel.org> # 3.16+ > >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > > > > So userspace just needs to cycle through piles of framebuffer objects to > > make bad things happen? Doesn't sound like a terribly solid plan. > > IDs will still get recycled, but only once all IDs got used. So this is "safe". > > Sure, user-space can create 2 billion framebuffers and destroy them > again, thus causing the ID range to overflow and recycle old IDs. Not > sure how fast you can run 2 billion syscalls.. If that's a real issue, > I'd vote for using the high-range for user-space managed objects, > low-range for kernel-space managed objects ([1...INT_MAX] and > [INT_MAX+1...UINT_MAX] or so). > > > I guess we could save this by doing normal id allocations for fbs and > > monotonically increasing allocations for all other objects. > > This doesn't work. A connector with ID #n might get unplugged and > another process created a new FB, which will then get ID #n. Sure, I > doubt there's a real conflict as ioctls check the type, but it still > sounds really ugly to me. Oh, ugly for sure, but it should work since userspace doesn't really have any reason to mix up ids and the kernel does need to check the type anyway. And with a simple if (type == OBJ_FB) idr_alloc else idr_alloc_cyclic I think it should work out with minimal fuss. At least the full separation of id spaces wouldn't be any more or less complexity. -Daniel
On Fri, Aug 29, 2014 at 9:10 AM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Fri, Aug 29, 2014 at 2:57 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >> Hi >> >> On Fri, Aug 29, 2014 at 2:51 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Fri, Aug 29, 2014 at 02:01:01PM +0200, David Herrmann wrote: >>>> With MST, we now have connector hotplugging, yey! Pretty easy to use in >>>> user-space, but introduces some nasty races: >>>> * If a connector is removed and added again while a compositor is in >>>> background, it will get the same ID again. If the compositor wakes up, >>>> it cannot know whether its the same connector or a new one, thus they >>>> must re-read EDID information, etc. >>>> * possible_clones, possible_crtcs, etc. depend on indices, not IDs. So if >>>> an object is removed and a new one is added, those bitmasks are invalid >>>> and must be refreshed. This currently does not affect connectors, but >>>> only crtcs and encoders, but it's only a matter of time when this will >>>> change. >>>> >>>> The easiest way to protect against this, is to not recylce modeset IDs. >>>> Instead, always allocate a new, higher ID. All ioctls that affect modeset >>>> objects, take IDs. Thus, during hotplug races, those ioctls will simply >>>> fail if invalid IDs are passed. They will no longer silently run on a >>>> newly hotplugged object. >>>> >>>> Furthermore, hotplug-races during state sync can now be easily detected. A >>>> call to GET_RESOURCES returns a list of available IDs atomically. >>>> User-space can now start fetching all those objects via GET_* ioctls. If >>>> any of those fails, they know that the given object was unplugged. Thus, >>>> the "possible_*" bit-fields are invalidated. User-space can now decide >>>> whether to restart the sync all over or wait for the 'change' uevent that >>>> is sent on modeset object modifications (or, well, is supposed to be sent >>>> and probably will be at some point). >>>> >>>> With this in place, modeset object hotplugging should work fine for all >>>> modeset objects in the KMS API. >>>> >>>> CC'ing stable so we can rely on all kernels with MST to not recycle IDs. >>>> >>>> Cc: <stable@vger.kernel.org> # 3.16+ >>>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> >>> >>> So userspace just needs to cycle through piles of framebuffer objects to >>> make bad things happen? Doesn't sound like a terribly solid plan. >> >> IDs will still get recycled, but only once all IDs got used. So this is "safe". >> >> Sure, user-space can create 2 billion framebuffers and destroy them >> again, thus causing the ID range to overflow and recycle old IDs. Not >> sure how fast you can run 2 billion syscalls.. If that's a real issue, >> I'd vote for using the high-range for user-space managed objects, >> low-range for kernel-space managed objects ([1...INT_MAX] and >> [INT_MAX+1...UINT_MAX] or so). >> >>> I guess we could save this by doing normal id allocations for fbs and >>> monotonically increasing allocations for all other objects. >> >> This doesn't work. A connector with ID #n might get unplugged and >> another process created a new FB, which will then get ID #n. Sure, I >> doubt there's a real conflict as ioctls check the type, but it still >> sounds really ugly to me. > > On a second thought: maybe your idea isn't as bad as I thought. I > mean, everyone must do type-checking when looking up mode-objects, so > it seems safe to rely on that. note that for atomic there are cases where we do lookup w/ ANY_TYPE.. *but* the idea might still work since FB's are still handled specially(ish) do to refcnting.. BR, -R > Thanks > David > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 97eba56..ab0fe6d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -286,7 +286,7 @@ static int drm_mode_object_get_reg(struct drm_device *dev, idr_preload(GFP_KERNEL); spin_lock_irq(&dev->mode_config.idr_lock); - ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT); + ret = idr_alloc_cyclic(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, 1, 0, GFP_NOWAIT); if (ret >= 0) { /* * Set up the object linking under the protection of the idr
With MST, we now have connector hotplugging, yey! Pretty easy to use in user-space, but introduces some nasty races: * If a connector is removed and added again while a compositor is in background, it will get the same ID again. If the compositor wakes up, it cannot know whether its the same connector or a new one, thus they must re-read EDID information, etc. * possible_clones, possible_crtcs, etc. depend on indices, not IDs. So if an object is removed and a new one is added, those bitmasks are invalid and must be refreshed. This currently does not affect connectors, but only crtcs and encoders, but it's only a matter of time when this will change. The easiest way to protect against this, is to not recylce modeset IDs. Instead, always allocate a new, higher ID. All ioctls that affect modeset objects, take IDs. Thus, during hotplug races, those ioctls will simply fail if invalid IDs are passed. They will no longer silently run on a newly hotplugged object. Furthermore, hotplug-races during state sync can now be easily detected. A call to GET_RESOURCES returns a list of available IDs atomically. User-space can now start fetching all those objects via GET_* ioctls. If any of those fails, they know that the given object was unplugged. Thus, the "possible_*" bit-fields are invalidated. User-space can now decide whether to restart the sync all over or wait for the 'change' uevent that is sent on modeset object modifications (or, well, is supposed to be sent and probably will be at some point). With this in place, modeset object hotplugging should work fine for all modeset objects in the KMS API. CC'ing stable so we can rely on all kernels with MST to not recycle IDs. Cc: <stable@vger.kernel.org> # 3.16+ Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)