From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guillaume Nault Subject: Re: [PATCH net 1/3] l2tp: hold reference on tunnels in netlink dumps Date: Fri, 13 Apr 2018 18:09:12 +0200 Message-ID: <20180413160912.GA1405@alphalink.fr> References: <20180413.105703.352607094166315307.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, jchapman@katalix.com To: David Miller Return-path: Received: from zimbra.alphalink.fr ([217.15.80.77]:51818 "EHLO zimbra.alphalink.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751714AbeDMQJQ (ORCPT ); Fri, 13 Apr 2018 12:09:16 -0400 Content-Disposition: inline In-Reply-To: <20180413.105703.352607094166315307.davem@davemloft.net> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Apr 13, 2018 at 10:57:03AM -0400, David Miller wrote: > From: Guillaume Nault > Date: Thu, 12 Apr 2018 20:50:33 +0200 > > > l2tp_tunnel_find_nth() is unsafe: no reference is held on the returned > > tunnel, therefore it can be freed whenever the caller uses it. > > This patch defines l2tp_tunnel_get_nth() which works similarly, but > > also takes a reference on the returned tunnel. The caller then has to > > drop it after it stops using the tunnel. > > > > Convert netlink dumps to make them safe against concurrent tunnel > > deletion. > > > > Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP") > > Signed-off-by: Guillaume Nault > > During the entire invocation of l2tp_nl_cmd_tunnel_dump(), the RTNL > mutex is held. > > Therefore no tunnel configuration changes may occur and the tunnel > object will persist and is safe to access. > Yes, but only for updates done with the genl API. For L2TPv2, the tunnel can be created by connecting a PPPOL2TP and a UDP socket. Closing these sockets destroys the tunnel without any RTNL synchronisation. > The netlink dump should be safe as-is. > > Were you actually able to trigger a crash or KASAN warning or is > this purely from code inspection? > Yes, I have a KASAN use-after-free for this case. I remember I saw a few complains about stack traces in commit messages, so I've stopped putting them there. I can paste (stripped) traces again. Just let me know if you have any preference. Guillaume