[PATCH 2/6] drm: Introduce DRM_MODE_DUMB_KERNEL_MAP flag

[Rob Herring]

On Wed, Oct 23, 2019 at 9:28 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Rob,
>
> On Tue, Oct 22, 2019 at 07:42:06AM -0500, Rob Herring wrote:
> > On Tue, Oct 22, 2019 at 6:14 AM Laurent Pinchart wrote:
> > > On Mon, Oct 21, 2019 at 04:45:46PM -0500, Rob Herring wrote:
> > > > Introduce a new flag, DRM_MODE_DUMB_KERNEL_MAP, for struct
> > > > drm_mode_create_dumb. This flag is for internal kernel use to indicate
> > > > if dumb buffer allocation needs a kernel mapping. This is needed only for
> > > > CMA where creating a kernel mapping or not has to be decided at allocation
> > > > time because creating a mapping on demand (with vmap()) is not guaranteed
> > > > to work. Several drivers are using CMA, but not the CMA helpers because
> > > > they distinguish between kernel and userspace allocations to create a
> > > > kernel mapping or not.
> > > >
> > > > Update the callers of drm_mode_dumb_create() to set
> > > > drm_mode_dumb_create.flags to appropriate defaults. Currently, flags can
> > > > be set to anything by userspace, but is unused within the kernel. Let's
> > > > force flags to zero (no kernel mapping) for userspace callers by default.
> > > > For in kernel clients, set DRM_MODE_DUMB_KERNEL_MAP by default. Drivers
> > > > can override this as needed.
> > > >
> > > > Cc: David Airlie <airlied at linux.ie>
> > > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > Cc: Maxime Ripard <mripard at kernel.org>
> > > > Cc: Sean Paul <sean at poorly.run>
> > > > Signed-off-by: Rob Herring <robh at kernel.org>
> > > > ---
> > > >  drivers/gpu/drm/drm_client.c       | 1 +
> > > >  drivers/gpu/drm/drm_dumb_buffers.c | 5 ++++-
> > > >  include/uapi/drm/drm_mode.h        | 2 ++
> > > >  3 files changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> > > > index d9a2e3695525..dbfc8061b392 100644
> > > > --- a/drivers/gpu/drm/drm_client.c
> > > > +++ b/drivers/gpu/drm/drm_client.c
> > > > @@ -264,6 +264,7 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
> > > >       dumb_args.width = width;
> > > >       dumb_args.height = height;
> > > >       dumb_args.bpp = info->cpp[0] * 8;
> > > > +     dumb_args.flags = DRM_MODE_DUMB_KERNEL_MAP;
> > > >       ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
> > > >       if (ret)
> > > >               goto err_delete;
> > > > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> > > > index d18a740fe0f1..74a13f14c173 100644
> > > > --- a/drivers/gpu/drm/drm_dumb_buffers.c
> > > > +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> > > > @@ -97,7 +97,10 @@ int drm_mode_create_dumb(struct drm_device *dev,
> > > >  int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> > > >                              void *data, struct drm_file *file_priv)
> > > >  {
> > > > -     return drm_mode_create_dumb(dev, data, file_priv);
> > > > +     struct drm_mode_create_dumb *args = data;
> > > > +
> > > > +     args->flags = 0;
> > >
> > > I would prefer returning an error if flags is set to a non-zero value,
> > > to catch userspace errors early, but I'm also worried it would break
> > > existing userspace by uncovering existing bugs. Still, if we later add
> > > flags that userspace can set, those existing bugs will be triggered, so
> > > we'll have to deal with them anyway, and we could already give it a try.
> >
> > I would like that too, but the comment just above this code tells me
> > that's likely to break things:
> >
> >         /*
> >          * handle, pitch and size are output parameters. Zero them out to
> >          * prevent drivers from accidentally using uninitialized data. Since
> >          * not all existing userspace is clearing these fields properly we
> >          * cannot reject IOCTL with garbage in them.
> >          */
> >
> > Maybe userspace does correctly zero out input params.
>
> But if that holds true, it means that we will never be able to add
> userspace flags, as doing so could break applications for the same
> reason. The flag field should thus be marked as deprecated for userspace
> usage. I wonder how big the risk is.

Good point. I guess another option is add a WARN(flags != 0) and see
what happens.

The commit adding the comment was f60859522a83 ("drm: Sanitize
DRM_IOCTL_MODE_CREATE_DUMB input"). Maybe Thierry has some comment?

Rob