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

I see the concern on using ihold/iput on performance. I find Jospeh's
suggestion valuable. Let me send in a new patch, I will just save
ip_blkno as a plain integer, that should be zero overhead


On Wed, Apr 8, 2026 at 1:12 PM Andrew Morton <[email protected]> wrote:
>
> 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.