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

On 4/8/26 11:50 AM, tejas bharambe 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?
> 
It seems theoretically possible.
Since we only want to trace ip_blkno here, not inode itself, we can
simply save it at first.

Thanks,
Joseph

> 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.