| Message ID | 20170509153654.23464-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: > Disadvantages: > * depending on userspace architecture, layers left on screen could > be considered an information leak, ie. new incoming master process > has access to buffers that are still being scanned out. I'm not sure this is much of a problem really, or at least I suspect we have bigger issues: the GETFB ioctl allows you to get at the gem bo behind any framebuffer, as long as you're the current master. There's no need for that framebuffer to be active on the screen. Not sure that's a good idea really, we might want to fix up that ioctl to only hand out the backing storage objects for currently active objects. But kinda separate issue. Other -Daniel
On Wed, May 10, 2017 at 8:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: >> Disadvantages: >> * depending on userspace architecture, layers left on screen could >> be considered an information leak, ie. new incoming master process >> has access to buffers that are still being scanned out. > > I'm not sure this is much of a problem really, or at least I suspect > we have bigger issues: the GETFB ioctl allows you to get at the gem bo > behind any framebuffer, as long as you're the current master. There's > no need for that framebuffer to be active on the screen. Not sure > that's a good idea really, we might want to fix up that ioctl to only > hand out the backing storage objects for currently active objects. But > kinda separate issue. > > Other Oops, hit send too early: Otherwise looks good. Well, you can forgo the kernel-doc (just leave a comment if you want to explain the difference), since in drm core only the driver interface stuff is documented with kernel-doc. At least that's what I've been doing. -Daniel
On Wed, May 10, 2017 at 2:27 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, May 10, 2017 at 8:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: >>> Disadvantages: >>> * depending on userspace architecture, layers left on screen could >>> be considered an information leak, ie. new incoming master process >>> has access to buffers that are still being scanned out. >> >> I'm not sure this is much of a problem really, or at least I suspect >> we have bigger issues: the GETFB ioctl allows you to get at the gem bo >> behind any framebuffer, as long as you're the current master. There's >> no need for that framebuffer to be active on the screen. Not sure >> that's a good idea really, we might want to fix up that ioctl to only >> hand out the backing storage objects for currently active objects. But >> kinda separate issue. hmm, it would seem unusual for incoming process to inherit any fb's *other* than what is on screen (what else would be holding a ref to keep them live?) It did occur to me that we could keep a list of "weakly ref'd" fb's in drm_file to kill at lastclose. Not entirely sure if that is useful or not.. would need feedback from various userspaces. (I did leave room for a flags field, so we could add kill-on-close flag later.) BR, -R >> Other > > Oops, hit send too early: Otherwise looks good. Well, you can forgo > the kernel-doc (just leave a comment if you want to explain the > difference), since in drm core only the driver interface stuff is > documented with kernel-doc. At least that's what I've been doing. > > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, May 10, 2017 at 1:24 PM, Rob Clark <robdclark@gmail.com> wrote: > On Wed, May 10, 2017 at 2:27 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Wed, May 10, 2017 at 8:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: >>>> Disadvantages: >>>> * depending on userspace architecture, layers left on screen could >>>> be considered an information leak, ie. new incoming master process >>>> has access to buffers that are still being scanned out. >>> >>> I'm not sure this is much of a problem really, or at least I suspect >>> we have bigger issues: the GETFB ioctl allows you to get at the gem bo >>> behind any framebuffer, as long as you're the current master. There's >>> no need for that framebuffer to be active on the screen. Not sure >>> that's a good idea really, we might want to fix up that ioctl to only >>> hand out the backing storage objects for currently active objects. But >>> kinda separate issue. > > hmm, it would seem unusual for incoming process to inherit any fb's > *other* than what is on screen (what else would be holding a ref to > keep them live?) This was an observation about the existing code, not your new ioctl. If you vt-switch between different users, and the other X.org server keeps framebuffer objects around after vt-leave, then the current master can read the contents of those. I assume that userspace is not 100% dutiful with getting this right, and it might be easier to plug this hole in the kernel. This is not about the case where the other compositor quits, but keeps running in the background. -Daniel
On Wed, May 10, 2017 at 08:26:42AM +0200, Daniel Vetter wrote: > On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: > > Disadvantages: > > * depending on userspace architecture, layers left on screen could > > be considered an information leak, ie. new incoming master process > > has access to buffers that are still being scanned out. > > I'm not sure this is much of a problem really, Agreed. I've been under the impression that relying on pipe cleanup in rmfb is a somewhat inelegant thing to do anyways. One bikeshed comment I have is with the name. We've moved everything from *_unreference to *_put. To stay consistent with that, as well as balance out GETFB, could we rename to PUTFB? Sean > or at least I suspect > we have bigger issues: the GETFB ioctl allows you to get at the gem bo > behind any framebuffer, as long as you're the current master. There's > no need for that framebuffer to be active on the screen. Not sure > that's a good idea really, we might want to fix up that ioctl to only > hand out the backing storage objects for currently active objects. But > kinda separate issue. > > Other > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, May 10, 2017 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote: > On Wed, May 10, 2017 at 08:26:42AM +0200, Daniel Vetter wrote: >> On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: >> > Disadvantages: >> > * depending on userspace architecture, layers left on screen could >> > be considered an information leak, ie. new incoming master process >> > has access to buffers that are still being scanned out. >> >> I'm not sure this is much of a problem really, > > Agreed. I've been under the impression that relying on pipe cleanup in rmfb is a > somewhat inelegant thing to do anyways. > > One bikeshed comment I have is with the name. We've moved everything from > *_unreference to *_put. To stay consistent with that, as well as balance out > GETFB, could we rename to PUTFB? In general, I'm ok w/ PUTFB as the name.. except GETFB is actually more like GETFBINFO (ie. it doesn't take a ref).. so calling it PUTFB implies it is the counterpart to GETFB when it really isn't. (I suppose technically we could rename GETFB to something else without breaking ABI, and since we copy the headers into libdrm it might not be *that* big a pita..) BR, -R > Sean > >> or at least I suspect >> we have bigger issues: the GETFB ioctl allows you to get at the gem bo >> behind any framebuffer, as long as you're the current master. There's >> no need for that framebuffer to be active on the screen. Not sure >> that's a good idea really, we might want to fix up that ioctl to only >> hand out the backing storage objects for currently active objects. But >> kinda separate issue. >> >> Other >> -Daniel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Sean Paul, Software Engineer, Google / Chromium OS
On Wed, May 10, 2017 at 10:20:09AM -0400, Rob Clark wrote: > On Wed, May 10, 2017 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote: > > On Wed, May 10, 2017 at 08:26:42AM +0200, Daniel Vetter wrote: > >> On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: > >> > Disadvantages: > >> > * depending on userspace architecture, layers left on screen could > >> > be considered an information leak, ie. new incoming master process > >> > has access to buffers that are still being scanned out. > >> > >> I'm not sure this is much of a problem really, > > > > Agreed. I've been under the impression that relying on pipe cleanup in rmfb is a > > somewhat inelegant thing to do anyways. > > > > One bikeshed comment I have is with the name. We've moved everything from > > *_unreference to *_put. To stay consistent with that, as well as balance out > > GETFB, could we rename to PUTFB? > > In general, I'm ok w/ PUTFB as the name.. except GETFB is actually > more like GETFBINFO (ie. it doesn't take a ref).. so calling it PUTFB > implies it is the counterpart to GETFB when it really isn't. > > (I suppose technically we could rename GETFB to something else without > breaking ABI, and since we copy the headers into libdrm it might not > be *that* big a pita..) If we bikeshed the name, CLOSEFB? It is not an unreference operation, since the drm_file fb reference is not refcounted. The underlying fb is, but not the per-file reference (which is the thing we nuke here). This is similar to closing files, which will close it even if you have piles of other references to the same fd in userspace (but the underlying struct file in the kernel might or might not disappear). Seems more fitting to me than either UNREF or PUT. Also fits with GEM_CLOSE on the gem side, which works the same way (it just drops the drm_file reference, nothing else). -Daniel
On Wed, May 10, 2017 at 11:11 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, May 10, 2017 at 10:20:09AM -0400, Rob Clark wrote: >> On Wed, May 10, 2017 at 9:48 AM, Sean Paul <seanpaul@chromium.org> wrote: >> > On Wed, May 10, 2017 at 08:26:42AM +0200, Daniel Vetter wrote: >> >> On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: >> >> > Disadvantages: >> >> > * depending on userspace architecture, layers left on screen could >> >> > be considered an information leak, ie. new incoming master process >> >> > has access to buffers that are still being scanned out. >> >> >> >> I'm not sure this is much of a problem really, >> > >> > Agreed. I've been under the impression that relying on pipe cleanup in rmfb is a >> > somewhat inelegant thing to do anyways. >> > >> > One bikeshed comment I have is with the name. We've moved everything from >> > *_unreference to *_put. To stay consistent with that, as well as balance out >> > GETFB, could we rename to PUTFB? >> >> In general, I'm ok w/ PUTFB as the name.. except GETFB is actually >> more like GETFBINFO (ie. it doesn't take a ref).. so calling it PUTFB >> implies it is the counterpart to GETFB when it really isn't. >> >> (I suppose technically we could rename GETFB to something else without >> breaking ABI, and since we copy the headers into libdrm it might not >> be *that* big a pita..) > > If we bikeshed the name, CLOSEFB? It is not an unreference operation, > since the drm_file fb reference is not refcounted. The underlying fb is, > but not the per-file reference (which is the thing we nuke here). > > This is similar to closing files, which will close it even if you have > piles of other references to the same fd in userspace (but the underlying > struct file in the kernel might or might not disappear). Seems more > fitting to me than either UNREF or PUT. Also fits with GEM_CLOSE on the > gem side, which works the same way (it just drops the drm_file reference, > nothing else). yeah, I like CLOSEFB BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On 10/05/17 10:16 PM, Daniel Vetter wrote: > On Wed, May 10, 2017 at 1:24 PM, Rob Clark <robdclark@gmail.com> wrote: >> On Wed, May 10, 2017 at 2:27 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Wed, May 10, 2017 at 8:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: >>>>> Disadvantages: >>>>> * depending on userspace architecture, layers left on screen could >>>>> be considered an information leak, ie. new incoming master process >>>>> has access to buffers that are still being scanned out. >>>> >>>> I'm not sure this is much of a problem really, or at least I suspect >>>> we have bigger issues: the GETFB ioctl allows you to get at the gem bo >>>> behind any framebuffer, as long as you're the current master. There's >>>> no need for that framebuffer to be active on the screen. Not sure >>>> that's a good idea really, we might want to fix up that ioctl to only >>>> hand out the backing storage objects for currently active objects. But >>>> kinda separate issue. >> >> hmm, it would seem unusual for incoming process to inherit any fb's >> *other* than what is on screen (what else would be holding a ref to >> keep them live?) > > This was an observation about the existing code, not your new ioctl. > If you vt-switch between different users, and the other X.org server > keeps framebuffer objects around after vt-leave, then the current > master can read the contents of those. I assume that userspace is not > 100% dutiful with getting this right, and it might be easier to plug > this hole in the kernel. This is not about the case where the other > compositor quits, but keeps running in the background. Even with your suggested change though, the new master would presumably be able to access at least the last FBs scanned out by the old master? The old master could only prevent that by turning off all CRTCs, which would make VT switching ugly and slow.
On Thu, May 11, 2017 at 04:45:09PM +0900, Michel Dänzer wrote: > On 10/05/17 10:16 PM, Daniel Vetter wrote: > > On Wed, May 10, 2017 at 1:24 PM, Rob Clark <robdclark@gmail.com> wrote: > >> On Wed, May 10, 2017 at 2:27 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>> On Wed, May 10, 2017 at 8:26 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > >>>> On Tue, May 9, 2017 at 5:36 PM, Rob Clark <robdclark@gmail.com> wrote: > >>>>> Disadvantages: > >>>>> * depending on userspace architecture, layers left on screen could > >>>>> be considered an information leak, ie. new incoming master process > >>>>> has access to buffers that are still being scanned out. > >>>> > >>>> I'm not sure this is much of a problem really, or at least I suspect > >>>> we have bigger issues: the GETFB ioctl allows you to get at the gem bo > >>>> behind any framebuffer, as long as you're the current master. There's > >>>> no need for that framebuffer to be active on the screen. Not sure > >>>> that's a good idea really, we might want to fix up that ioctl to only > >>>> hand out the backing storage objects for currently active objects. But > >>>> kinda separate issue. > >> > >> hmm, it would seem unusual for incoming process to inherit any fb's > >> *other* than what is on screen (what else would be holding a ref to > >> keep them live?) > > > > This was an observation about the existing code, not your new ioctl. > > If you vt-switch between different users, and the other X.org server > > keeps framebuffer objects around after vt-leave, then the current > > master can read the contents of those. I assume that userspace is not > > 100% dutiful with getting this right, and it might be easier to plug > > this hole in the kernel. This is not about the case where the other > > compositor quits, but keeps running in the background. > > Even with your suggested change though, the new master would presumably > be able to access at least the last FBs scanned out by the old master? > The old master could only prevent that by turning off all CRTCs, which > would make VT switching ugly and slow. RMFB is nothing else than the proposed CLOSEFB here plus disabling all CRTC (if the fb shows somewhere). VT switching is still slow if you want to make sure your fbs don't show up anywhere, whether the kernel shuts down the display for you, or whether you do it explicitly. The idea behind CLOSEFB is that you switch to an fb without anything interesting on it (fade to black), then CLOSEFB (but leave the CRTC on), so that the other compositor can take over without a full modeset. Atm you have to do that by keeping the fb around until after the other compositor has taken over, which is kinda silly. -Daniel
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 8c04275..dafa17b 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -167,6 +167,8 @@ int drm_mode_addfb2(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_mode_rmfb(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_mode_unref_fb(struct drm_device *dev, + void *data, struct drm_file *file_priv); int drm_mode_getfb(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_mode_dirtyfb_ioctl(struct drm_device *dev, diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index e8f9c13..8f2afdf 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -356,31 +356,17 @@ static void drm_mode_rmfb_work_fn(struct work_struct *w) } } -/** - * drm_mode_rmfb - remove an FB from the configuration - * @dev: drm device for the ioctl - * @data: data pointer for the ioctl - * @file_priv: drm file for the ioctl call - * - * Remove the FB specified by the user. - * - * Called by the user via ioctl. - * - * Returns: - * Zero on success, negative errno on failure. - */ -int drm_mode_rmfb(struct drm_device *dev, - void *data, struct drm_file *file_priv) +static int __rmfb(struct drm_device *dev, struct drm_file *file_priv, + uint32_t fb_id, bool rmfb) { struct drm_framebuffer *fb = NULL; struct drm_framebuffer *fbl = NULL; - uint32_t *id = data; int found = 0; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; - fb = drm_framebuffer_lookup(dev, *id); + fb = drm_framebuffer_lookup(dev, fb_id); if (!fb) return -ENOENT; @@ -406,7 +392,7 @@ int drm_mode_rmfb(struct drm_device *dev, * so run this in a separate stack as there's no way to correctly * handle this after the fb is already removed from the lookup table. */ - if (drm_framebuffer_read_refcount(fb) > 1) { + if (rmfb && drm_framebuffer_read_refcount(fb) > 1) { struct drm_mode_rmfb_work arg; INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn); @@ -427,6 +413,50 @@ int drm_mode_rmfb(struct drm_device *dev, } /** + * drm_mode_rmfb - remove an FB from the configuration + * @dev: drm device for the ioctl + * @data: data pointer for the ioctl + * @file_priv: drm file for the ioctl call + * + * Remove the FB specified by the user. + * + * Called by the user via ioctl. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_mode_rmfb(struct drm_device *dev, + void *data, struct drm_file *file_priv) +{ + uint32_t *id = data; + return __rmfb(dev, file_priv, *id, true); +} + +/** + * drm_mode_unref_fb - unreference an FB + * @dev: drm device for the ioctl + * @data: data pointer for the ioctl + * @file_priv: drm file for the ioctl call + * + * Unreference the FB specified by the user. + * + * Called by the user via ioctl. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_mode_unref_fb(struct drm_device *dev, void *data, + struct drm_file *file_priv) +{ + struct drm_mode_unref_fb *r = data; + + if (r->pad) + return -EINVAL; + + return __rmfb(dev, file_priv, r->fb_id, false); +} + +/** * drm_mode_getfb - get FB info * @dev: drm device for the ioctl * @data: data pointer for the ioctl diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 7d6deaa..a113972 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -642,6 +642,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_UNREFFB, drm_mode_unref_fb, DRM_CONTROL_ALLOW|DRM_UNLOCKED), }; #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index b2c5284..b485253 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -813,6 +813,7 @@ extern "C" { #define DRM_IOCTL_MODE_ATOMIC DRM_IOWR(0xBC, struct drm_mode_atomic) #define DRM_IOCTL_MODE_CREATEPROPBLOB DRM_IOWR(0xBD, struct drm_mode_create_blob) #define DRM_IOCTL_MODE_DESTROYPROPBLOB DRM_IOWR(0xBE, struct drm_mode_destroy_blob) +#define DRM_IOCTL_MODE_UNREFFB DRM_IOR( 0xBF, struct drm_mode_unref_fb) /** * Device specific ioctls should only be in their respective headers diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 8c67fc0..1804b2d 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -685,6 +685,26 @@ struct drm_mode_destroy_blob { __u32 blob_id; }; +/** + * Similar to rmfb but does not have the side effect of shutting down any + * pipes that are scanning out the fb. + * + * Advantages compared to rmfb: + * * slightly easier userspace, it doesn't have to track fb-id's until + * they come of the screen + * * it might be desirable to keep existing layers on screen across + * process restart (for crashing or upgrading the compositor) + * + * Disadvantages: + * * depending on userspace architecture, layers left on screen could + * be considered an information leak, ie. new incoming master process + * has access to buffers that are still being scanned out. + */ +struct drm_mode_unref_fb { + __u32 pad; /* must be zero for now.. */ + __u32 fb_id; +}; + #if defined(__cplusplus) } #endif
Similar to rmfb but does not have the side effect of shutting down any pipes that are scanning out the fb. Advantages compared to rmfb: * slightly easier userspace, it doesn't have to track fb-id's until they come of the screen * it might be desirable to keep existing layers on screen across process restart (for crashing or upgrading the compositor) Disadvantages: * depending on userspace architecture, layers left on screen could be considered an information leak, ie. new incoming master process has access to buffers that are still being scanned out. Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/gpu/drm/drm_crtc_internal.h | 2 ++ drivers/gpu/drm/drm_framebuffer.c | 66 +++++++++++++++++++++++++++---------- drivers/gpu/drm/drm_ioctl.c | 1 + include/uapi/drm/drm.h | 1 + include/uapi/drm/drm_mode.h | 20 +++++++++++ 5 files changed, 72 insertions(+), 18 deletions(-)