Re: [Intel-xe] [PATCH v2 3/4] fixup! drm/xe: Introduce a new DRM driver for Intel GPUs

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Aug 15, 2023 at 11:40:31AM -0700, Umesh Nerlige Ramappa wrote:
> Hi Matt, Rodrigo,
> 
> I have a uAPI change here that breaks UMDs. How do I land this and what UMDs
> should I inform for updating xe_drm.h?

Hi Umesh, if you don't mind, I'm going to take your patches here and put
with the series that me and Francois are putting together so we change
the uapi only once.

It is critical for our series, since Francois' removal of mmio depend
on the addition of this one here, so Mesa can stop using the mmio_ioctl.

Also, I'm going to rename the patches. They should not be fixups at
this time.

> 
> Thanks,
> Umesh
> 
> On Tue, Aug 15, 2023 at 07:03:42AM -0700, Souza, Jose wrote:
> > On Mon, 2023-08-14 at 15:37 -0700, Umesh Nerlige Ramappa wrote:
> > > struct drm_xe_engine_class_instance might get padded for 64-bit
> > > alignment based on compiler used. Since engine information is kmalloced
> > > in the query and drm_xe_engine_class_instance may be padded, it could
> > > potentially leak some kernel memory to user.
> > > 
> > > Add a rsvd field to struct drm_xe_engine_class_instance to make it
> > > 64-bit aligned and zero out the field before returning to user.
> > > 
> > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_query.c | 7 +++++--
> > >  include/uapi/drm/xe_drm.h     | 1 +
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > > index 99a4800c7c53..b9d565264ceb 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -65,7 +65,7 @@ static int query_engines(struct xe_device *xe,
> > >  		return -EINVAL;
> > >  	}
> > > 
> > > -	hw_engine_info = kmalloc(size, GFP_KERNEL);
> > > +	hw_engine_info = kzalloc(size, GFP_KERNEL);
> > >  	if (!hw_engine_info)
> > >  		return -ENOMEM;
> > > 
> > > @@ -78,7 +78,10 @@ static int query_engines(struct xe_device *xe,
> > >  				xe_to_user_engine_class[hwe->class];
> > >  			hw_engine_info[i].engine_instance =
> > >  				hwe->logical_instance;
> > > -			hw_engine_info[i++].gt_id = gt->info.id;
> > > +			hw_engine_info[i].gt_id = gt->info.id;
> > > +			hw_engine_info[i].rsvd = 0;
> > > +
> > > +			i++;
> > >  		}
> > > 
> > >  	if (copy_to_user(query_ptr, hw_engine_info, size)) {
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index 86f16d50e9cc..53cd57342620 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -753,6 +753,7 @@ struct drm_xe_engine_class_instance {
> > > 
> > >  	__u16 engine_instance;
> > >  	__u16 gt_id;
> > > +	__u16 rsvd;
> > 
> > I don't think this change should be fixup, as this breaks the uAPI.
> > Please ping the Rodrigo, Matt Brost... about what we should do here.
> > But at least you need to sync with other UMD contacts about landing this change at the same time UMDs update their xe_drm.h.
> > 
> > >  };
> > > 
> > >  struct drm_xe_exec_queue_create {
> >