[PATCH] drm: pre-fill getfb2 modifier array with INVALID

[PATCH] drm: pre-fill getfb2 modifier array with INVALID

[Simon Ser]

User-space shouldn't look up the modifier array when the modifier
flag is missing, but at the moment no docs make this clear (working
on it). Right now the modifier array is pre-filled with zeroes, aka.
LINEAR. Instead, pre-fill with INVALID to avoid footguns.

This is a uAPI change, but OTOH any user-space which looks up the
modifier array without checking the flag is broken already, so
should be fine.

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/drm_framebuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 07f5abc875e9..f7041c0a0407 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -601,7 +601,7 @@ int drm_mode_getfb2_ioctl(struct drm_device *dev,
 		r->handles[i] = 0;
 		r->pitches[i] = 0;
 		r->offsets[i] = 0;
-		r->modifier[i] = 0;
+		r->modifier[i] = DRM_FORMAT_MOD_INVALID;
 	}
 
 	for (i = 0; i < fb->format->num_planes; i++) {
-- 
2.33.1

Re: [PATCH] drm: pre-fill getfb2 modifier array with INVALID

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Thu, 11 Nov 2021 10:10:54 +0000
Simon Ser <contact@emersion.fr> wrote:

> User-space shouldn't look up the modifier array when the modifier
> flag is missing, but at the moment no docs make this clear (working
> on it). Right now the modifier array is pre-filled with zeroes, aka.
> LINEAR. Instead, pre-fill with INVALID to avoid footguns.
> 
> This is a uAPI change, but OTOH any user-space which looks up the
> modifier array without checking the flag is broken already, so
> should be fine.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 07f5abc875e9..f7041c0a0407 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -601,7 +601,7 @@ int drm_mode_getfb2_ioctl(struct drm_device *dev,
>  		r->handles[i] = 0;
>  		r->pitches[i] = 0;
>  		r->offsets[i] = 0;
> -		r->modifier[i] = 0;
> +		r->modifier[i] = DRM_FORMAT_MOD_INVALID;
>  	}
>  
>  	for (i = 0; i < fb->format->num_planes; i++) {

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: pre-fill getfb2 modifier array with INVALID

[Daniel Stone]

On Thu, 11 Nov 2021 at 10:11, Simon Ser <contact@emersion.fr> wrote:
> User-space shouldn't look up the modifier array when the modifier
> flag is missing, but at the moment no docs make this clear (working
> on it). Right now the modifier array is pre-filled with zeroes, aka.
> LINEAR. Instead, pre-fill with INVALID to avoid footguns.
>
> This is a uAPI change, but OTOH any user-space which looks up the
> modifier array without checking the flag is broken already, so
> should be fine.

I don't know of any userspace which this would break.

Acked-by: Daniel Stone <daniels@collabora.com>

Re: [PATCH] drm: pre-fill getfb2 modifier array with INVALID

[Ville Syrjälä]

On Thu, Nov 11, 2021 at 10:10:54AM +0000, Simon Ser wrote:
> User-space shouldn't look up the modifier array when the modifier
> flag is missing, but at the moment no docs make this clear (working
> on it). Right now the modifier array is pre-filled with zeroes, aka.
> LINEAR. Instead, pre-fill with INVALID to avoid footguns.
> 
> This is a uAPI change, but OTOH any user-space which looks up the
> modifier array without checking the flag is broken already, so
> should be fine.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Daniel Stone <daniels@collabora.com>

Isn't this going to break the test where we pass the get
getfb2 result back into addfb2?

> ---
>  drivers/gpu/drm/drm_framebuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 07f5abc875e9..f7041c0a0407 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -601,7 +601,7 @@ int drm_mode_getfb2_ioctl(struct drm_device *dev,
>  		r->handles[i] = 0;
>  		r->pitches[i] = 0;
>  		r->offsets[i] = 0;
> -		r->modifier[i] = 0;
> +		r->modifier[i] = DRM_FORMAT_MOD_INVALID;
>  	}
>  
>  	for (i = 0; i < fb->format->num_planes; i++) {
> -- 
> 2.33.1
> 

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: pre-fill getfb2 modifier array with INVALID

[Simon Ser]

On Thursday, November 11th, 2021 at 13:50, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Thu, Nov 11, 2021 at 10:10:54AM +0000, Simon Ser wrote:
> > User-space shouldn't look up the modifier array when the modifier
> > flag is missing, but at the moment no docs make this clear (working
> > on it). Right now the modifier array is pre-filled with zeroes, aka.
> > LINEAR. Instead, pre-fill with INVALID to avoid footguns.
> >
> > This is a uAPI change, but OTOH any user-space which looks up the
> > modifier array without checking the flag is broken already, so
> > should be fine.
> >
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > Cc: Daniel Stone <daniels@collabora.com>
>
> Isn't this going to break the test where we pass the get
> getfb2 result back into addfb2?

Shouldn't be the case, since the kernel will ignore modifiers if the
flag isn't set.

Re: [PATCH] drm: pre-fill getfb2 modifier array with INVALID

[Ville Syrjälä]

On Mon, Nov 15, 2021 at 09:18:42AM +0000, Simon Ser wrote:
> On Thursday, November 11th, 2021 at 13:50, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Thu, Nov 11, 2021 at 10:10:54AM +0000, Simon Ser wrote:
> > > User-space shouldn't look up the modifier array when the modifier
> > > flag is missing, but at the moment no docs make this clear (working
> > > on it). Right now the modifier array is pre-filled with zeroes, aka.
> > > LINEAR. Instead, pre-fill with INVALID to avoid footguns.
> > >
> > > This is a uAPI change, but OTOH any user-space which looks up the
> > > modifier array without checking the flag is broken already, so
> > > should be fine.
> > >
> > > Signed-off-by: Simon Ser <contact@emersion.fr>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > > Cc: Daniel Stone <daniels@collabora.com>
> >
> > Isn't this going to break the test where we pass the get
> > getfb2 result back into addfb2?
> 
> Shouldn't be the case, since the kernel will ignore modifiers if the
> flag isn't set.

if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
	DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
		r->modifier[i], i);
	return -EINVAL;
}

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: pre-fill getfb2 modifier array with INVALID

[Simon Ser]

Hm indeed, RIP. I got confused by this one:

		/* Pre-FB_MODIFIERS userspace didn't clear the structs properly. */
		if (!(r->flags & DRM_MODE_FB_MODIFIERS))
			continue;

But it's only run for unused planes.