Re: [PATCH] gpu/drm/xe/xe_devcoredump: Check NULL before dereferencing coredump.

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

On Wed, Mar 20, 2024 at 10:54:57AM +0530, Ghimiray, Himal Prasad wrote:
> 
> On 20-03-2024 10:39, Dixit, Ashutosh wrote:
> > On Tue, 19 Mar 2024 22:17:19 -0700, Himal Prasad Ghimiray wrote:
> > > Derefernce coredump to get xe_devcoredump_snapshot only if coredump is
> > > not NULL.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_devcoredump.c | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> > > index 0fcd30680323..83a1f3cd37cf 100644
> > > --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> > > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> > > @@ -74,17 +74,20 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset,
> > > 				   size_t count, void *data, size_t datalen)
> > >   {
> > > 	struct xe_devcoredump *coredump = data;
> > > +	if (!coredump)
> > Needs empty line before this line.
> sure.
> > 
> > > +		return -ENODATA;
> > > +
> > > 	struct xe_device *xe = coredump_to_xe(coredump);
> > Declarations after code statements are not premitted in standard C. Does
> > this compile?
> I was also assuming it will fail but it does pass compilation.
> > 
> > If you fix these things looks like we're back to the original code?
> Sorry I didn't understand this. Can you elaborate on this ?

you need something like this
{
	struct xe_devcoredump *coredump = data;
	struct xe_device *xe;

	if (!coredump)
	   return -ENODATA;

	xe = coredump_to_xe(coredump);
	if (!xe)
	   return;

and respecting all the blank lines above..

let's kill the comment as well... that's obvious...

with that,

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > 
> > 
> > > +	/* Our device is gone already... */
> > > +	if (!xe)
> > > +		return -ENODEV;
> > > +
> > > 	struct xe_devcoredump_snapshot *ss = &coredump->snapshot;
> > > 	struct drm_printer p;
> > > 	struct drm_print_iterator iter;
> > > 	struct timespec64 ts;
> > > 	int i;
> > > 
> > > -	/* Our device is gone already... */
> > > -	if (!data || !coredump_to_xe(coredump))
> > > -		return -ENODEV;
> > > -
> > > 	/* Ensure delayed work is captured before continuing */
> > > 	flush_work(&ss->work);
> > > 
> > > --
> > > 2.25.1
> > >