| Message ID | 20211109085601.170275-1-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: document DRM_IOCTL_MODE_GETFB2 | expand |
On Tue, Nov 09, 2021 at 08:56:10AM +0000, Simon Ser wrote: > There are a few details specific to the GETFB2 IOCTL. > > It's not immediately clear how user-space should check for the > number of planes. Suggest using the pitches field. > > The modifier array is filled with zeroes, ie. DRM_FORMAT_MOD_LINEAR. > So explicitly tell user-space to not look at it unless the flag is > set. > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > --- > include/uapi/drm/drm.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 3b810b53ba8b..9809078b0f51 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -1096,6 +1096,22 @@ extern "C" { > #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct drm_syncobj_transfer) > #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) > > +/** > + * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata. > + * > + * This queries metadata about a framebuffer. User-space fills > + * &drm_mode_fb_cmd2.fb_id as the input, and the kernels fills the rest of the > + * struct as the output. > + * > + * If the client is not DRM master and doesn't have &CAP_SYS_ADMIN, > + * &drm_mode_fb_cmd2.handles will be zeroed. Planes are valid until one has a > + * zero &drm_mode_fb_cmd2.pitches -- this can be used to compute the number of > + * planes. Maybe explain that when actually importing the buffer userspace should look for non-zeor handles instead, since some drivers/formats leave a silly pitch value behind. Or at least I think that can happen. Just for additional robustness? > + * > + * If the framebuffer has a format modifier, &DRM_MODE_FB_MODIFIERS will be set > + * in &drm_mode_fb_cmd2.flags. Otherwise, &drm_mode_fb_cmd2.modifier has > + * undefined contents. Uh is this true? We should always clear values to avoid accidental leaks and stuff. > + */ I think kerneldoc for drm_mode_fb_cmd2 would be neat too, so we can document how pitch is supposed to work and all that stuff. Anyway lgtm otherwise. -Daniel > #define DRM_IOCTL_MODE_GETFB2 DRM_IOWR(0xCE, struct drm_mode_fb_cmd2) > > /* > -- > 2.33.1 > >
On Tuesday, November 9th, 2021 at 10:24, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 09, 2021 at 08:56:10AM +0000, Simon Ser wrote: > > There are a few details specific to the GETFB2 IOCTL. > > > > It's not immediately clear how user-space should check for the > > number of planes. Suggest using the pitches field. > > > > The modifier array is filled with zeroes, ie. DRM_FORMAT_MOD_LINEAR. > > So explicitly tell user-space to not look at it unless the flag is > > set. > > > > Signed-off-by: Simon Ser <contact@emersion.fr> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Pekka Paalanen <ppaalanen@gmail.com> > > --- > > include/uapi/drm/drm.h | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 3b810b53ba8b..9809078b0f51 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -1096,6 +1096,22 @@ extern "C" { > > #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct drm_syncobj_transfer) > > #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) > > > > +/** > > + * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata. > > + * > > + * This queries metadata about a framebuffer. User-space fills > > + * &drm_mode_fb_cmd2.fb_id as the input, and the kernels fills the rest of the > > + * struct as the output. > > + * > > + * If the client is not DRM master and doesn't have &CAP_SYS_ADMIN, > > + * &drm_mode_fb_cmd2.handles will be zeroed. Planes are valid until one has a > > + * zero &drm_mode_fb_cmd2.pitches -- this can be used to compute the number of > > + * planes. > > Maybe explain that when actually importing the buffer userspace should > look for non-zeor handles instead, since some drivers/formats leave a > silly pitch value behind. Or at least I think that can happen. Just for > additional robustness? The IOCTL zeroes out the arrays at the start, so should be fine. handles will all be zero if user-space is unprivileged, so can't use them to figure out the number of planes. > > + * > > + * If the framebuffer has a format modifier, &DRM_MODE_FB_MODIFIERS will be set > > + * in &drm_mode_fb_cmd2.flags. Otherwise, &drm_mode_fb_cmd2.modifier has > > + * undefined contents. > > Uh is this true? We should always clear values to avoid accidental leaks > and stuff. See the commit message: the modifier array is zeroed, which means it's filled with DRM_FORMAT_MOD_LINEAR instead of DRM_FORMAT_MOD_INVALID -- this as good as undefined contents for me. Since this is a pretty good footgun, maybe we should change this? But the docs would still need to tell userspace to not look at the array because older kernels will still exist. > > + */ > > I think kerneldoc for drm_mode_fb_cmd2 would be neat too, so we can > document how pitch is supposed to work and all that stuff. Agreed, maybe will take a stab at this once this one is merged.
On Tue, Nov 09, 2021 at 09:29:54AM +0000, Simon Ser wrote: > On Tuesday, November 9th, 2021 at 10:24, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Tue, Nov 09, 2021 at 08:56:10AM +0000, Simon Ser wrote: > > > There are a few details specific to the GETFB2 IOCTL. > > > > > > It's not immediately clear how user-space should check for the > > > number of planes. Suggest using the pitches field. > > > > > > The modifier array is filled with zeroes, ie. DRM_FORMAT_MOD_LINEAR. > > > So explicitly tell user-space to not look at it unless the flag is > > > set. > > > > > > Signed-off-by: Simon Ser <contact@emersion.fr> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: Pekka Paalanen <ppaalanen@gmail.com> > > > --- > > > include/uapi/drm/drm.h | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > > index 3b810b53ba8b..9809078b0f51 100644 > > > --- a/include/uapi/drm/drm.h > > > +++ b/include/uapi/drm/drm.h > > > @@ -1096,6 +1096,22 @@ extern "C" { > > > #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct drm_syncobj_transfer) > > > #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) > > > > > > +/** > > > + * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata. > > > + * > > > + * This queries metadata about a framebuffer. User-space fills > > > + * &drm_mode_fb_cmd2.fb_id as the input, and the kernels fills the rest of the > > > + * struct as the output. > > > + * > > > + * If the client is not DRM master and doesn't have &CAP_SYS_ADMIN, > > > + * &drm_mode_fb_cmd2.handles will be zeroed. Planes are valid until one has a > > > + * zero &drm_mode_fb_cmd2.pitches -- this can be used to compute the number of > > > + * planes. > > > > Maybe explain that when actually importing the buffer userspace should > > look for non-zeor handles instead, since some drivers/formats leave a > > silly pitch value behind. Or at least I think that can happen. Just for > > additional robustness? > > The IOCTL zeroes out the arrays at the start, so should be fine. > > handles will all be zero if user-space is unprivileged, so can't use them to > figure out the number of planes. Yeah maybe just mention that unused/undefined fields are zeroed or something like that. > > > + * > > > + * If the framebuffer has a format modifier, &DRM_MODE_FB_MODIFIERS will be set > > > + * in &drm_mode_fb_cmd2.flags. Otherwise, &drm_mode_fb_cmd2.modifier has > > > + * undefined contents. > > > > Uh is this true? We should always clear values to avoid accidental leaks > > and stuff. > > See the commit message: the modifier array is zeroed, which means it's filled > with DRM_FORMAT_MOD_LINEAR instead of DRM_FORMAT_MOD_INVALID -- this as good as > undefined contents for me. > > Since this is a pretty good footgun, maybe we should change this? But the docs > would still need to tell userspace to not look at the array because older > kernels will still exist. Hm I think that's maybe better to document in the struct, since the entire "no modifier means implied modifier with driver-specific rules" thing applies to both addfb and getfb. Maybe we should also merge Daniel's "how to modifier" so that we could link at in-depth discussion of these topics in all the places where that's needed. > > > > + */ > > > > I think kerneldoc for drm_mode_fb_cmd2 would be neat too, so we can > > document how pitch is supposed to work and all that stuff. > > Agreed, maybe will take a stab at this once this one is merged. With a line about "undefined/unused stuff is cleared to zero" added this is: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
On Tue, 9 Nov 2021 10:24:33 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Nov 09, 2021 at 08:56:10AM +0000, Simon Ser wrote: > > There are a few details specific to the GETFB2 IOCTL. > > > > It's not immediately clear how user-space should check for the > > number of planes. Suggest using the pitches field. > > > > The modifier array is filled with zeroes, ie. DRM_FORMAT_MOD_LINEAR. > > So explicitly tell user-space to not look at it unless the flag is > > set. > > > > Signed-off-by: Simon Ser <contact@emersion.fr> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Pekka Paalanen <ppaalanen@gmail.com> > > --- > > include/uapi/drm/drm.h | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) Hi Simon, Regardless of your debate with danvet, this sounds fine to me, so Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> > > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 3b810b53ba8b..9809078b0f51 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -1096,6 +1096,22 @@ extern "C" { > > #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct drm_syncobj_transfer) > > #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) > > > > +/** > > + * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata. > > + * > > + * This queries metadata about a framebuffer. User-space fills > > + * &drm_mode_fb_cmd2.fb_id as the input, and the kernels fills the rest of the > > + * struct as the output. > > + * > > + * If the client is not DRM master and doesn't have &CAP_SYS_ADMIN, > > + * &drm_mode_fb_cmd2.handles will be zeroed. Planes are valid until one has a > > + * zero &drm_mode_fb_cmd2.pitches -- this can be used to compute the number of > > + * planes. > > Maybe explain that when actually importing the buffer userspace should > look for non-zeor handles instead, since some drivers/formats leave a > silly pitch value behind. Or at least I think that can happen. Just for > additional robustness? Sounds like pitch is literally undefined sometimes as well. What else would one call "silly" than undefined. Maybe implementation defined, but that's not better. > > > + * > > + * If the framebuffer has a format modifier, &DRM_MODE_FB_MODIFIERS will be set > > + * in &drm_mode_fb_cmd2.flags. Otherwise, &drm_mode_fb_cmd2.modifier has > > + * undefined contents. > > Uh is this true? We should always clear values to avoid accidental leaks > and stuff. This is userspace facing documentation, so saying "is undefined" is perfectly fine. It just means "expect garbage here, so don't even look", not that the kernel must literally leave that memory uninitialized. Thanks, pq > > > + */ > > I think kerneldoc for drm_mode_fb_cmd2 would be neat too, so we can > document how pitch is supposed to work and all that stuff. > > Anyway lgtm otherwise. > -Daniel > > > #define DRM_IOCTL_MODE_GETFB2 DRM_IOWR(0xCE, struct drm_mode_fb_cmd2) > > > > /* > > -- > > 2.33.1 > > > > >
On Tue, 9 Nov 2021 at 08:56, Simon Ser <contact@emersion.fr> wrote: > There are a few details specific to the GETFB2 IOCTL. > > It's not immediately clear how user-space should check for the > number of planes. Suggest using the pitches field. In fairness it is perfectly clear, it's just that I never considered calling it without master/admin because why would you ever do that? Anyway, the docs look correct and the more the better for sure, so this is: Acked-by: Daniel Stone <daniels@collabora.com>
On Thursday, November 11th, 2021 at 12:50, Daniel Stone <daniel@fooishbar.org> wrote: > In fairness it is perfectly clear, it's just that I never considered > calling it without master/admin because why would you ever do that? FWIW, drm_info does it to display the current buffers metadata. Pretty useful when debugging.
On Mon, Nov 15, 2021 at 12:09:47PM +0000, Simon Ser wrote: > On Thursday, November 11th, 2021 at 12:50, Daniel Stone <daniel@fooishbar.org> wrote: > > > In fairness it is perfectly clear, it's just that I never considered > > calling it without master/admin because why would you ever do that? > > FWIW, drm_info does it to display the current buffers metadata. Pretty > useful when debugging. We also have the state debugfs file for this, which should dump everything we can decode. If it doesn't we should extend it there ideally, instead of hand-rolling some tool that users/devs then don't have handy. debugfs tends to be there (at least for developers). -Daniel
On Monday, November 15th, 2021 at 15:20, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Nov 15, 2021 at 12:09:47PM +0000, Simon Ser wrote: > > On Thursday, November 11th, 2021 at 12:50, Daniel Stone <daniel@fooishbar.org> wrote: > > > > > In fairness it is perfectly clear, it's just that I never considered > > > calling it without master/admin because why would you ever do that? > > > > FWIW, drm_info does it to display the current buffers metadata. Pretty > > useful when debugging. > > We also have the state debugfs file for this, which should dump everything > we can decode. If it doesn't we should extend it there ideally, instead of > hand-rolling some tool that users/devs then don't have handy. debugfs > tends to be there (at least for developers). The debugfs output isn't as nice as drm_info, and can't produce structured output like JSON.
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3b810b53ba8b..9809078b0f51 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -1096,6 +1096,22 @@ extern "C" { #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct drm_syncobj_transfer) #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) +/** + * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata. + * + * This queries metadata about a framebuffer. User-space fills + * &drm_mode_fb_cmd2.fb_id as the input, and the kernels fills the rest of the + * struct as the output. + * + * If the client is not DRM master and doesn't have &CAP_SYS_ADMIN, + * &drm_mode_fb_cmd2.handles will be zeroed. Planes are valid until one has a + * zero &drm_mode_fb_cmd2.pitches -- this can be used to compute the number of + * planes. + * + * If the framebuffer has a format modifier, &DRM_MODE_FB_MODIFIERS will be set + * in &drm_mode_fb_cmd2.flags. Otherwise, &drm_mode_fb_cmd2.modifier has + * undefined contents. + */ #define DRM_IOCTL_MODE_GETFB2 DRM_IOWR(0xCE, struct drm_mode_fb_cmd2) /*
There are a few details specific to the GETFB2 IOCTL. It's not immediately clear how user-space should check for the number of planes. Suggest using the pitches field. The modifier array is filled with zeroes, ie. DRM_FORMAT_MOD_LINEAR. So explicitly tell user-space to not look at it unless the flag is set. Signed-off-by: Simon Ser <contact@emersion.fr> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Pekka Paalanen <ppaalanen@gmail.com> --- include/uapi/drm/drm.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)