Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230427225647.1101172-1-dave.hansen@linux.intel.com>
 <CAHk-=wgrdOm8RwO+u8bydrbgs0wXJV_9mBYAtzX9d9hEY7a25A@mail.gmail.com>
 <CAHk-=wiHvdgbLxayyCsRU45JnKCZkCOpEJPfOvE3eYCnGjVcSA@mail.gmail.com>
 <20230429003822.n3mglslg666j3npp@box.shutemov.name> <641a9348-a052-6bb5-e6c7-64acb6405328@intel.com>
 <CAHk-=wgnGoj8qZg3SRh1Z1-mU=b3ryydXZykUuarnrAn6LuHBw@mail.gmail.com>
In-Reply-To: <CAHk-=wgnGoj8qZg3SRh1Z1-mU=b3ryydXZykUuarnrAn6LuHBw@mail.gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 2 May 2023 13:14:33 -0700
Message-ID: <CAHk-=wgN=GY2A0htYQQRmPaLCorWnFatY_XO0X2w_m9xXQDcPA@mail.gmail.com>
Subject: Re: [GIT PULL] x86/mm for 6.4
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     "Kirill A. Shutemov" <kirill@shutemov.name>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        linux-kernel@vger.kernel.org, kirill.shutemov@linux.intel.com,
        Peter Zijlstra <peterz@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, May 2, 2023 at 9:00=E2=80=AFAM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> > I guess it also wouldn't matter as much either if we hid it in a helper
> > like the attached patch and I didn't have to read it twice. ;)
>
> Yeah, I think that's a good solution.

Hmm. And as I was rebasing the patch to fix up my patch, I realized
that the current -git top-of-tree state is actually broken.

That

  #define access_ok(addr, size)                                           \
  ({                                                                      \
          WARN_ON_IN_IRQ();                                               \
          likely(__access_ok(untagged_addr(addr), size));                 \
  })

is actually *wrong* in two ways.

Now, in my original patch, I added a comment about how that
"WARN_ON_IN_IRQ()" is bogus and this shouldn't be x86-specific at all.

I ended up going back in time to see why it was added, and I think it
was added because we used to access 'current' in access_ok(), due to
it using that user_addr_max() thing:

        likely(!__range_not_ok(addr, size, user_addr_max()));

but that was all removed by the set_fs() removal by Christoph Hellwig.

So there is actually nothing task-related in "access_ok()" any more,
and any warning about using it in the wrong context is just bogus.
That warning simply shouldn't exist any more (or maybe it should be in
a different place, like the actual user copy functions)

But that's actually not the problem with the above.

No, the problem is that probably *because* "access_ok()" has that
warning, not all users use "access_ok()" at all. We have places that
use "__access_ok()" instead. Like copy_from_nmi().

So now copy_from_nmi() doesn't do the untagging, so if you were to use
tagged pointers for the stack, you'd not get stack traces.

End result: I think that

 (a) that WARN_ON_IN_IRQ() is actively detrimental and causes problems

 (b) the current "use untagged_addr() in access_ok()" model is also broken

and my patch - which was meant to just improve code generation -
actually fixes this.

            Linus