Re: [PATCH 1/2] drm/panthor: Fix missing explicit padding in drm_panthor_gpu_info

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 17 Apr 2025 11:24:32 +0100
Steven Price <steven.price@arm.com> wrote:

> On 17/04/2025 11:05, Boris Brezillon wrote:
> > drm_panthor_gpu_info::shader_present is currently automatically offset
> > by 4 byte to meet Arm's 32-bit/64-bit field alignment rules, but those
> > constraints don't stand on 32-bit x86 and cause a mismatch when running
> > an x86 binary in a user emulated environment like FEX. It's also
> > generally agreed that uAPIs should explicitly pad their struct fields,
> > which we originally intended to do, but a mistake slipped through during
> > the submission process, leading drm_panthor_gpu_info::shader_present to
> > be misaligned.
> > 
> > This uAPI change doesn't break any of the existing users of panthor
> > which are either arm32 or arm64 where the 64-bit alignment of
> > u64 fields is already enforced a the compiler level.
> > 
> > Fixes: 0f25e493a246 ("drm/panthor: Add uAPI")
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  include/uapi/drm/panthor_drm.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > index 97e2c4510e69..1379a2d4548c 100644
> > --- a/include/uapi/drm/panthor_drm.h
> > +++ b/include/uapi/drm/panthor_drm.h
> > @@ -293,6 +293,18 @@ struct drm_panthor_gpu_info {
> >  	/** @as_present: Bitmask encoding the number of address-space exposed by the MMU. */
> >  	__u32 as_present;
> >  
> > +	/**
> > +	 * @garbage: Unused field that's not even zero-checked.
> > +	 *
> > +	 * This originates from a missing padding that leaked in the initial driver submission
> > +	 * and was only found when testing the driver in a 32-bit x86 environment, where
> > +	 * u64 field alignment rules are relaxed compared to aarch32.
> > +	 *
> > +	 * This field can't be repurposed, because it's never been checked by the driver and
> > +	 * userspace is not guaranteed to zero it out.  
> 
> Why would user space be providing this structure? This is meant to be
> provided from the kernel to user space, and (fingers-crossed) we've been
> zeroing the padding even though not explicitly? (rather than leaking
> some kernel data).

Uh, you're right this doesn't matter for kernel -> user data transfers.
I guess we can just make it a regular padding field, and allow it to be
repurposed if needed (as long as the expected default value is zero, of
course).

> 
> Other than the comment - yes this is a uAPI mistake we should fix.
> 
> I'm not sure how much we care about historic x86 uAPI but it also should
> be possible to identify an old x86 client using the x86 padding because
> the structure will be too short. But my preference would be to say "it's
> always been broken on x86" and therefore there's no regression.

That's also my opinion: we don't have native x86 users of panthor, and
emulated ones are just broken right now, because the kernel side (which
is arm32/64) already has a different layout.

> 
> Thanks,
> Steve
> 
> > +	 */
> > +	__u32 garbage;
> > +
> >  	/** @shader_present: Bitmask encoding the shader cores exposed by the GPU. */
> >  	__u64 shader_present;
> >    
>