Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201014204529.934574-1-andrii@kernel.org> <20201014230812.GK3576660@ZenIV.linux.org.uk>
In-Reply-To: <20201014230812.GK3576660@ZenIV.linux.org.uk>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 14 Oct 2020 18:07:48 -0700
Message-ID: <CAEf4BzZQdkJaaqx7CQ6kjA+_nKuE3N1sBzR933wohmB+a5GvpA@mail.gmail.com>
Subject: Re: [PATCH] fs: fix NULL dereference due to data race in prepend_path()
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Andrii Nakryiko <andrii@kernel.org>, linux-fsdevel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Kernel Team <kernel-team@fb.com>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Oct 14, 2020 at 4:08 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Oct 14, 2020 at 01:45:28PM -0700, Andrii Nakryiko wrote:
> > Fix data race in prepend_path() with re-reading mnt->mnt_ns twice without
> > holding the lock. is_mounted() does check for NULL, but is_anon_ns(mnt->mnt_ns)
> > might re-read the pointer again which could be NULL already, if in between
> > reads one of kern_unmount()/kern_unmount_array()/umount_tree() sets mnt->mnt_ns
> > to NULL.
>
> Cute...  What config/compiler has resulted in that?  I agree with the analysis, but

Don't know for sure, but nothing special or exotic, a typical Facebook
production kernel config and some version of GCC (I didn't check
exactly which one).

Just given enough servers in the fleet, with time and intensive
workloads races like this, however unlikely, do surface up pretty
regularly.

> I really hate the open-coded (and completely unexplained) use of IS_ERR_OR_NULL()
> there.
>
> > -                     if (is_mounted(vfsmnt) && !is_anon_ns(mnt->mnt_ns))
> > +                     mnt_ns = READ_ONCE(mnt->mnt_ns);
> > +                     /* open-coded is_mounted() to use local mnt_ns */
> > +                     if (!IS_ERR_OR_NULL(mnt_ns) && !is_anon_ns(mnt_ns))
> >                               error = 1;      // absolute root
> >                       else
> >                               error = 2;      // detached or not attached yet
>
> Better turn that into an inlined helper in fs/mount.h, next to is_mounted(), IMO,
> and kill that IS_ERR_OR_NULL garbage there.  What that thing does is
>         if (ns == NULL || ns == MNT_NS_INTERNAL)
> and it's *not* on any kind of hot path to warrant that kind of microoptimizations.

Sounds good. I didn't know code well enough to infer NULL ||
MNT_NS_INTERNAL instead of IS_ERR_OR_NULL from is_mounted(), so just
open-coded the latter.

>
> So let's make that
>
> static inline bool is_real_ns(struct mnt_namespace *mnt_ns)
> {
>         return mnt_ns && mnt_ns != MNT_NS_INTERNAL;
> }
>
> turn is_mounted(m) into is_real_ns(m->mnt_ns) and replace that line in your fix
> with
>                         if (is_real_ns(mnt_ns) && !is_anon_ns(mnt_ns))
>
> Objections?

Nope, I'll send a follow-up patch, thanks.