Re: [Intel-gfx] [PATCH] drm/i915: do not return invalid pointers as a *dentry

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

On Thu, Jan 31, 2019 at 07:17:02PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 31, 2019 at 09:59:26AM -0800, Rodrigo Vivi wrote:
> > On Thu, Jan 31, 2019 at 02:15:07PM +0100, Greg Kroah-Hartman wrote:
> > > When calling debugfs functions, they can now return error values if
> > > something went wrong.  If that happens, return a NULL as a *dentry to
> > > the relay core instead of passing it an illegal pointer.
> > > 
> > > The relay core should be able to handle an illegal pointer, but add this
> > > check to be safe.
> > > 
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_guc_log.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> > > index d3ebdbc0182e..8bf03497dcd8 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc_log.c
> > > +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> > > @@ -140,6 +140,9 @@ static struct dentry *create_buf_file_callback(const char *filename,
> > >  
> > >  	buf_file = debugfs_create_file(filename, mode,
> > >  				       parent, buf, &relay_file_operations);
> > > +	if (IS_ERR(buf_file))
> > > +		return NULL;
> > 
> > I still see a return NULL inside debugfs_create_file on master,
> > but probably you are ahead with some change that I didn't see yet right?
> 
> Yes, this patch is in linux-next now and should go to Linus for
> 5.0-final:
> 	https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-linus&id=ff9fb72bc07705c00795ca48631f7fffe24d2c6b
> 
> > I'm just wondering if it wouldn't be better for now to go with
> > 
> > if (IS_ERR_OR_NULL(buf_file))
> 
> Not really, because the next line is:
> 
> > >  	return buf_file;
> 
> So it's the same thing :)
> 
> > apparently we also need it on i915_debugfs.c i915_debugfs_register()
> 
> I have a bunch of patches I'm working on to go through and fix up all of
> this (you shouldn't be checking the return value at all, unless you
> really want to use it as a "real" dentry and not just something that
> debugfs uses internally.
> 
> But for now, you should be fine, that call will only fail if you are out
> of memory, or did something really wrong.

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

do you wanna push this with your own stuff or do you want me to pick
up on drm-intel-next?

> 
> thanks,
> 
> greg k-h
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel