Re: [PATCH v4] ocfs2: fix use-after-free in ocfs2_fault() when VM_FAULT_RETRY

On Wed, 8 Apr 2026 03:50:17 +0000 tejas bharambe <[email protected]> wrote:

> Hi Andrew,
> 
> You're right, I missed that scenario.
> 
> The inode can be freed if the file descriptor is closed after mmap() and munmap() races with the fault handler.
> 
> I can do one of the following:
> 1. I can skip the trace firing when VM_FAULT_RETRY is set as I did in v1. It was changed to v4 after Joseph's suggestion to keep traces.
> 2. If we want to keep traces, we can use ihold()/iput() as shown below:
> 
> ihold(inode);   //pin inode
> ret = filemap_fault(vmf);
> trace_ocfs2_fault(OCFS2_I(inode)->ip_blkno, ...);  // safe, refcount held
> iput(inode);  //release inode
> 
> 
> Which approach do you prefer?

Well, that's down to the ocfs2 maintiners.  Me, omitting traces doesn't
sound good.

But we should consider performance implications - this is a fairly hot
path and iget/iput are a little costly.  Perhaps there's a way to avoid
the iget/iput if tracing isn't enabled.  As long as we handle the case
where tracing get enabled immediately after we've done the

	if (tracing enabled)
		iget()


> Thanks,
> Tejas
> ________________________________________
> From: Andrew Morton <[email protected]>
> Sent: Saturday, April 4, 2026 5:50 PM
> To: tejas bharambe <[email protected]>
> Cc: Tejas Bharambe <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
> Subject: Re: [PATCH v4] ocfs2: fix use-after-free in ocfs2_fault() when VM_FAULT_RETRY
> 
> On Sun, 5 Apr 2026 00:30:14 +0000 tejas bharambe <[email protected]> wrote:
> 
> > Following is my response for question posted on https://sashiko.dev/#/patchset/20260403035333.136824-1-tejas.bharambe%40outlook.com
> >
> >
> > No. For ocfs2_fault() to be executing, the file must be open and
> > the process holds an active file descriptor. The inode's lifetime
> > is tied to the file's reference count, which remains held by the
> > file descriptor for the duration of the fault handler. munmap()
> > can free the VMA (decrementing vm_file's refcount) but cannot
> > free the inode as long as the file descriptor is open. The faulting
> > thread cannot call close() while it is inside the fault handler,
> > so the inode is guaranteed to outlive the trace call.
> 
> I don't think that's the scenario which Sashiko is suggesting.
> 
> Suppose userspace does
> 
>         fd = open(...);
>         p = mmap(fd, ...);
>         close(fd);
> 
> Now, that mmap is the only ref against fd.
> 
> Now, suppose that userspace does munmap() while another thread is in
> the fault handler.