From patchwork Wed Aug 22 09:14:42 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Stanislav Kinsbursky X-Patchwork-Id: 179268 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 350292C0081 for ; Wed, 22 Aug 2012 19:15:15 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758843Ab2HVJOy (ORCPT ); Wed, 22 Aug 2012 05:14:54 -0400 Received: from relay.parallels.com ([195.214.232.42]:33526 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757966Ab2HVJOv convert rfc822-to-8bit (ORCPT ); Wed, 22 Aug 2012 05:14:51 -0400 Received: from msk-exch1.sw.swsoft.com ([10.30.1.231] helo=mail.sw.ru) by relay.parallels.com with esmtps (TLSv1:RC4-MD5:128) (Exim 4.77) (envelope-from ) id 1T471U-0005zP-1P; Wed, 22 Aug 2012 13:14:48 +0400 Received: from [10.30.20.35] (10.30.20.35) by mail.sw.ru (10.30.1.231) with Microsoft SMTP Server (TLS) id 8.3.213.0; Wed, 22 Aug 2012 13:14:46 +0400 Message-ID: <5034A302.4090406@parallels.com> Date: Wed, 22 Aug 2012 13:14:42 +0400 From: Stanislav Kinsbursky User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120714 Thunderbird/14.0 MIME-Version: 1.0 To: Neal Cardwell CC: David Miller , "dhowells@redhat.com" , "netdev@vger.kernel.org" , "rick.jones2@hp.com" , "ycheng@google.com" , "linux-kernel@vger.kernel.org" , "mikulas@artax.karlin.mff.cuni.cz" Subject: Re: [PATCH] tun: don't zeroize sock->file on detach References: <20120809124436.5156.26944.stgit@localhost.localdomain> <20120809.161639.1789560369123168415.davem@davemloft.net> <5033B199.6080305@parallels.com> In-Reply-To: Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 21.08.2012 21:18, Neal Cardwell пишет: > On Tue, Aug 21, 2012 at 12:04 PM, Stanislav Kinsbursky > wrote: >> 10.08.2012 03:16, David Miller пишет: >> >>> From: Stanislav Kinsbursky >>> Date: Thu, 09 Aug 2012 16:50:40 +0400 >>> >>>> This is a fix for bug, introduced in 3.4 kernel by commit >>>> 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d, which, among other things, >>>> replaced >>>> simple sock_put() by sk_release_kernel(). Below is sequence, which leads >>>> to >>>> oops for non-persistent devices: >>>> >>>> tun_chr_close() >>>> tun_detach() <== tun->socket.file = NULL >>>> tun_free_netdev() >>>> sk_release_sock() >>>> sock_release(sock->file == NULL) >>>> iput(SOCK_INODE(sock)) <== dereference on NULL pointer >>>> >>>> This patch just removes zeroing of socket's file from __tun_detach(). >>>> sock_release() will do this. >>>> >>>> Cc: stable@vger.kernel.org >>>> Reported-by: Ruan Zhijie >>>> Tested-by: Ruan Zhijie >>>> Acked-by: Al Viro >>>> Acked-by: Eric Dumazet >>>> Acked-by: Yuchung Cheng >>>> Signed-off-by: Stanislav Kinsbursky >>> >>> >>> Applied, thanks. >>> >> >> Hi, David. >> I found out, that this commit: b09e786bd1dd66418b69348cb110f3a64764626a >> was previous attempt to fix the problem. >> I believe this commit have to be dropped. > > Have you tried testing with that commit reverted? AFAICT from reading > the code, if you revert b09e786bd1dd66418b69348cb110f3a64764626a then > the sockets_in_use count becomes incorrect, because sock_release() > will be calling this_cpu_sub() for each tun socket teardown when there > was no corresponding this_cpu_add() for the tun socket (because the > tun socket is not allocated with sock_alloc()). > > Can you sketch in more detail why that commit should be dropped? > Yep, I've noticed, that first commit patch fixes two problems simultaneously. Here are they: 1) Dereference of invalid SOCK_INODE() 2) sockets_in_use incorrect value. But I believe, that introducing new SOCK_EXTERNALLY_ALLOCATED socket flag and use it in generic code just to handle tun issues is overkill. My patch solves first problem mush simpler, than mentioned commit. About second problem... What about this: ? > neal > diff --git a/net/socket.c b/net/socket.c index dfe5b66..dab462b 100644 --- a/net/socket.c +++ b/net/socket.c @@ -526,8 +526,8 @@ void sock_release(struct socket *sock) if (test_bit(SOCK_EXTERNALLY_ALLOCATED, &sock->flags)) return; - this_cpu_sub(sockets_in_use, 1); if (!sock->file) { + this_cpu_sub(sockets_in_use, 1); iput(SOCK_INODE(sock)); return; }