Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] iwlwifi: mvm: no need to check return value of
 debugfs_create functions
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org,
        Johannes Berg <johannes.berg@intel.com>,
        Emmanuel Grumbach <emmanuel.grumbach@intel.com>,
        Luca Coelho <luciano.coelho@intel.com>,
        Intel Linux Wireless <linuxwifi@intel.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless@vger.kernel.org
References: <20190122152151.16139-24-gregkh@linuxfoundation.org>
 <03bb68be-8e42-a463-2d57-3be051dc2016@redhat.com>
 <20190327012600.GA3649@kroah.com>
From:   Laura Abbott <labbott@redhat.com>
Message-ID: <5660d50d-6cbf-3b36-6e6c-312b1617ed23@redhat.com>
Date:   Tue, 26 Mar 2019 18:47:33 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.0
MIME-Version: 1.0
In-Reply-To: <20190327012600.GA3649@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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);

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.

Thanks,
Laura

> strange,
> 
> greg k-h
>