Received-SPF: pass (google.com: domain of linux-kernel+bounces-154030-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 22 Apr 2024 17:08:09 -0400
From: Peter Xu <peterx@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Nadav Amit <nadav.amit@gmail.com>,
	David Hildenbrand <david@redhat.com>,
	syzbot+d8426b591c36b21c750e@syzkaller.appspotmail.com
Subject: Re: [PATCH] mm/userfaultfd: Reset ptes when close() for wr-protected
 ones
Message-ID: <ZibRuVdLPBu3JtH3@x1n>
References: <20240422133311.2987675-1-peterx@redhat.com>
 <20240422124719.5097e42a736403d306ba7cf0@linux-foundation.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240422124719.5097e42a736403d306ba7cf0@linux-foundation.org>

On Mon, Apr 22, 2024 at 12:47:19PM -0700, Andrew Morton wrote:
> On Mon, 22 Apr 2024 09:33:11 -0400 Peter Xu <peterx@redhat.com> wrote:
> 
> > Userfaultfd unregister includes a step to remove wr-protect bits from all
> > the relevant pgtable entries, but that only covered an explicit
> > UFFDIO_UNREGISTER ioctl, not a close() on the userfaultfd itself.  Cover
> > that too.
> 
> We should include a description of the userspace-visible effects of the
> bug, please.  Always.

Ah, this one is a bit special so I didn't consider copying stable at all,
but I'll be more verbose next time..

The only user visible side effect is the user can observe leftover
wr-protect bits even if the user close()ed on an userfaultfd when releasing
the last reference of it.  However hopefully that should be harmless, and
nothing bad should happen even if so.

This change is now more important after the recent page-table-check patch
we merged in mm-unstable (446dd9ad37d0 ("mm/page_table_check: support
userfault wr-protect entries")), as we'll do sanity check on uffd-wp bits
without vma context.  So it's better if we can 100% guarantee no uffd-wp
bit leftovers, to make sure each report will be valid.

> 
> I see it triggers a WARN, but so what - why ca't we simply delete the
> WARN statement if that's the only effect?  Presumably there are other
> consequences - what are they?

Because that's newly added and we want to keep using those WARNINGs to trap
real bugs (and I'd expect new reports coming after this one.. we at least
have one real bug to fix somewhere..).

> 
> Also, a WARN-triggering bug should be fixed in -stable kernels so we'll
> need a FIXES:, please?

This only triggers due to the most recently added WARNING, so I assume it
shouldn't trigger in any old kernels, even Linus's tree shouldn't trigger
because the WARNING isn't there.

Though maybe it's indeed better to also pick this one up for stable, as it
does similar thing as what below commit does, however just to cover close()
too which was overlooked:

commit f369b07c861435bd812a9d14493f71b34132ed6f
Author: Peter Xu <peterx@redhat.com>
Date:   Thu Aug 11 16:13:40 2022 -0400

    mm/uffd: reset write protection when unregister with wp-mode

So I think that Fixes should be:

Fixes: f369b07c8614 ("mm/uffd: reset write protection when unregister with wp-mode")

Thanks,

-- 
Peter Xu