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;
Date:   Thu, 6 Oct 2022 12:44:23 -0300
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Jann Horn <jannh@google.com>
Cc:     Linux-MM <linux-mm@kvack.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Christoph Hellwig <hch@lst.de>,
        kernel list <linux-kernel@vger.kernel.org>,
        David Hildenbrand <david@redhat.com>
Subject: Re: ptep_get_lockless() on 32-bit x86/mips/sh looks wrong
Message-ID: <Yz7316NubHtw2xCa@nvidia.com>
References: <CAG48ez3h-mnp9ZFC10v+-BW_8NQvxbwBsMYJFP8JX31o0B17Pg@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAG48ez3h-mnp9ZFC10v+-BW_8NQvxbwBsMYJFP8JX31o0B17Pg@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?EWtcOpl85K6X4EVNX50Pxuh/ItvmxTDtPEyWin5sPPmTrNnLIS09816IJueo?=
 =?us-ascii?Q?AkaAQo544IAx6Vi/Pvb2WJhdX370OJyqt6MfJXbDv+vUfHhOpe+iuqwCrqGA?=
 =?us-ascii?Q?wQ0NHAX8jIH3SJ2JfkAjOj35ZekvSdxfyMOwzFiWaLulAt5pnW+yph/DUaLE?=
 =?us-ascii?Q?Agzxy7wdgS/kDKeHTq4IwQgTR5IPrDz38yWRMCg4YqdfY2iszmsUx7inphLJ?=
 =?us-ascii?Q?DIIKWIpBxSwisJiPZrH/H95SRcO1M2khQTx6vD7cQW5d4opQWFAUbfMN0tnA?=
 =?us-ascii?Q?5bkptlwyQjpDiDtFDnQTUrXhaAFR3jLWw322kFDs9LZdsvSJr8v/UA//Gxdk?=
 =?us-ascii?Q?HZr9KxtlXjd4ISJMJyn6AL73QmgobXhcHi6LnthrQMrMCksIBE8BxI8IbfQh?=
 =?us-ascii?Q?Py/eDblnKZ8RmzhnHOELJcWk3hx9eZ+vDvm5gyw1L5i+POLnifDcYsM50WVv?=
 =?us-ascii?Q?sBR/IPFxifvyH1onx0KXdVYm+fCheOj6A9OcGQK2HdABPAKEiIem3cxAdzh7?=
 =?us-ascii?Q?bSD1q17zc0SwGfDmSCmbySO2ZYQI7+kRVT7hI342MTG73GxbUQxc5lRJwfjC?=
 =?us-ascii?Q?qNagLegVFWTc0l48krBiUXtcsrHydWxmQQghNmnStCFKw2YpFaS1Nys2U4XX?=
 =?us-ascii?Q?BAXyYVARMymCrAPmv0SsBVSYTmi0SJYFt53e/pl2R70AyzoylaLN2WL0kiLG?=
 =?us-ascii?Q?/yBJvv4i5PRLBgzZrOAXUSb4IwhfPdL9uKiBS2sYz/f6AiJGkyfsaGHGqHY8?=
 =?us-ascii?Q?fb7uehDU9erltG5625+uQA7NmfMzHujSF/+Pd6G3CZEM9Bp084jHVAkJw3Qb?=
 =?us-ascii?Q?WQ8XIEthCW5CxR4qq6RyCRuYQIRG41sM0DKZgBov6A1hp8ZC+CN67RuC8gjB?=
 =?us-ascii?Q?nvgCjEcAZ8gOuoLlXNzIuS69BJ/aR3Gf6G1x4bfQyhdyGd0Kne04TGred3Fc?=
 =?us-ascii?Q?e9lwzl+KaFbHG1qkj2+hoxm+7gykW2HNw8U/1YU8YkHuodJozHk7igF+v/Py?=
 =?us-ascii?Q?CvBCiJ9rlx3sqGC/uhrs3YyJYpwoZ106AfZqiLXxmYpNqm5CTKWdtQclhIZr?=
 =?us-ascii?Q?PnbEbvdHMqF0/ARl+6hB9uMGmbHfpPHHr1eWGywYTRZQ0gPnW30DVkiESyQ1?=
 =?us-ascii?Q?SAIHVXgd7jtPe5rjIC4ANjV+dwsYEiBVLGG1I6LQZe/b/w2K1BrxqUaoKgbV?=
 =?us-ascii?Q?KZp3Rc+3ndQp94bE706oA8fTlJsJxJMwcZmUpSLB5Z51jun3NBp/WBu+tWD+?=
 =?us-ascii?Q?YtavUnW/AEa8IzDBz68YK1vhaJlVHHW8ZZC1e2B5qIJQjWFhXXssIR/Uh/r/?=
 =?us-ascii?Q?b3qHdDrzAE13RGCGz/pK1VkurwHJBzZ95PCrS0S4mv/Me3feA0Rq/578sr05?=
 =?us-ascii?Q?BvVVzmddTOmiuogoHpFq46C7cbAzVmgymnz7LpJmkVSyvYcvUYPo8QAjh4VD?=
 =?us-ascii?Q?IJh+H6e7uuq1Z70OhvP7okRn/oUF1ZG3mrmKgnN2z8Tx8FJIuz2kT3joEq2w?=
 =?us-ascii?Q?DDyVJyKhvjPPv7bIy4062CW8gsBvJUT2oeA5OhN48R0n7fOR0WOUBqN3Cdp8?=
 =?us-ascii?Q?0tFgMTyGCztWFaUTuUg=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 05ed2f24-94c0-4cbc-5354-08daa7b1a4c9
