From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 90D4730FC33 for ; Tue, 26 May 2026 15:23:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779809016; cv=none; b=Y2HrEagzH7EJxffFTZUuZ3Ev4KWPRs0EgY7+NeeTYyfepVTZ8tXSyMfoX9dG44Ktev4IUjQ/T70uHrw4rRIXf9+oS16nKU5drLUQTeitJEuoQx+bGT8JMHQFau9Hmn0P6eXY9gzxVlaRzM+897cd9qeMk8mX9IFOHWzpZLII6ck= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779809016; c=relaxed/simple; bh=qSN4hZ1gGEgB+UNBRBinT6DL4fLWAacVRSUh1mAipYM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ipsrr1SNZd224agHJu0D7CEdgbfY4RgXtWhEUTPB1iYm5fI50C5gnKlAKxOJ3lgGbE91x64dL8676r5ls2lJtg5KcIzs6wKpFyW2ocNGeXZ5AN+Yy8iZ7pmHgtQGmmBwfvjD0zD1iD8k1R+FvJPoIDKG8xbXzhX4Qxr/dEEJROM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=BDuK0YBS; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="BDuK0YBS" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 18A1F1F000E9; Tue, 26 May 2026 15:23:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1779809015; bh=pn6lDdvUYTKkSBMw3yepluGky+KaZAI74satoVYpEpQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=BDuK0YBS08KgRFVabkIm6k7EsJI3pcFaNvckQbAgAv6dEylkVkNhfQl0AmWkh131c 883gxURZUnTvXro0zjELP9AeKCjYV2nzyvoWbPynnKog3bsf/Pd6xzjvkifnbImdci 3s8SePW2nmEpDU6byGdBjRiqH/oukzXZ3xfqOq/cASGItx+K/W9B2/46UU56ymXL66 H+hDYPA6lxDrVrZzJ0C8D7vwVYrG4rDatRnlyQeqRaljVP7mDJiPsInO47hW64/hCV iUXvLQIG4DKCe5r8RJc3ZYT+G+tzV8UBp4U1AHkrCiPL3fPY7eAwuaxrTgISFToQHO 9rrD5uagP8THw== Date: Tue, 26 May 2026 16:23:30 +0100 From: Lorenzo Stoakes To: "David Hildenbrand (Arm)" Cc: Mike Rapoport , Andrew Morton , David Carlier , Heechan Kang , "Liam R. Howlett" , Michael Bommarito , Peter Xu , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RESEND] userfaultfd: snapshot VMA state across UFFDIO_COPY retry Message-ID: References: <20260519052516.3315196-1-rppt@kernel.org> <881cec2a-9425-49f5-b96e-4595813a4384@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <881cec2a-9425-49f5-b96e-4595813a4384@kernel.org> On Wed, May 20, 2026 at 04:03:03PM +0200, David Hildenbrand (Arm) wrote: > On 5/20/26 15:48, David Hildenbrand (Arm) wrote: > > On 5/20/26 14:53, Mike Rapoport wrote: > >> On Wed, May 20, 2026 at 01:09:04PM +0200, David Hildenbrand (Arm) wrote: > >>> > >>> As this is all uffd specific, I wonder whether that should be "struct > >>> uffd_vma_snapshot"/"vma_uffd_snapshot" etc. > >> > >> It's local to this file, I wouldn't worry about namespacing until there's > >> an actual need if it will ever arise. > > > > Indeed, fair enough. > > > >> > >>> From a high level, this LGTM. > >>> > >>> I wish we could identify relevant VMA changes more easily. Like, using a per-MM > >>> sequence counter that we simply increment on any VMA changes. > >> > >> Do you mean per-VMA? > >> Per-MM counter would capture unrelated changes, e.g. an masvise() for > >> unrelated range. > > > > I was thinking about a per-MM thing for simplicity. If there were any changes, > > we'd retry (-EAGAIN). > > > > IOW, something like &mm->mm_lock_seq, which we have for per-vma locks already. > > > > Not sure if unrelated changes would really be a problem in practice (another > > thread gabbing the mmap_lock in write mode until we serviced the fault). > > Assume something like the following: > > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > index 180bad42fc79..608bb3c3a613 100644 > --- a/mm/userfaultfd.c > +++ b/mm/userfaultfd.c > @@ -448,9 +448,12 @@ static int mfill_copy_folio_retry(struct mfill_state *state, > { > const struct vm_uffd_ops *orig_ops = vma_uffd_ops(state->vma); > unsigned long src_addr = state->src_addr; > + unsigned int seq; > void *kaddr; > int err; > > + seq = raw_read_seqcount(&mm->mm_lock_seq); > + > /* retry copying with mm_lock dropped */ > mfill_put_vma(state); > > @@ -472,7 +475,7 @@ static int mfill_copy_folio_retry(struct mfill_state *state, > * (e.g. replaced with a hugetlb mapping), making the caller's > * ops pointer stale. > */ > - if (vma_uffd_ops(state->vma) != orig_ops) > + if (read_seqcount_retry(&mm->mm_lock_seq, seq)) > return -EAGAIN; > > err = mfill_establish_pmd(state); > > > In case there is concurrent mmap_write lock, we'd retry. But we triggered and > resolved the fault. So the retry would just succeed in copying from the page, > not triggering another fault here. > > So I'd not expect this to matter in practice? > > > We'd have to implement &mm->mm_lock_seq also without per-VMA locks. Which > wouldn't be too bad given that Dave proposed a patch set to just enable per-VMA > locks unconditionally. That set unfortunately is broken for its ostensible purpose AFAICT, but the idea of making per-VMA locks permanently available is good and I hope that we land that at some point. > > As a hotfix, what you have is probably better, but a simplification like this > might later make sense. Hmmm, but the proposed logic absolutely tolerates changes to the VMA that does not alter the fundamental properties being checked for, so this is an entirely different check no? I don't think we can both say that we only care about properties X, Y, and Z changing and simultaneously tracking whether a VMA write lock was applied since we dropped the lock? Also a VMA write lock having been obtained doesn't mean the VMA was necessarily changed? And then couldn't -EAGAIN cause spurious write locks to abort the operation altogether? Given reacquisition of the lock will fall back to waiting on the mmap_read_lock() if the VMA was write locked, that makes a race very possible. I mean maybe we want to tolerate all of the above and just move to a safer world where we conservatively consider any VMA write lock acquisition to be one in which we abort the operation, which I think would be nicer... > > -- > Cheers, > > David Cheers, Lorenzo