Re: [PATCH net-next] net: netconsole: Populate dynamic entry even if netpoll fails

[Breno Leitao <leitao@debian.org>]

On Thu, Aug 15, 2024 at 06:07:20PM +0200, Paolo Abeni wrote:
> On 8/15/24 15:46, Breno Leitao wrote:
> > On Wed, Aug 14, 2024 at 12:06:48PM +0200, Paolo Abeni wrote:
> > > I fear the late cleanup could still be dangerous - what if multiple,
> > > consecutive, enabled_store() on the same target fails?
> > > 
> > > I *think* it would be safer always zeroing np->dev in the error path of
> > > netpoll_setup().
> > > 
> > > It could be a separate patch for bisectability.
> > > 
> > > Side note: I additionally think that in the same error path we should
> > > conditionally clear np->local_ip.ip, if the previous code initialized such
> > > field, or we could get weird results if e.g.
> > > - a target uses eth0 with local_ip == 0
> > > - enabled_store() of such target fails e.g. due ndo_netpoll_setup() failure
> > > - address on eth0 changes for some reason
> > > - anoter enabled_store() is issued on the same target.
> > > 
> > > At this point the netpoll target should be wrongly using the old address.
> > 
> > Agree with you. I think we always want to keep struct netpoll objects
> > either initialized or unitialized, not keeping them half-baked.
> > 
> > How about the following patch:
> 
> Overall LGTM, a couple of minor comments below.
> 
> >      netpoll: Ensure clean state on setup failures
> >      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.
> >      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>
> > 
> > diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> > index a58ea724790c..348d76a51c20 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 */
> > @@ -681,6 +681,7 @@ int netpoll_setup(struct netpoll *np)
> >   	int err;
> >   	rtnl_lock();
> > +	DEBUG_NET_WARN_ON_ONCE(np->dev);
> 
> This looks redundant
> 
> >   	if (np->dev_name[0]) {
> >   		struct net *net = current->nsproxy->net_ns;
> >   		ndev = __dev_get_by_name(net, np->dev_name);
> > @@ -782,11 +783,14 @@ int netpoll_setup(struct netpoll *np)
> >   	err = __netpoll_setup(np, ndev);
> >   	if (err)
> > -		goto put;
> > +		goto clear_ip;
> >   	rtnl_unlock();
> >   	return 0;
> > +clear_ip:
> > +	memset(&np->local_ip, 0, sizeof(np->local_ip));
> 
> I think it would be better to clear the local_ip only if np->local_ip was
> set/initialized/filled by netpoll_setup() otherwise the sysfs contents could
> suddenly/unexpetedly change on failure.

Makes sense. I was not able to come up with any other solution other than
tracking the overwrite and checking it later, which is admittedly not
beautiful, but might do the job. Let me know if you think about
something more elegant.

	 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);


I really appreciate your time helping me here!
--breno