From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (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 747C7377EC8 for ; Mon, 30 Mar 2026 19:38:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774899532; cv=none; b=IW9lx/rmFcVXGAFAUrj5Vkf4sOlnJXjmyZJItjjfmbLVrG+Zfrlwcvr+Bli204qpcSipG2y5JzjdNKNGZh7P5lCk+qklY8nDPUhQACNlR4V/4I2BzLBvz/OnznxTKrdZylbLPaBOwcJABJi6o8Ftqbtd/dAvLLCSg+OeePAkOJc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774899532; c=relaxed/simple; bh=OJMY2qqH7b7QF5A248KWeDzZvpuO7Xln22PDNd7Q1s0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=jjGW090zrN580UjVAz2HRcDKcwqzqkg6F1nexy+EbFhWdiolj3K+20JH89q7WJzaqMescXJbRVegsDHCGGFi2jz++uYDGsowXSgt+cWCuTCy6pdPs3T7OCqZKsB3q579tnLWKNfisXPxNWRKd8dfnvjlnzkOUJi7s0v3uooSD0A= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=W2K0WkSp; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=AjJvkaJx; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="W2K0WkSp"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="AjJvkaJx" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1774899530; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=b0GrCWjFJdwivrglfaRGY9APiPoMwEIvIsexr/luwWs=; b=W2K0WkSpEm7GmoeZHviI25q2yVb4QYCrswzgu9pA9VKJJkWIbpmq/R9pkzaHY7X07jI2Gh ENrbiQk2XGc7WFswDLYWrqoTfDUav4bTSQFRcs9MyUGmrj8qgDDpBtPgEky/FoAWs2W/Cz n0A/LJrWK9nWyu4zdsq/V+/Q6fSq09g= Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-643-CNT9YDqkPUa7i5IG4DZGqA-1; Mon, 30 Mar 2026 15:38:48 -0400 X-MC-Unique: CNT9YDqkPUa7i5IG4DZGqA-1 X-Mimecast-MFC-AGG-ID: CNT9YDqkPUa7i5IG4DZGqA_1774899528 Received: by mail-qt1-f197.google.com with SMTP id d75a77b69052e-50b4fe4ff7bso198449411cf.0 for ; Mon, 30 Mar 2026 12:38:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1774899528; x=1775504328; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=b0GrCWjFJdwivrglfaRGY9APiPoMwEIvIsexr/luwWs=; b=AjJvkaJxg98oyE8c/jFvPbtu9uHC35efOvkOd/iN4SifCvF13lQGEpJIR6mIPYcwOU xNZON9mitrvXpQ1qA+tLzOnoG2p3j16gdxx251tzKmJYncdoW4DlzreE+xYFv7gewO4a 6Uw/Xi82pxW/nGZPDKQ0vsUHYeekFP9/wM/ePKVadJTX88f3ixMzWZ6CxCGZ0kvq/Aa6 8LNec1ZxlWDx3Kk9JwZlUlcWhB5av6pQKzYkZAKzI4evGsWgfm8m/PicOygkquuT431o 6NppxtSVgpn6hTtEIjZWx6ANRn0xt3kjIWcDPB+9vHIWZaxhM80SYuM63s0l7Vy4pX8w VF2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774899528; x=1775504328; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=b0GrCWjFJdwivrglfaRGY9APiPoMwEIvIsexr/luwWs=; b=mIyiWyaqeGL4hJ2BBrWuIjmsV1oXlw2ItMCQr8ssSQ9yJsGIJHzz60uY+r4dXUj4S3 EijV6XA08lVFevtNwEQojpNDQb3nXlkx8RwF9OQPa2P3c7anBDngOyrgq/7DhB3w+ji6 Ns01KVVdcOec3co57HMJon0oGTJNrAykH+go54a2hQp7H4pmpBryPD38CAIqGrpL1+fL QFuVdJ/L8sm1tlBbt+xXdoF/3SQmisZsoa3je6kRlvtQj44TY803cRViA/nxx/o9+iyE D+d4xKjVI5XiatbEDNll+YyT29kEFL42YcVT4sWftFjjnptEAyPf4djwOv0xy8oXZ/6Y N9xQ== X-Forwarded-Encrypted: i=1; AJvYcCXc3SB1rv92XtvzP+/tM2Qa13mgOkq64bkuYyZTsrh+6lDNCjGLgXKGmJ3Kk3v1NPe2LyPXWJFgWtS/nIc=@vger.kernel.org X-Gm-Message-State: AOJu0YwyrdbD/F71NPtgyIjk9tMk0JAVCH+jRtQ42INyxMPoZoH76omK 31YwyNx3w0pacOv7xeg++lJWWsurOgI46EOXAF3qnvGiZd/TDKel1ex+qwpbamAS0ZxGANS886z jB3U6zkjuZmN92E7vdglPOzzWzXpQZnZy6EFUtFmSA/EuwtEKgr304WG1Oh+0Fhd/eA== X-Gm-Gg: ATEYQzwvqlAJsCqZWtlSW32iBpPys3B6swdj/cNrD3kvFMiFJ4BHuxHxy1AWpgvRqby f95L34VX4IbERkGWJinBZY7zlnGCYirFBGL86ACJq98/iAAZcrmOi6Ld33+zW9WFGLcYfBvuOJF SBe/0neSO8oy4cAl9JEA9QJxBZ74d2/Z8nSafiDFdM05egqJboM++9ONsMpCuUTCX9E8iKeF7ke Deu4Sm6WbRZsNQfAaS+rCMCxFr9tqKaSu0F8FW2MgO7ieEBLehmBn/SsXZKuPLvUUPeGDRgMgb7 bl3AeNjymb4J/WVf8hgD5HKDo4ohg8pQ0qqu3RaBmIK0HkkPOWbaAYBLMoHIGU0YmtqAxWAAMSC 2o46I8s0m4HCLFg== X-Received: by 2002:a05:622a:618c:b0:509:1523:8b07 with SMTP id d75a77b69052e-50d2c8d6329mr10386441cf.25.1774899527998; Mon, 30 Mar 2026 12:38:47 -0700 (PDT) X-Received: by 2002:a05:622a:618c:b0:509:1523:8b07 with SMTP id d75a77b69052e-50d2c8d6329mr10385981cf.25.1774899527444; Mon, 30 Mar 2026 12:38:47 -0700 (PDT) Received: from x1.local ([142.189.10.167]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-50bbf69fee9sm56760151cf.30.2026.03.30.12.38.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Mar 2026 12:38:46 -0700 (PDT) Date: Mon, 30 Mar 2026 15:38:45 -0400 From: Peter Xu To: David Carlier Cc: Andrew Morton , Mike Rapoport , linux-mm@kvack.org, linux-kernel@vger.kernel.org, James Houghton Subject: Re: [PATCH 1/2] mm/userfaultfd: fix stale ops and VMA type mismatch after copy retry Message-ID: References: <20260328170101.184163-1-devnexen@gmail.com> 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=utf-8 Content-Disposition: inline In-Reply-To: <20260328170101.184163-1-devnexen@gmail.com> Hi, David, On Sat, Mar 28, 2026 at 05:01:00PM +0000, David Carlier wrote: > In mfill_atomic_pte_copy(), ops is derived from the VMA once and passed > to __mfill_atomic_pte(). When the initial copy_from_user() fails under > pagefault_disable(), mfill_copy_folio_retry() drops all locks, performs > the copy with page faults enabled, then re-acquires locks via > mfill_get_vma(). During this window, the VMA can be replaced entirely > (e.g. munmap + mmap + UFFDIO_REGISTER by another thread), but ops is > never re-validated. Thanks for the report, this seems a bug indeed. > > If a shared shmem VMA is replaced by an anonymous VMA, the stale > shmem_uffd_ops->filemap_add calls shmem_mfill_filemap_add() with an > anonymous VMA, causing a NULL pointer dereference at file_inode(vma-> > vm_file) since vm_file is NULL for anonymous mappings. > > The mmap_changing guard does not fully prevent this because > userfaultfd_unmap_prep() only increments mmap_changing when > UFFD_FEATURE_EVENT_UNMAP is enabled, which is optional. Without it, > munmap proceeds without any signal to the retry path. > > The copy_from_user() in the retry runs with page faults enabled and can > block on slow backing stores (FUSE, NFS), significantly widening the > race window. > > Fix this by: > - Validating that the VMA's userfaultfd context matches state->ctx in > mfill_get_vma() to detect cross-context VMA replacement. > - Re-checking that vma_uffd_ops() still matches the frozen ops after > the retry, and that the VMA is still VM_SHARED when ops expects it > to be, returning -EAGAIN otherwise. > > Signed-off-by: David Carlier > --- > mm/userfaultfd.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > index 481ec7eb4442..2a6e034b15aa 100644 > --- a/mm/userfaultfd.c > +++ b/mm/userfaultfd.c > @@ -225,8 +225,9 @@ static int mfill_get_vma(struct mfill_state *state) > */ > down_read(&ctx->map_changing_lock); > state->vma = dst_vma; > + > err = -EAGAIN; > - if (atomic_read(&ctx->mmap_changing)) > + if (dst_vma->vm_userfaultfd_ctx.ctx != ctx || atomic_read(&ctx->mmap_changing)) This is a valid and good check. Though IMHO it doesn't need to be in the same patch as a fix to the bug reported, likely it suites for a separate patch. For example, a new VMA (as you described in the case of when the bug can happen) can also be registered to the same userfaultfd ctx, and this check won't help there since the ctx will still match. > goto out_unlock; > > err = -EINVAL; > @@ -498,6 +499,12 @@ static int __mfill_atomic_pte(struct mfill_state *state, > ret = mfill_copy_folio_retry(state, folio); > if (ret) > goto err_folio_put; > + if (vma_uffd_ops(state->vma) != ops || > + (ops != &anon_uffd_ops && > + !(state->vma->vm_flags & VM_SHARED))) { Hard-code this (out of mfill_atomic_pte_copy) is very likely not a good idea.. Meanwhile I also feel like it won't completely fix the problem. Consider the changed VMA is not about shmem VMA becoming anon VMA, but shmem VMA1 becoming shmem VMA2 and both of them can even have the same flags. IMHO in that case we should still fallback with EAGAIN because we still hold a folio that we got allocated from VMA1 (rather than VMA2) here. Ideally, we should check "if the VMA has changed at all", which will be a very safe check, however I don't think we have any good way to refcount VMA... at least not something I'm aware of. A simple workaround here is we take a snapshot memory of the VMA attributes and making sure that didn't change after the retried copy_from_user(), assuming that guarantees the VMA didn't change. AFAIU, the important bit is at least two things: (1) the inode representing the backing store of the VMA mapping (when it's a file), and (2) vma flags. This patch below will do something like what I discussed above; that's the best I can come up with so far. Not sure if there's even better way to do. Thanks, ===8<=== diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index 481ec7eb44420..caac1afabd520 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -443,33 +443,90 @@ static int mfill_copy_folio_locked(struct folio *folio, unsigned long src_addr) return ret; } +struct vma_snapshot { + struct inode *inode; + vma_flags_t flags; +}; + +static void vma_snapshot_take(struct vm_area_struct *vma, + struct vma_snapshot *s) +{ + struct inode *inode; + + memcpy(&s->flags, &vma->flags, sizeof(s->flags)); + if (vma->vm_file) { + /* We're holding vma lock, so file and inode are available */ + inode = vma->vm_file->f_inode; + ihold(inode); + s->inode = inode; + } else { + s->inode = NULL; + } +} + +static bool vma_snapshot_changed(struct vm_area_struct *vma, + struct vma_snapshot *s) +{ + /* If vma flags changed? */ + if (memcmp(&s->flags, &vma->flags, sizeof(s->flags))) + return true; + + /* If there's a backing store of mapping, make sure it didn't change */ + if (s->inode && vma->vm_file->f_inode != s->inode) + return true; + + /* If not, making sure it's still anonymous */ + if (!s->inode && !vma_is_anonymous(vma)) + return true; + + return false; +} + +static void vma_snapshot_release(struct vma_snapshot *s) +{ + if (s->inode) { + iput(s->inode); + s->inode = NULL; + } +} + static int mfill_copy_folio_retry(struct mfill_state *state, struct folio *folio) { unsigned long src_addr = state->src_addr; + struct vma_snapshot s; void *kaddr; int err; + /* Take a quick snapshot of the current vma */ + vma_snapshot_take(state->vma, &s); + /* retry copying with mm_lock dropped */ mfill_put_vma(state); kaddr = kmap_local_folio(folio, 0); err = copy_from_user(kaddr, (const void __user *) src_addr, PAGE_SIZE); kunmap_local(kaddr); - if (unlikely(err)) - return -EFAULT; + if (unlikely(err)) { + err = -EFAULT; + goto out; + } flush_dcache_folio(folio); /* reget VMA and PMD, they could change underneath us */ err = mfill_get_vma(state); if (err) - return err; + goto out; - err = mfill_establish_pmd(state); - if (err) - return err; + if (vma_snapshot_changed(state->vma, &s)) { + err = -EAGAIN; + goto out; + } - return 0; + err = mfill_establish_pmd(state); +out: + vma_snapshot_release(&s); + return err; } static int __mfill_atomic_pte(struct mfill_state *state, -- 2.50.1 -- Peter Xu