Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20230914152620.2743033-1-surenb@google.com> <20230914152620.2743033-3-surenb@google.com>
 <4F9BBE45-22D0-4F8D-BA56-CA3459998DC4@gmail.com>
In-Reply-To: <4F9BBE45-22D0-4F8D-BA56-CA3459998DC4@gmail.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Thu, 14 Sep 2023 20:28:23 -0700
Message-ID: <CAJuCfpGWkzDUL4+9evD-Kx5uGoc+=g808CXtc1hrSUdCRMtRgA@mail.gmail.com>
Subject: Re: [PATCH 2/3] userfaultfd: UFFDIO_REMAP uABI
To:     Nadav Amit <nadav.amit@gmail.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, viro@zeniv.linux.org.uk,
        brauner@kernel.org, shuah@kernel.org,
        Andrea Arcangeli <aarcange@redhat.com>, lokeshgidra@google.com,
        Peter Xu <peterx@redhat.com>,
        David Hildenbrand <david@redhat.com>,
        Hugh Dickins <hughd@google.com>, mhocko@suse.com,
        Axel Rasmussen <axelrasmussen@google.com>,
        Mike Rapoport <rppt@kernel.org>,
        Matthew Wilcox <willy@infradead.org>, Liam.Howlett@oracle.com,
        Jann Horn <jannh@google.com>, zhangpeng362@huawei.com,
        bgeffon@google.com, kaleshsingh@google.com, ngeoffray@google.com,
        jdduke@google.com, linux-mm <linux-mm@kvack.org>,
        linux-fsdevel@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-kselftest@vger.kernel.org, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 14, 2023 at 2:57=E2=80=AFPM Nadav Amit <nadav.amit@gmail.com> w=
rote:
>
>
> > On Sep 14, 2023, at 8:26 AM, Suren Baghdasaryan <surenb@google.com> wro=
te:
> >
> > +     if (!pte_same(ptep_clear_flush(src_vma, src_addr, src_pte),
> > +             orig_src_pte))
> > +             BUG_ON(1);
>
> Just a minor detail regarding these few lines:
>
> Besides the less-than-ideal use of BUG_ON() here, I think that this code
> assumes that the PTE cannot change at this point. However, as the PTE was
> still mapped at this point, I think the access and dirty bits can be set.

At this point we are holding PTLs for both PTEs (see
double_pt_lock()).  Can a PTE be modified from under us in this
situation?

>
> tl;dr: this appears to be triggerable by userspace.
>
> [ as for the performance of this code, the lack of batching would mean
>   that for multithreaded applications where more than a single page is
>   remapped, performance would suffer ]

Thanks for the note! I'll see if it's possible to implement some
batching mechanism here.
Thanks,
Suren.