From: Trond Myklebust <trondmy@primarydata.com>
To: "torvalds@linux-foundation.org" <torvalds@linux-foundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bfields@fieldses.org" <bfields@fieldses.org>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "schumaker.anna@gmail.com" <schumaker.anna@gmail.com>,
        "davej@codemonkey.org.uk" <davej@codemonkey.org.uk>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13
Thread-Topic: [GIT PULL] Please pull NFS client changes for Linux 4.13
Thread-Index: AQHS/B1u2F85s8I96U27Pz1m90AG+KJTYeuAgAOXKACAABx5gIAARTIAgAELdQCAFckMAIAA6JGAgACsC4CAABjegIAAAscA
Date: Tue, 1 Aug 2017 17:30:30 +0000
Message-ID: <1501608628.70813.1.camel@primarydata.com>
References: <cf7efbe6-899c-368d-6d56-c2cb3eb28acc@gmail.com>
         <20170714142543.k5xcbnb4mww3sxpy@codemonkey.org.uk>
         <20170716211530.sx7mn35f2mhmykug@codemonkey.org.uk>
         <1500245845.13893.3.camel@primarydata.com>
         <20170717030504.qca74wsswct26ytn@codemonkey.org.uk>
         <CA+55aFy6kUwPTbzKebO4=BovEc_Nq3XCoYMeofgCM3ypxrkvJg@mail.gmail.com>
         <20170731154322.tfzkukscda4fe7wm@codemonkey.org.uk>
         <CA+55aFwwkc-YJ8WQzE6Ax3BAJ3XXR8fhN+0AEH_U8TtcQn7opg@mail.gmail.com>
         <20170801155131.xy7nbw5ih7ml5fmf@codemonkey.org.uk>
         <CA+55aFyQdHt0qYQ0uBSK+9wfPOyFfx8mJE4r9FfWoxa3Gyei9g@mail.gmail.com>
In-Reply-To: <CA+55aFyQdHt0qYQ0uBSK+9wfPOyFfx8mJE4r9FfWoxa3Gyei9g@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-ID: <BAFECECAE7FBAE47A4571D985E5C122B@namprd11.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Aug 2017 17:30:30.7663
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 03193ed6-8726-4bb3-a832-18ab0d28adb7
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB0075
X-MC-Unique: D3-JpFf2OGGzbxccaB3AKg-1
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id v71HUlFW013403
Content-Length: 3018
Lines: 84

On Tue, 2017-08-01 at 10:20 -0700, Linus Torvalds wrote:
> On Tue, Aug 1, 2017 at 8:51 AM, davej@codemonkey.org.uk
> <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?
> 

I came to the same conclusion yesterday, and have a stable patch that
does something similar. I just got distracted with the other bugs that
were introduced by the exchangeid patch series in Linux-4.9 (including
what looks like a duplicate free issue in nfs4_test_session_trunk()).

I can pass a few of the more critical patches on to Anna for merging in
this cycle, then I've got some clean ups ready for the 4.14 merge
window.

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com