Message-ID: <of6vjajyda3355hl7vskbjjb67wrqlqfv4hn5i2vxihn2qspau@3dqu5dolu3z5>
Date: Thu, 2 Oct 2025 18:08:51 +0200
From: Jan Kara <jack@...e.cz>
To: Deepanshu Kartikey <kartikey406@...il.com>
Cc: jack@...e.cz, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] isofs: fix inode leak caused by disconnected dentries
 from exportfs

Hello,

On Thu 02-10-25 01:57:13, Deepanshu Kartikey wrote:
> Thank you for the review. You're absolutely right - my initial
> explanation was incorrect. I've done extensive debugging to understand
> the actual mechanism causing the leak.

Hrm, the text below looks like it was generated by some AI agent :) More
importantly it still doesn't really explain the underlying cause of the
leak - it is fine to use AI for debugging but then one should get his own
understanding of the situation. As I was curious about the root cause, I
did the debugging myself and [1] has the proper analysis of the problem and
the fix.

								Honza

[1] https://lore.kernel.org/all/20251002155506.10755-2-jack@suse.cz/

> 
> Root Cause Analysis
> ===================
> 
> The leak occurs specifically with CONFIG_JOLIET=y through the following sequence:
> 
> 1. Joliet Root Switching During Mount
> --------------------------------------
> 
> In isofs_fill_super(), when Joliet extensions are detected:
> - Primary root inode 1792 is created with i_count=1, i_nlink=3
> - During Joliet switching, iput(inode) is called on inode 1792
> - i_count decrements to 0, but generic_drop_inode() returns false (i_nlink=3 > 0)
> - Inode 1792 remains cached at i_count=0
> - New Joliet root inode 1920 is created and attached to sb->s_root
> 
> Debugging output:
>   [9.653617] isofs: switching roots, about to iput ino=1792, i_count=1
>   [9.653676] isofs: after iput, getting new root
>   [9.653880] isofs: old inode after iput ino=1792, i_count=0, i_nlink=3
>   [9.654219] isofs: got new root ino=1920, i_count=1
> 
> 2. open_by_handle_at() Triggers Reconnection
> ---------------------------------------------
> 
> When the system call attempts to resolve a file handle:
> - exportfs_decode_fh_raw() calls fh_to_dentry() which returns inode 1856
> - The dentry is marked DCACHE_DISCONNECTED
> - reconnect_path() is invoked to connect the path to root
> - This calls reconnect_one() to walk up the directory tree
> 
> 3. Reference Accumulation in reconnect_one()
> ---------------------------------------------
> 
> I instrumented reconnect_one() to track dentry reference counts:
> 
>   [8.010398] reconnect_one: called for inode 1856
>   [8.010735] isofs: __isofs_iget got inode 1792, i_count=1
>   [8.011041] After fh_to_parent: d_count=1
>   [8.011319] After exportfs_get_name: d_count=2
>   [8.011769] After lookup_one_unlocked: d_count=3
> 
> The parent dentry (inode 1792) accumulates 3 references:
> 1. Initial reference from fh_to_parent()
> 2. Additional reference taken by exportfs_get_name()
> 3. Another reference taken by lookup_one_unlocked()
> 
> Then lookup_one_unlocked() creates a dentry for inode 1807:
>   [8.015179] isofs: __isofs_iget got inode 1807, i_count=1
>   [8.016169] lookup returns tmp (inode 1807), d_count=1
> 
> The code enters the tmp != dentry branch and calls dput(tmp), then goes to 
> out_reconnected.
> 
> 4. Insufficient Cleanup
> -----------------------
> 
> For inode 1807, I traced through dput():
>   [10.083359] fast_dput: lockref_put_return returned 0
>   [10.083699] fast_dput: RETAINING dentry for inode 1807, d_flags=0x240043
> 
> The dentry refcount goes to 0, but retain_dentry() returns true because of 
> the DCACHE_REFERENCED flag (0x40 in 0x240043). The dentry is kept in cache 
> with refcount 0, still holding the inode reference.
> 
> For inode 1792:
>   [10.084125] fast_dput: lockref_put_return returned 2
> 
> At out_reconnected, only one dput(parent) is called, decrementing from 3 to 2. 
> Two references remain leaked.
> 
> 5. Unmount Failure
> ------------------
> 
> At unmount time:
> - shrink_dcache_for_umount() doesn't evict dentries with positive refcounts (1792)
> - Doesn't aggressively evict retained dentries with refcount 0 (1807)
> - Both inodes appear as leaked with i_count=1
> 
>   [10.155385] LEAKED INODE: ino=1807, i_count=1, i_state=0x0, i_nlink=1
>   [10.155604] LEAKED INODE: ino=1792, i_count=1, i_state=0x0, i_nlink=1
> 
> Why shrink_dcache_sb() Works
> =============================
> 
> Calling shrink_dcache_sb() in isofs_put_super() forces eviction of:
> - Dentries with extra references that weren't properly released
> - Retained dentries sitting in cache at refcount 0
> 
> This ensures cleanup happens before the superblock is destroyed.
> 
> Open Questions
> ==============
> 
> 1. Are exportfs_get_name() and lookup_one_unlocked() supposed to take 
>    references to the parent dentry that the caller must release? Should 
>    reconnect_one() be calling dput(parent) multiple times, or are these 
>    functions leaking references?
> 
> 2. Is adding shrink_dcache_sb() in put_super() the appropriate fix, or 
>    should this be handled in the reconnection error path when 
>    reconnect_one() fails?
> 
> 3. Does this indicate a broader issue with how exportfs handles parent 
>    dentry references during failed path reconnections that might affect 
>    other filesystems?
> 
> I can investigate further into the implementation of exportfs_get_name() 
> and lookup_one_unlocked() to understand where exactly the extra references 
> are taken if that would be helpful.
> 
> Best regards,
> Deepanshu Kartikey
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists