diff mbox series

[net-next,v2,1/3] netpoll: Ensure clean state on setup failures

Message ID 20240819103616.2260006-2-leitao@debian.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series netconsole: Populate dynamic entry even if netpoll fails | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 17 this patch: 17
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 60 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-20--09-00 (tests: 712)

Commit Message

Breno Leitao Aug. 19, 2024, 10:36 a.m. UTC
Modify netpoll_setup() and __netpoll_setup() to ensure that the netpoll
structure (np) is left in a clean state if setup fails for any reason.
This prevents carrying over misconfigured fields in case of partial
setup success.

Key changes:
- np->dev is now set only after successful setup, ensuring it's always
  NULL if netpoll is not configured or if netpoll_setup() fails.
- np->local_ip is zeroed if netpoll setup doesn't complete successfully.
- Added DEBUG_NET_WARN_ON_ONCE() checks to catch unexpected states.
- Reordered some operations in __netpoll_setup() for better logical flow.

These changes improve the reliability of netpoll configuration, since it
assures that the structure is fully initialized or totally unset.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 net/core/netpoll.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Jakub Kicinski Aug. 20, 2024, 11:20 p.m. UTC | #1
On Mon, 19 Aug 2024 03:36:11 -0700 Breno Leitao wrote:
> +	DEBUG_NET_WARN_ON_ONCE(np->dev);
>  	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
>  		np_err(np, "%s doesn't support polling, aborting\n",
> -		       np->dev_name);
> +		       ndev->name);
>  		err = -ENOTSUPP;
>  		goto out;
>  	}

>  put:
> +	DEBUG_NET_WARN_ON_ONCE(np->dev);

nit: having two warnings feels a bit excessive, let's pick one location?
Breno Leitao Aug. 21, 2024, 8:44 a.m. UTC | #2
Hello Jakub,

On Tue, Aug 20, 2024 at 04:20:10PM -0700, Jakub Kicinski wrote:
> On Mon, 19 Aug 2024 03:36:11 -0700 Breno Leitao wrote:
> > +	DEBUG_NET_WARN_ON_ONCE(np->dev);
> >  	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
> >  		np_err(np, "%s doesn't support polling, aborting\n",
> > -		       np->dev_name);
> > +		       ndev->name);
> >  		err = -ENOTSUPP;
> >  		goto out;
> >  	}
> 
> >  put:
> > +	DEBUG_NET_WARN_ON_ONCE(np->dev);
> 
> nit: having two warnings feels a bit excessive, let's pick one location?

Sure, I think it might be better to keep it in the fail (put:) case, so,
we make sure that netpoll is in a clear state in the failure path.

Thanks
--breno
diff mbox series

Patch

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index a58ea724790c..c5577d250a21 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -626,12 +626,10 @@  int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	const struct net_device_ops *ops;
 	int err;
 
-	np->dev = ndev;
-	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
-
+	DEBUG_NET_WARN_ON_ONCE(np->dev);
 	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
 		np_err(np, "%s doesn't support polling, aborting\n",
-		       np->dev_name);
+		       ndev->name);
 		err = -ENOTSUPP;
 		goto out;
 	}
@@ -649,7 +647,7 @@  int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 
 		refcount_set(&npinfo->refcnt, 1);
 
-		ops = np->dev->netdev_ops;
+		ops = ndev->netdev_ops;
 		if (ops->ndo_netpoll_setup) {
 			err = ops->ndo_netpoll_setup(ndev, npinfo);
 			if (err)
@@ -660,6 +658,8 @@  int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 		refcount_inc(&npinfo->refcnt);
 	}
 
+	np->dev = ndev;
+	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
 	npinfo->netpoll = np;
 
 	/* last thing to do is link it to the net device structure */
@@ -677,6 +677,7 @@  EXPORT_SYMBOL_GPL(__netpoll_setup);
 int netpoll_setup(struct netpoll *np)
 {
 	struct net_device *ndev = NULL;
+	bool ip_overwritten = false;
 	struct in_device *in_dev;
 	int err;
 
@@ -740,6 +741,7 @@  int netpoll_setup(struct netpoll *np)
 				goto put;
 			}
 
+			ip_overwritten = true;
 			np->local_ip.ip = ifa->ifa_local;
 			np_info(np, "local IP %pI4\n", &np->local_ip.ip);
 		} else {
@@ -757,6 +759,7 @@  int netpoll_setup(struct netpoll *np)
 					    !!(ipv6_addr_type(&np->remote_ip.in6) & IPV6_ADDR_LINKLOCAL))
 						continue;
 					np->local_ip.in6 = ifp->addr;
+					ip_overwritten = true;
 					err = 0;
 					break;
 				}
@@ -787,6 +790,9 @@  int netpoll_setup(struct netpoll *np)
 	return 0;
 
 put:
+	DEBUG_NET_WARN_ON_ONCE(np->dev);
+	if (ip_overwritten)
+		memset(&np->local_ip, 0, sizeof(np->local_ip));
 	netdev_put(ndev, &np->dev_tracker);
 unlock:
 	rtnl_unlock();