linux-kernel - Re: [PATCH v2 04/23] netconsole: Call netpoll

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: <20101109120752.GA18269@hmsreliant.think-freely.org>
Date:	Tue, 9 Nov 2010 07:07:52 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Mike Waychison <mikew@...gle.com>
Cc:	simon.kagstrom@...insight.net, davem@...emloft.net,
	Matt Mackall <mpm@...enic.com>, adurbin@...gle.com,
	linux-kernel@...r.kernel.org, chavey@...gle.com,
	Greg KH <greg@...ah.com>,
	Américo Wang <xiyou.wangcong@...il.com>,
	akpm@...ux-foundation.org, linux-api@...r.kernel.org
Subject: Re: [PATCH v2 04/23] netconsole: Call netpoll_cleanup() in process
 context

On Mon, Nov 08, 2010 at 12:32:00PM -0800, Mike Waychison wrote:
> The netconsole driver currently deadlocks if a NETDEV_UNREGISTER event
> is received while netconsole is in use, which in turn causes it to pin a
> reference to the network device.  The first deadlock was dealt with in
> 3b410a31 so that we wouldn't recursively grab RTNL, but even calling
> __netpoll_cleanup isn't safe to do considering that we are in atomic
> context.  __netpoll_cleanup assumes it can sleep and has several
> sleeping calls, such as synchronize_rcu_bh and
> cancel_rearming_delayed_work.
> 
> Fix this by deferring netpoll_cleanup using scheduling work that
> operates in process context.  We have to grab a reference to the
> config_item in this case as we need to pin the item in place until it is
> operated on.
> 
> Signed-off-by: Mike Waychison <mikew@...gle.com>
> ---
>  drivers/net/netconsole.c |   55 ++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 288a025..02ba5c4 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -106,6 +106,7 @@ struct netconsole_target {
>  #endif
>  	int			np_state;
>  	struct netpoll		np;
> +	struct work_struct	cleanup_work;
>  };
>  
>  #ifdef	CONFIG_NETCONSOLE_DYNAMIC
> @@ -166,6 +167,22 @@ static void netconsole_target_put(struct netconsole_target *nt)
>  
>  #endif	/* CONFIG_NETCONSOLE_DYNAMIC */
>  
> +static void deferred_netpoll_cleanup(struct work_struct *work)
> +{
> +	struct netconsole_target *nt;
> +	unsigned long flags;
> +
> +	nt = container_of(work, struct netconsole_target, cleanup_work);
> +	netpoll_cleanup(&nt->np);
> +
> +	spin_lock_irqsave(&target_list_lock, flags);
> +	BUG_ON(nt->np_state != NETPOLL_CLEANING);
> +	nt->np_state = NETPOLL_DISABLED;
> +	spin_unlock_irqrestore(&target_list_lock, flags);
> +
> +	netconsole_target_put(nt);
> +}
> +
Where is the synchronization on the new work queue when the module is getting
removed? The target get/put code does nothing to the module refcount, and
cleanup_netconsole just deletes targets, it doesn't block or fail on netconsole
refcounts, so you could run this work after the module has been removed and oops
the system.

Neil
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/