linux-kernel - Re: [PATCH net-next v8 4/5] netconsole: resume previously deactivated target

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: <gmzstxyozxprqt4g6qotq6tjkbzx25e2c5mvvah6w43uvullw4@6vzf7qtvvh62>
Date: Mon, 1 Dec 2025 20:51:40 +0000
From: Andre Carvalho <asantostc@...il.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, 
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v8 4/5] netconsole: resume previously
 deactivated target

On Mon, Dec 01, 2025 at 03:35:04AM -0800, Breno Leitao wrote:
> Given you are completely lockless here, so, there is a chance you hit
> a TOCTOU, also.
> 
> I think you want to have dynamic_netconsole_mutex held during the
> operation of process_resume_target().
> 
>   * mutex_lock(&dynamic_netconsole_mutex);
>   * remove from the list
>   * resume
>   * re-add to the list
>   * mutex_unlock(&dynamic_netconsole_mutex);
>   

This is a pretty good point. Will address this on the next version.

> > +	if (bound_by_mac(nt))
> > +		/* ensure netpoll_setup will retrieve device by mac */
> > +		memset(&nt->np.dev_name, 0, IFNAMSIZ);
> 
> This is a clean-up step that was missing whent the target is getting
> down, and htis is just a work around that doesn't belong in here.
> 
> Please move it to netconsole_process_cleanups_core(), in a separate
> patch.

Sounds good. Will include this as a separated patch on the next version of this
series.

> Something as: 
> 
> 	list_for_each_entry_safe(nt, tmp, &target_cleanup_list, list)
> 		do_netpoll_cleanup(&nt->np);
> 		if (bound_by_mac(nt))
> 			memset(&nt->np.dev_name, 0, IFNAMSIZ);
> 			
> 
> Ideally this should belong to do_netpoll_cleanup(), but let's keep it in
> netconsole_process_cleanups_core() for three reasons:
> 
> 
> 1) Bounding by mac is a netconsole concept
> 2) do_netpoll_cleanup() is only used by netconsole, and I plan to move
>    it back to netconsole. Some PoC in [1]
> 3) bound_by_mac() should be in netconsole and we do not want to export
>    it.
> 
> [1]:
> https://lore.kernel.org/all/20250902-netpoll_untangle_v3-v1-3-51a03d6411be@debian.org/

The reasoning makes sense to me. I considered performing this cleanup on netpoll,
but given your patch opted for this 'hack' before setup - I think doing it on
netconsole_process_cleanups_core makes more sense. 

I need to check this more, but I'm wondering if we would be able to completely
remove dev_name and dev_mac from netpoll and make it part of the netconsole_target.
Perhaps as a future refactoring after your patch series.

> 
> It needs to be initialized earlier before the kzalloc, otherwise we
> might hit a similar problem to the one fixed by e5235eb6cfe0  ("net:
> netpoll: initialize work queue before error checks")
> 
> The code path would be:
>   * alloc_param_target()
> 	  * alloc_and_init()
> 		  * kzalloc() fails and return NULL.
> 		  * resume_wq() is still not initialized
>   fail:
> 	* free_param_target()
> 		* cancel_work_sync(&nt->resume_wq); and resume_wq is not
> 		  initialized

Ack. Will fix this.

> 
> Thanks for the patch,
> --breno

Thanks again for the review. Will submit a new version addressing all the comments
once net-next re-opens.

-- 
Andre Carvalho