From patchwork Tue Aug 1 17:20:31 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 9875107 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 773D760361 for ; Tue, 1 Aug 2017 17:20:49 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5FF712808F for ; Tue, 1 Aug 2017 17:20:49 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 54D16286FF; Tue, 1 Aug 2017 17:20:49 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C3C1F2808F for ; Tue, 1 Aug 2017 17:20:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751910AbdHARUe (ORCPT ); Tue, 1 Aug 2017 13:20:34 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:37721 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751496AbdHARUd (ORCPT ); Tue, 1 Aug 2017 13:20:33 -0400 Received: by mail-oi0-f66.google.com with SMTP id j194so3007446oib.4; Tue, 01 Aug 2017 10:20:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to; bh=wQDelSSMzk/3UsR3gw0TUy9ENPoxkQE6vR3CGqVPSO8=; b=itmpUeWVU5MdVynXGjw3N5Q0+qzkDCS2lTB1QPBvokb65kEmA4nsESXwabmkvvb3zT kFq3iZ0LRHUUt77wGiec4CKuQgDqG2IbFegJhOw6T+NbWOAQ1lMUrOpYBYEXQ4gYXGEr ERD7B4ZuDjme6JmMAMxobH2KLImwSMXS3QffzR580ka4072EyBU0z4AHmexLNg+6thtI u9wikkaLKN3yb6SdjlsFFJsSnVoPN+MkLMsa0FuwVQauJ+mho+95H8mDv4uBnQEuyrEF C58Zat8ElvOWKw69Wt7m/BtW7RlrfOKDid8jcOwaAQ/HBMzODDHxHhykPLb75TGWNro6 VjOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to; bh=wQDelSSMzk/3UsR3gw0TUy9ENPoxkQE6vR3CGqVPSO8=; b=BHEDpI3253wRj1X3Uvw/Uw+q4Hh2wIsopk71rL/+FPUDyQ/5WxvDIXJG5wPfAVK+XU sPD/PJmyBSYPn7jebhhN4VgYkx5pr2XBaj15yn7qelMDxkiK8i+Huo8es3ML7195YhQ0 I97yIQuZbFv0V7T8O9BPyl1fHLEKlLVmQPiwUS0h+IyQtOAIkmZ5tLeJmzAdc99wRLHx MAWotHXeGyrukD8HL989gpff4xIsOQZgEcoGtN4afRd7KuLdMmmdJGEWY5iqz66usbkC DGwLMuPhU9mAk0BXxFR6d8yKskncgjcPlmll0x8rj2BpvXut+SZjgBRjf56fddJxMU5y QZdA== X-Gm-Message-State: AIVw111kvnyoubKr9VdGiUxCkx9/wfTT8GOHSQrXgVdNc0rEmdug3c/7 uLI3sFLxUDKExoQNDQzEk7fWArPR5w== X-Received: by 10.202.172.11 with SMTP id v11mr16254286oie.164.1501608032259; Tue, 01 Aug 2017 10:20:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.71.109 with HTTP; Tue, 1 Aug 2017 10:20:31 -0700 (PDT) In-Reply-To: <20170801155131.xy7nbw5ih7ml5fmf@codemonkey.org.uk> References: <20170714142543.k5xcbnb4mww3sxpy@codemonkey.org.uk> <20170716211530.sx7mn35f2mhmykug@codemonkey.org.uk> <1500245845.13893.3.camel@primarydata.com> <20170717030504.qca74wsswct26ytn@codemonkey.org.uk> <20170731154322.tfzkukscda4fe7wm@codemonkey.org.uk> <20170801155131.xy7nbw5ih7ml5fmf@codemonkey.org.uk> From: Linus Torvalds Date: Tue, 1 Aug 2017 10:20:31 -0700 X-Google-Sender-Auth: q-W2d25CtZ_ndcIB2PbZ-QQ7xtc Message-ID: Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 To: "davej@codemonkey.org.uk" , Trond Myklebust , "linux-kernel@vger.kernel.org" , "bfields@fieldses.org" , "linux-nfs@vger.kernel.org" , "schumaker.anna@gmail.com" , "linux-fsdevel@vger.kernel.org" Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Aug 1, 2017 at 8:51 AM, davej@codemonkey.org.uk wrote: > On Mon, Jul 31, 2017 at 10:35:45PM -0700, Linus Torvalds wrote: > > Any chance of getting the output from > > > > ./scripts/faddr2line vmlinux nfs4_exchange_id_done+0x3d7/0x8e0 > > > Hm, that points to this.. > > 7463 /* Save the EXCHANGE_ID verifier session trunk tests */ > 7464 memcpy(clp->cl_confirm.data, cdata->args.verifier->data, > 7465 sizeof(clp->cl_confirm.data)); Ok, that certainly made no sense to me, because the KASAN report made it look like a stale pathname access (allocated in getname, freed in putname), but I think the issue is more fundamental than that. That cdata->args.verifier seems to be entirely broken. AT least for the "xprt == NULL" case, it does the following: - use the address of a local variable ("&verifier") - wait for the rpc completion using rpc_wait_for_completion_task(). That's unacceptably buggy crap. rpc_wait_for_completion_task() will happily exit on a deadly signal even if the rpc hasn't been completed, so now you'll have a stale pointer to a stack that has been freed. So I think the 'pathname' part may actually be entirely a red herring, and it's the underlying access itself that just picks up a random pointer from a stack that now contains something different. And KASAN didn't notice the stale stack access itself, because the stack slot is still valid - it's just no longer the original 'verifier' allocation. Or *something* like that. None of this looks even remotely new, though - the code seems to go back to 2009. Have you just changed what you're testing to trigger these things? I'm not even sure why it does that stupid stack allocation. It does a *real* allocation just a few lines later: struct nfs41_exchange_id_data *calldata ... calldata = kzalloc(sizeof(*calldata), GFP_NOFS); and the whole verifier structure could easily have been part of that same allocation as far as I can tell. And that really might seem to be the right thing to do. TOTALLY UNTESTED PROBABLY COMPLETE CRAP patch attatched. That patch compiles for me. It *might* even work. Or it might just be the ramblings of a diseased mind. Anna? Trond? So caveat probatorem, Linus fs/nfs/nfs4proc.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 18ca6879d8de..0712af3d38f8 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -7490,6 +7490,11 @@ static const struct rpc_call_ops nfs4_exchange_id_call_ops = { .rpc_release = nfs4_exchange_id_release, }; +struct verifier_and_calldata { + struct nfs41_exchange_id_data calldata; + nfs4_verifier verifier; +}; + /* * _nfs4_proc_exchange_id() * @@ -7498,7 +7503,8 @@ static const struct rpc_call_ops nfs4_exchange_id_call_ops = { static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred, u32 sp4_how, struct rpc_xprt *xprt) { - nfs4_verifier verifier; + struct verifier_and_calldata *vna; + nfs4_verifier *verifier; struct rpc_message msg = { .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_EXCHANGE_ID], .rpc_cred = cred, @@ -7516,14 +7522,17 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred, if (!atomic_inc_not_zero(&clp->cl_count)) return -EIO; - calldata = kzalloc(sizeof(*calldata), GFP_NOFS); - if (!calldata) { + vna = kzalloc(sizeof(*vna), GFP_NOFS); + if (!vna) { nfs_put_client(clp); return -ENOMEM; } + /* kfree() of calldata will also free the verifier */ + calldata = &vna->calldata; + verifier = &vna->verifier; if (!xprt) - nfs4_init_boot_verifier(clp, &verifier); + nfs4_init_boot_verifier(clp, verifier); status = nfs4_init_uniform_client_string(clp); if (status) @@ -7566,7 +7575,7 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred, RPC_TASK_SOFT|RPC_TASK_SOFTCONN|RPC_TASK_ASYNC; calldata->args.verifier = &clp->cl_confirm; } else { - calldata->args.verifier = &verifier; + calldata->args.verifier = verifier; } calldata->args.client = clp; #ifdef CONFIG_NFS_V4_1_MIGRATION