Move validation into set, removing .set_validate operation as its current
implementation holds the rtnl lock for acquiring the PHY device, defeating
the intended purpose of checking before grabbing the lock.

Reported-by: syzbot+ec369e6d58e210135f71@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ec369e6d58e210135f71
Fixes: 31748765bed3 ("net: ethtool: pse-pd: Target the command to the requested PHY")
Signed-off-by: Diogo Jahchan Koike <djahchankoike@gmail.com>
---
v4:
  - remove .set_validate op, move validation to set
v3: https://lore.kernel.org/all/20240826173913.7763-1-djahchankoike@gmail.com/
  - hold rtnl lock while interacting with the PHY device
  - rollback to v1
v2: https://lore.kernel.org/all/20240826140628.99849-1-djahchankoike@gmail.com/
v1: https://lore.kernel.org/all/20240826130712.91391-1-djahchankoike@gmail.com/
---
 net/ethtool/pse-pd.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 507cb21d6bf0..b56b79f41a3a 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -222,13 +222,10 @@ const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1] = {
 };
 
 static int
-ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
+ethnl_set_pse_validate(struct phy_device *phydev, struct genl_info *info)
 {
 	struct nlattr **tb = info->attrs;
-	struct phy_device *phydev;
-
-	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
-				      info->extack);
+	
 	if (IS_ERR_OR_NULL(phydev)) {
 		NL_SET_ERR_MSG(info->extack, "No PHY is attached");
 		return -EOPNOTSUPP;
@@ -254,7 +251,7 @@ ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
 		return -EOPNOTSUPP;
 	}
 
-	return 1;
+	return 0;
 }
 
 static int
@@ -262,12 +259,13 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 {
 	struct nlattr **tb = info->attrs;
 	struct phy_device *phydev;
-	int ret = 0;
+	int ret;
 
 	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
 				      info->extack);
-	if (IS_ERR_OR_NULL(phydev))
-		return -ENODEV;
+	ret = ethnl_set_pse_validate(phydev, info);
+	if (ret)
+		return ret;
 
 	if (tb[ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT]) {
 		unsigned int pw_limit;
@@ -314,7 +312,6 @@ const struct ethnl_request_ops ethnl_pse_request_ops = {
 	.fill_reply		= pse_fill_reply,
 	.cleanup_data		= pse_cleanup_data,
 
-	.set_validate		= ethnl_set_pse_validate,
 	.set			= ethnl_set_pse,
 	/* PSE has no notification */
 };
-- 
2.43.0

Hi Diogo,

On Wed, 28 Aug 2024 14:43:17 -0300
Diogo Jahchan Koike <djahchankoike@gmail.com> wrote:

> Move validation into set, removing .set_validate operation as its current
> implementation holds the rtnl lock for acquiring the PHY device, defeating
> the intended purpose of checking before grabbing the lock.

[...] 

>  static int
> -ethnl_set_pse_validate(struct ethnl_req_info *req_info, struct genl_info *info)
> +ethnl_set_pse_validate(struct phy_device *phydev, struct genl_info *info)
>  {
>  	struct nlattr **tb = info->attrs;
> -	struct phy_device *phydev;
> -
> -	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PSE_HEADER],
> -				      info->extack);
> +	
   ^^^
You have an extra white space here. Make sure you run your patch through
the checkpatch script to detect this kind of issues.

The rest looks good to me. For the next version,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

Hi Maxime,

On 8/29/24 05:35, Maxime Chevallier wrote
> You have an extra white space here. Make sure you run your patch through
> the checkpatch script to detect this kind of issues. 

My apologies, I did forget to run checkpatch, thanks for pointing that out. Will

send the corrected patch.


Thanks,

Diogo Jahchan Koike