netdev - Re: [PATCH v2] drivers: ethernet: cpsw: fix panic when intrrupt coaleceing is set via ethtool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e0a1e509-f488-0472-2574-b55f1e393ad1@wago.com>
Date:   Wed, 23 Mar 2022 15:03:50 +0000
From:   Sondhauß, Jan <Jan.Sondhauss@...o.com>
To:     Vignesh Raghavendra <vigneshr@...com>,
        "grygorii.strashko@...com" <grygorii.strashko@...com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>
Subject: Re: [PATCH v2] drivers: ethernet: cpsw: fix panic when intrrupt
 coaleceing is set via ethtool



On 23/03/2022 12:10, Vignesh Raghavendra wrote:
> On 23/03/22 2:17 pm, Sondhauß, Jan wrote: > cpsw_ethtool_begin directly 
> returns the result of pm_runtime_get_sync > when successful. > 
> pm_runtime_get_sync returns -error code on failure and 0 on successful > 
> resume but also 1 when ZjQcmQRYFpfptBannerStart
> This Message Is From an External Sender
> Please use caution when clicking on links or opening attachments!
> ZjQcmQRYFpfptBannerEnd
> 
> On 23/03/22 2:17 pm, Sondhauß, Jan wrote:
>> cpsw_ethtool_begin directly returns the result of pm_runtime_get_sync
>> when successful.
>> pm_runtime_get_sync returns -error code on failure and 0 on successful
>> resume but also 1 when the device is already active. So the common case
>> for cpsw_ethtool_begin is to return 1. That leads to inconsistent calls
>> to pm_runtime_put in the call-chain so that pm_runtime_put is called
>> one too many times and as result leaving the cpsw dev behind suspended.
>> 
>> The suspended cpsw dev leads to an access violation later on by
>> different parts of the cpsw driver.
>> 
>> Fix this by calling the return-friendly pm_runtime_resume_and_get
>> function.
>> 
>> Signed-off-by: Jan Sondhauss <jan.sondhauss@...o.com>
>> ---
>>  drivers/net/ethernet/ti/cpsw_ethtool.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/ti/cpsw_ethtool.c b/drivers/net/ethernet/ti/cpsw_ethtool.c
>> index 158c8d3793f4..b5bae6324970 100644
>> --- a/drivers/net/ethernet/ti/cpsw_ethtool.c
>> +++ b/drivers/net/ethernet/ti/cpsw_ethtool.c
>> @@ -364,11 +364,9 @@ int cpsw_ethtool_op_begin(struct net_device *ndev)
>>  	struct cpsw_common *cpsw = priv->cpsw;
>>  	int ret;
>>  
>> -	ret = pm_runtime_get_sync(cpsw->dev);
>> -	if (ret < 0) {
>> +	ret = pm_runtime_resume_and_get(cpsw->dev);
>> +	if (ret < 0)
>>  		cpsw_err(priv, drv, "ethtool begin failed %d\n", ret);
>> -		pm_runtime_put_noidle(cpsw->dev);
>> -	}
>>  
>>  	return ret;
>>  }
> 
> Thanks for the fix!
> 
> Reviewed-by: Vignesh Raghavendra <vigneshr@...com>
> 
> I am guessing this issue started with d43c65b05b84 (ethtool:
> runtime-resume netdev parent in ethnl_ops_begin) ? If so, could you add
> a Fixes tag so that patch gets backported to all affected stable kernels?

Yes it started with that commit.

> 
> Regards
> Vignesh
>