Re: [PATCH] iwlwifi: mvm: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Wed, Mar 27, 2019 at 08:35:37AM +0200, Luca Coelho wrote:
> On Wed, 2019-03-27 at 10:53 +0900, Greg Kroah-Hartman wrote:
> > On Tue, Mar 26, 2019 at 06:47:33PM -0700, Laura Abbott wrote:
> > > On 3/26/19 6:26 PM, Greg Kroah-Hartman wrote:
> > > > On Tue, Mar 26, 2019 at 04:55:54PM -0700, Laura Abbott wrote:
> > > > > On 1/22/19 7:21 AM, Greg Kroah-Hartman wrote:
> > > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
> > > > > > index 33b0af24a537..c52cdc538678 100644
> > > > > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
> > > > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
> > > > > > @@ -1446,9 +1446,8 @@ static ssize_t iwl_dbgfs_quota_min_read(struct file *file,
> > > > > >    #define MVM_DEBUGFS_READ_WRITE_FILE_OPS(name, bufsz) \
> > > > > >    	_MVM_DEBUGFS_READ_WRITE_FILE_OPS(name, bufsz, struct ieee80211_vif)
> > > > > >    #define MVM_DEBUGFS_ADD_FILE_VIF(name, parent, mode) do {		\
> > > > > > -		if (!debugfs_create_file(#name, mode, parent, vif,	\
> > > > > > -					 &iwl_dbgfs_##name##_ops))	\
> > > > > > -			goto err;					\
> > > > > > +		debugfs_create_file(#name, mode, parent, vif,		\
> > > > > > +				    &iwl_dbgfs_##name##_ops);		\
> > > > > >    	} while (0)
> > > > > >    MVM_DEBUGFS_READ_FILE_OPS(mac_params);
> > > > > > @@ -1483,12 +1482,6 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
> > > > > >    	mvmvif->dbgfs_dir = debugfs_create_dir("iwlmvm", dbgfs_dir);
> > > > > > -	if (!mvmvif->dbgfs_dir) {
> > > > > > -		IWL_ERR(mvm, "Failed to create debugfs directory under %pd\n",
> > > > > > -			dbgfs_dir);
> > > > > > -		return;
> > > > > > -	}
> > > > > > -
> > > > > >    	if (iwlmvm_mod_params.power_scheme != IWL_POWER_SCHEME_CAM &&
> > > > > >    	    ((vif->type == NL80211_IFTYPE_STATION && !vif->p2p) ||
> > > > > >    	     (vif->type == NL80211_IFTYPE_STATION && vif->p2p)))
> > > > > > @@ -1537,12 +1530,6 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
> > > > > >    	mvmvif->dbgfs_slink = debugfs_create_symlink(dbgfs_dir->d_name.name,
> > > > > >    						     mvm->debugfs_dir, buf);
> > > > > > -	if (!mvmvif->dbgfs_slink)
> > > > > > -		IWL_ERR(mvm, "Can't create debugfs symbolic link under %pd\n",
> > > > > > -			dbgfs_dir);
> > > > > > -	return;
> > > > > > -err:
> > > > > > -	IWL_ERR(mvm, "Can't create debugfs entity\n");
> > > > > >    }
> > > > > 
> > > > > Fedora got a bug report https://bugzilla.redhat.com/show_bug.cgi?id=1691034
> > > > > of a crash with 5.0 and the user did a bisect which pointed to ff9fb72bc077
> > > > > ("debugfs: return error values, not NULL") because the error checking is
> > > > > no longer correct in this driver.
> > > > > 
> > > > > Based on https://patchwork.kernel.org/patch/10865839/, it looks like
> > > > > this is supposed to go in for 5.2 but this needs to go in now as
> > > > > the error checking is currently broken without it. Can this get queued
> > > > > for Linus so we can get it in 5.0 stable?
> > > > 
> > > > That's odd, I can't see how the error checking is wrong here.  If the
> > > > directory is not created, an error will be returned, which should be
> > > > able to be handled by debugfs_create_file().
> > > > 
> > > > So with this patch does the error go away?
> > > > 
> > > 
> > > The full patch didn't apply cleanly and I didn't try to backport it
> > > for the reporter to test. I was going off of the theory that if the
> > > patch was there it would fix the problem.
> > > 
> > > What I _think_ is going wrong is dbgfs_dir is actually an errno value:
> > > 
> > > 
> > >         struct dentry *dbgfs_dir = vif->debugfs_dir;
> > >         struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
> > >         char buf[100];
> > > 
> > >         /*
> > >          * Check if debugfs directory already exist before creating it.
> > >          * This may happen when, for example, resetting hw or suspend-resume
> > >          */
> > >         if (!dbgfs_dir || mvmvif->dbgfs_dir)
> > >                 return;
> > > 
> > > 
> > > so this blows up in the snprintf
> > > 
> > >         snprintf(buf, 100, "../../../%pd3/%pd",
> > >                  dbgfs_dir,
> > >                  mvmvif->dbgfs_dir);
> > 
> > Ah, yeah, that's horrible.  They had the name before, why pull it out of
> > the dentry again?  That will blow up hard, but maybe printk should check
> > to see if the pointer really is a pointer first.
> 
> I agree this is ugly.  But do you mean we could use
> ("../../../%pd3/%s", dbgfs_dir, "iwlmvm")? Or how did we have the name?

You "had" the name because you created this directory and name
previously :)

> Also, this would solve the sprintf() problem, but still wouldn't solve
> the real issue, which is not check for ERR in dbgfs_dir.

True.

> > > Unless I misunderstood what the debugfs error change did. I think this
> > > also means the if check needs to look for IS_ERR and not just !dbgfs_dir.
> > 
> > Yes, that is correct.
> 
> Yeah, we can do that.  So this patch doesn't need to be sent for
> v5.1-rc* and v5.0, right? At least I don't see how it would fix the
> issue.  What we need is a new patch with the IS_ERR check.

Ok, let me go create that right now...

thanks,

greg k-h