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

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2cf128ed-cd74-4025-8ae8-28934abbf25a@redhat.com>
Date: Thu, 15 Aug 2024 18:07:20 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Breno Leitao <leitao@...ian.org>
Cc: kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com,
 thepacketgeek@...il.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Aijay Adams <aijay@...a.com>
Subject: Re: [PATCH net-next] net: netconsole: Populate dynamic entry even if
 netpoll fails

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@...hat.com>
>      Signed-off-by: Breno Leitao <leitao@...ian.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.

Thanks,

Paolo