netdev - Re: [RFC PATCH 2/2] netconsole: Defer netpoll cleanup to avoid lock release during list traversal

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: <5145c46c47d98d917c8ef1401cdac15fc5f8b638.camel@surriel.com>
Date: Thu, 18 Jul 2024 15:53:54 -0400
From: Rik van Riel <riel@...riel.com>
To: Breno Leitao <leitao@...ian.org>, kuba@...nel.org, davem@...emloft.net, 
	edumazet@...gle.com, pabeni@...hat.com
Cc: thepacketgeek@...il.com, horms@...nel.org, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, paulmck@...nel.org, davej@...emonkey.org.uk
Subject: Re: [RFC PATCH 2/2] netconsole: Defer netpoll cleanup to avoid lock
 release during list traversal

On Thu, 2024-07-18 at 11:43 -0700, Breno Leitao wrote:
> 
> +/* Clean up every target in the cleanup_list and move the clean
> targets back to the
> + * main target_list.
> + */
> +static void netconsole_process_cleanups_core(void)
> +{
> +	struct netconsole_target *nt, *tmp;
> +	unsigned long flags;
> +
> +	/* The cleanup needs RTNL locked */
> +	ASSERT_RTNL();
> +
> +	mutex_lock(&target_cleanup_list_lock);
> +	list_for_each_entry_safe(nt, tmp, &target_cleanup_list,
> list) {
> +		/* all entries in the cleanup_list needs to be
> disabled */
> +		WARN_ON_ONCE(nt->enabled);
> +		do_netpoll_cleanup(&nt->np);
> +		/* moved the cleaned target to target_list. Need to
> hold both locks */
> +		spin_lock_irqsave(&target_list_lock, flags);
> +		list_move(&nt->list, &target_list);
> +		spin_unlock_irqrestore(&target_list_lock, flags);
> +	}
> +	WARN_ON_ONCE(!list_empty(&target_cleanup_list));
> +	mutex_unlock(&target_cleanup_list_lock);
> +}
> +
> +/* Do the list cleanup with the rtnl lock hold */
> +static void netconsole_process_cleanups(void)
> +{
> +	rtnl_lock();
> +	netconsole_process_cleanups_core();
> +	rtnl_unlock();
> +}
> 
I've got what may be a dumb question.

If the traversal of the target_cleanup_list happens under
the rtnl_lock, why do you need a new lock, and why is there
a wrapper function that only takes this one lock, and then
calls the other function?

Are you planning a user of netconsole_process_cleanups_core()
that already holds the rtnl_lock and should not use this
wrapper?

Also, the comment does not explain why the rtnl_lock is held.
We can see that it grabs it, but not why. It would be nice to
have that in the comment.



-- 
All Rights Reversed.