Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Daniel Vetter <daniel@ffwll.ch>]

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>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch