On Wed, Dec 18, 2024 at 1:23 AM Dmitry Safonov <dima@xxxxxxxxxx> wrote:
>
> Hi Amir and Miklos, linux-unionfs,
>
> On v6.9.0 kernel we stepped over the WARN_ON() in show_mark_fhandle():
>
> > ------------[ cut here ]------------
> > Can't encode file handler for inotify: 255
> > WARNING: CPU: 0 PID: 11136 at fs/notify/fdinfo.c:55 show_mark_fhandle+0xfa/0x110
> > CPU: 0 PID: 11136 Comm: lsof Kdump: loaded Tainted: P W O 6.9.0 #1
> > RIP: 0010:show_mark_fhandle+0xfa/0x110
> > Code: 00 00 00 5b 41 5c 5d e9 44 21 97 00 80 3d 0d af 99 01 00 75 d8 89 ce 48 c7 c7 68 ad 4a 82 c6 05 fb ae 99 01 01 e8 f6 98 cc ff <0f> 0b eb bf e8 4d 29 96 00 66 66 2e 0f 1f 84 00 00 00 00 00 66 90
> ...
> > Call Trace:
> > <TASK>
> > inotify_show_fdinfo+0x124/0x170
> > seq_show+0x188/0x1f0
> > seq_read_iter+0x115/0x4a0
> > seq_read+0xf9/0x130
> > vfs_read+0xb6/0x330
> > ksys_read+0x6b/0xf0
> > __do_fast_syscall_32+0x80/0x110
> > do_fast_syscall_32+0x37/0x80
> > entry_SYSCALL_compat_after_hwframe+0x75/0x75
> > RIP: 0023:0xf7f93569
>
> it later reproduced on v6.12.0. With some debug, it was narrowed down
> to the way overlayfs encodes file handlers in ovl_encode_fh(). It
> seems that currently it calculates them with the help of dentries.
> Straight away from that, the reproducer becomes an easy drop_caches +
> lsof (which parses procfs and finds some pid(s) that utilize inotify,
> reading their correspondent fdinfo(s)).
>
> So, my questions are: is a dentry actually needed for
> ovl_dentry_to_fid()? Can't it just encode fh based on an inode? It
> seems that the only reason it "needs" a dentry is to find the origin
> layer in ovl_check_encode_origin(), is it so?
Well, the answer depends on the overlayfs export operations or on:
bool decodable = ofs->config.nfs_export;
For decodable directory file handles, a dentry is surely needed for
ovl_connect_layer(), but this is not the common case, so the short answer is
Yes, ovl_dentry_to_fid() could encode fh based on the inode, but it
will need some helpers refactoring as you can see and the question is
"Is it worth the effort?"
>
> I guess, the potential solution here would be either to populate the
> dentry back (likely racy and ugh) or just encode file handles based on
> the lower-inode? IIUC, old file handles will become stale anyway after
> dropping the caches?
They will not become stale.
file handles are usually persistent unless the underlying fs is volatile
by nature like tmpfs.
>
> As a rare visitor to fs code, not sure I described the problem or
> understood it well enough.
You understood the problem and explained it perfectly, only
we also need to ask why is show_mark_fhandle() needed and
what is the assertion for?
Because there is one more simple solution -
Remove the WARN_ONCE assertion from show_mark_fhandle().
AFAIK, show_mark_fhandle() was originally added so that CRIU
could restore inotify marks after resume, but if file handles are not decodable
(i.e. !exportfs_can_encode_fh()), then are useless to userspace, so perhaps
the assertion is not needed?
IOW, I am not so worried about show_mark_fhandle().
However, I am concerned about the possibility of exportfs_encode_fid()
failing in fanotify_encode_fh().
Most fsnotify events are generated with a reference on the dentry, but
fsnotify_inoderemove() is called from dentry_unlink_inode() after removing
the dentry from the inode aliases list, so does that mean that FAN_DELETE_SELF
events from overlayfs are never reported with fid info and that we will
always print pr_warn_ratelimited("fanotify: failed to encode fid ("...?
I see that the LTP test to cover overlayfs fid events reporting (fanotify13)
does not cover FAN_DELETE_SELF events, so I need to go check.
Thanks,
Amir.
