On Mon, 11 Oct 2021, Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Oct 11, 2021 at 04:19:58PM +0200, Christian König wrote:
And then throw it away, later, when you want to remove the directory, look it up with a call to debugfs_lookup() and pass that to debugfs_remove() (which does so recursively).
There should never be a need to save, or check, the result of any debugfs call. If so, odds are it is being used incorrectly.
Yeah, exactly that's the problem I see here.
We save the return value because the DRM subsystem is creating a debugfs directory for the drivers to use.
That's fine for now, not a big deal. And even if there is an error, again, you can always feed that error back into the debugfs subsystem on another call and it will handle it correctly.
Problem is it isn't, we have a crash because the member isn't a pointer but an ERR_PTR instead.
Again, that is fine, you can feed that into debugfs and it will "just work". Treat it as an opaque pointer, not a *dentry and you will be fine.
Hmm, some of the patches add things like:
+ + if (!root) + goto error; + minor->debugfs_root = debugfs_create_dir(name, root);
Superficially this seems okay, as it looks like debugfs_create_dir() doesn't actually cope with NULL values. However, since ->debugfs_root comes from debugfs_create_dir() I presume it'll never be NULL on errors anyway but rather an error pointer!
So I think we probably need to go through the drm subsystem and look for existing similar patterns in fix them.
BR, Jani.
thanks,
greg k-h