netdev - Re: [PATCH] drivers: ethernet: cpsw: fix panic when interrupt 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-next>] [day] [month] [year] [list]

Date:   Tue, 22 Mar 2022 16:04:25 +0530
From:   Vignesh Raghavendra <vigneshr@...com>
To:     Sondhauß, Jan <Jan.Sondhauss@...o.com>,
        "grygorii.strashko@...com" <grygorii.strashko@...com>
CC:     "linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
        "David S . Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH] drivers: ethernet: cpsw: fix panic when interrupt
 coaleceing is set via ethtool

Hi,

Adding netdev list and maintainers

Please cc netdev ML and net maintainers

./scripts/get_maintainer.pl -f drivers/net/ethernet/ti/cpsw_ethtool.c

On 22/03/22 12:02 pm, Sondhauß, Jan wrote:
> cpsw_ethtool uses the power management in the begin and complete
> functions of the ethtool_ops. The result of pm_runtime_get_sync was
> returned unconditionally, which results in problems since the ethtool-
> interface relies on 0 for success and negativ values for errors.
> d43c65b05b84 (ethtool: runtime-resume netdev parent in ethnl_ops_begin)
> introduced power management to the netlink implementation for the
> ethtool interface and does not explicitly check for negative return
> values.
> 
> As a result the pm_runtime_suspend function is called one-too-many
> times in ethnl_ops_begin and that leads to an access violation when
> the cpsw hardware is accessed after using
> 'ethtool -C eth-of-cpsw rx-usecs 1234'. To fix this the call to
> pm_runtime_get_sync in cpsw_ethtool_op_begin is replaced with a call
> to pm_runtime_resume_and_get as it provides a returnable error-code.
> 

pm_runtime_resume_and_get() is just wrapper around pm_runtime_get_sync()
+ error handling (as done in the below code) and both return 0 on
success and -ve error code on failure


> Signed-off-by: Jan Sondhauss <jan.sondhauss@...o.com>
> ---
>  drivers/net/ethernet/ti/cpsw_ethtool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw_ethtool.c b/drivers/net/ethernet/ti/cpsw_ethtool.c
> index 158c8d3793f4..5eda20039cc1 100644
> --- a/drivers/net/ethernet/ti/cpsw_ethtool.c
> +++ b/drivers/net/ethernet/ti/cpsw_ethtool.c
> @@ -364,7 +364,7 @@ int cpsw_ethtool_op_begin(struct net_device *ndev)
>  	struct cpsw_common *cpsw = priv->cpsw;
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(cpsw->dev);
> +	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);


In fact code now ends up calling pm_runtime_put_noidle() twice in case
of failure, once inside pm_runtime_resume_and_get() and again here?

So something looks fishy?

Regards
Vignesh