X-MS-Exchange-CrossTenant-AuthSource: LV2PR12MB5869.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Oct 2022 15:44:24.2835
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ycsMHouiDpeSNX2HzxuWnj854c5YfnLI1Kb6jN8gsren5iTyVO0bNU79HOD6H56R
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB6791
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Oct 06, 2022 at 05:23:59PM +0200, Jann Horn wrote:
> ptep_get_lockless() does the following under CONFIG_GUP_GET_PTE_LOW_HIGH:
> 
> pte_t pte;
> do {
>   pte.pte_low = ptep->pte_low;
>   smp_rmb();
>   pte.pte_high = ptep->pte_high;
>   smp_rmb();
> } while (unlikely(pte.pte_low != ptep->pte_low));
> 
> It has a comment above it that argues that this is correct because:
> 1. A present PTE can't become non-present and then become a present
> PTE pointing to another page without a TLB flush in between.
> 2. TLB flushes involve IPIs.
> 
> As far as I can tell, in particular on x86, _both_ of those
> assumptions are false; perhaps on mips and sh only one of them is?
> 
> Number 2 is straightforward: X86 can run under hypervisors, and when
> it runs under hypervisors, the MMU paravirtualization code (including
> the KVM version) can implement remote TLB flushes without IPIs.
> 
> Number 1 is gnarlier, because breaking that assumption implies that
> there can be a situation where different threads see different memory
> at the same virtual address because their TLBs are incoherent. But as
> far as I know, it can happen when MADV_DONTNEED races with an
> anonymous page fault, because zap_pte_range() does not always flush
> stale TLB entries before dropping the page table lock. I think that's
> probably fine, since it's a "garbage in, garbage out" kind of
> situation - but if a concurrent GUP-fast can then theoretically end up
> returning a completely unrelated page, that's bad.
> 
> 
> Sadly, mips and sh don't define arch_cmpxchg_double(), so we can't
> just change ptep_get_lockless() to use arch_cmpxchg_double() and be
> done with it...

I think the argument here has nothing to do with IPIs, but is more a
statement on memory ordering. What we want to get is a non-torn load
of low/high, under some restricted rules.

PTE writes should be ordered so that the present/not present bit is
properly:

Zapping a PTE:

write_low (not present)
wmb()
write_high (a)
wmb()

Reestablish a PTE:

write_high (b)
wmb()
write_low (present)
wmb()

This ordering is necessary to make the TLB's atomic 64 bit load work
properly, otherwise the TLB could read a present entry with a bogus
other half!

For ptep_get_lockless() we define non-torn as meaning the same as for the TLB:

     pre-zap low / high (present)
  restablish low / high (b) (present)
         any low / any high (not present)

Other combinations are forbidden.

The read side has a corresponding list of reads:

read_low
rmb()
read_high
rmb()
read_low

So, it seems plausible this could be OK based only on atomics (I did
not check that the present bit is properly placed in the right
low/high). Do you see a way the atomics don't work out?

Jason