netdev - [bug report] net/ethtool: add netlink interface for the PLCA RS

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]

Message-ID: <Y8F+9UmSubFpBsYo@kili>
Date:   Fri, 13 Jan 2023 18:55:33 +0300
From:   Dan Carpenter <error27@...il.com>
To:     piergiorgio.beruto@...il.com
Cc:     netdev@...r.kernel.org
Subject: [bug report] net/ethtool: add netlink interface for the PLCA RS

Hello Piergiorgio Beruto,

The patch 8580e16c28f3: "net/ethtool: add netlink interface for the
PLCA RS" from Jan 9, 2023, leads to the following Smatch static
checker warning:

	net/ethtool/plca.c:155 ethnl_set_plca_cfg()
	info: return a literal instead of 'ret'

net/ethtool/plca.c
    140 int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info)
    141 {
    142         struct ethnl_req_info req_info = {};
    143         struct nlattr **tb = info->attrs;
    144         const struct ethtool_phy_ops *ops;
    145         struct phy_plca_cfg plca_cfg;
    146         struct net_device *dev;
    147         bool mod = false;
    148         int ret;
    149 
    150         ret = ethnl_parse_header_dev_get(&req_info,
    151                                          tb[ETHTOOL_A_PLCA_HEADER],
    152                                          genl_info_net(info), info->extack,
    153                                          true);
    154         if (!ret)
--> 155                 return ret;

This looks like the if statement is reversed.  Otherwise if this is
a short cut to success, please write it like so:

	if (!ret)
		return 0;


There are a bunch of if (!ret) if statements in this function but the
rest look intentional in context.  It's pretty common to reverse
the last if statement.  (Not a fan, myself though).

    156 
    157         dev = req_info.dev;
    158 
    159         rtnl_lock();
    160 
    161         // check that the PHY device is available and connected
    162         if (!dev->phydev) {
    163                 ret = -EOPNOTSUPP;
    164                 goto out_rtnl;
    165         }
    166 
    167         ops = ethtool_phy_ops;
    168         if (!ops || !ops->set_plca_cfg) {
    169                 ret = -EOPNOTSUPP;
    170                 goto out_rtnl;
    171         }
    172 
    173         ret = ethnl_ops_begin(dev);
    174         if (!ret)
    175                 goto out_rtnl;
    176 
    177         memset(&plca_cfg, 0xff, sizeof(plca_cfg));
    178         plca_update_sint(&plca_cfg.enabled, tb[ETHTOOL_A_PLCA_ENABLED], &mod);
    179         plca_update_sint(&plca_cfg.node_id, tb[ETHTOOL_A_PLCA_NODE_ID], &mod);
    180         plca_update_sint(&plca_cfg.node_cnt, tb[ETHTOOL_A_PLCA_NODE_CNT], &mod);
    181         plca_update_sint(&plca_cfg.to_tmr, tb[ETHTOOL_A_PLCA_TO_TMR], &mod);
    182         plca_update_sint(&plca_cfg.burst_cnt, tb[ETHTOOL_A_PLCA_BURST_CNT],
    183                          &mod);
    184         plca_update_sint(&plca_cfg.burst_tmr, tb[ETHTOOL_A_PLCA_BURST_TMR],
    185                          &mod);
    186 
    187         ret = 0;
    188         if (!mod)
    189                 goto out_ops;
    190 
    191         ret = ops->set_plca_cfg(dev->phydev, &plca_cfg, info->extack);
    192         if (!ret)
    193                 goto out_ops;
    194 
    195         ethtool_notify(dev, ETHTOOL_MSG_PLCA_NTF, NULL);
    196 
    197 out_ops:
    198         ethnl_ops_complete(dev);
    199 out_rtnl:
    200         rtnl_unlock();
    201         ethnl_parse_header_dev_put(&req_info);
    202 
    203         return ret;
    204 }

regards,
dan carpenter