overlayfs: WARN_ONCE(Can't encode file handler for inotify: 255)

overlayfs: WARN_ONCE(Can't encode file handler for inotify: 255)

[Dmitry Safonov]

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?

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?

As a rare visitor to fs code, not sure I described the problem or
understood it well enough.
FWIW, reverting commit 16aac5ad1fa9 ("ovl: support encoding
non-decodable file handles") seems to "fix" the warning locally (not
considering it a long-term fix).

Thanks,
            Dmitry

Re: overlayfs: WARN_ONCE(Can't encode file handler for inotify: 255)

[Amir Goldstein]

On Wed, Dec 18, 2024 at 1:23 AM Dmitry Safonov <dima@arista.com> 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.

Re: overlayfs: WARN_ONCE(Can't encode file handler for inotify: 255)

[Amir Goldstein]

On Wed, Dec 18, 2024 at 1:10 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Dec 18, 2024 at 1:23 AM Dmitry Safonov <dima@arista.com> 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?
>
...

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

As predicted, I added a test case for FAN_DELETE_SELF over overlayfs
and it fails
to get the file handle of the deleted inode:

https://github.com/amir73il/ltp/commits/ovl_encode_fid/

fanotify13.c:174: TINFO: Test #6.4: FAN_REPORT_FID of delete events
with mark type FAN_MARK_INODE
[ 2967.311260] fanotify_encode_fh: 23 callbacks suppressed
[ 2967.311276] fanotify: failed to encode fid (type=0, len=0, err=-2)
[ 2967.317410] fanotify: failed to encode fid (type=0, len=0, err=-2)
[ 2967.320933] fanotify: failed to encode fid (type=0, len=0, err=-2)
fanotify13.c:268: TFAIL: handle_bytes (0) returned in event does not
equal to handle_bytes (24) returned in name_to_handle_at(2)
fanotify13.c:268: TFAIL: handle_bytes (0) returned in event does not
equal to handle_bytes (24) returned in name_to_handle_at(2)
fanotify13.c:268: TFAIL: handle_bytes (180003) returned in event does
not equal to handle_bytes (24) returned in name_to_handle_at(2)

Note that this is not a regression, because FAN_REPORT_FID was not supported on
overlayfs before 16aac5ad1fa9 ("ovl: support encoding non-decodable
file handles"),
so I do plan to fix ovl_dentry_to_fid(), but with the holidays coming
up, it could take
more time than usual.

If you have an urgency to fix the reported WARN_ONCE(), then do feel free
to post a patch to remove this assertion, because my fix to ovl_dentry_to_fid()
may be simplified to deal only with unlinked inodes, so it may not be enough
fix the use case that you reported.

Thanks,
Amir.

Re: overlayfs: WARN_ONCE(Can't encode file handler for inotify: 255)

[Dmitry Safonov]

Thanks Amir for all the detailed information,

On Wed, Dec 18, 2024 at 4:26 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Dec 18, 2024 at 1:10 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Dec 18, 2024 at 1:23 AM Dmitry Safonov <dima@arista.com> wrote:
[..]
> > 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.
>
> As predicted, I added a test case for FAN_DELETE_SELF over overlayfs
> and it fails
> to get the file handle of the deleted inode:
>
> https://github.com/amir73il/ltp/commits/ovl_encode_fid/
>
> fanotify13.c:174: TINFO: Test #6.4: FAN_REPORT_FID of delete events
> with mark type FAN_MARK_INODE
> [ 2967.311260] fanotify_encode_fh: 23 callbacks suppressed
> [ 2967.311276] fanotify: failed to encode fid (type=0, len=0, err=-2)
> [ 2967.317410] fanotify: failed to encode fid (type=0, len=0, err=-2)
> [ 2967.320933] fanotify: failed to encode fid (type=0, len=0, err=-2)
> fanotify13.c:268: TFAIL: handle_bytes (0) returned in event does not
> equal to handle_bytes (24) returned in name_to_handle_at(2)
> fanotify13.c:268: TFAIL: handle_bytes (0) returned in event does not
> equal to handle_bytes (24) returned in name_to_handle_at(2)
> fanotify13.c:268: TFAIL: handle_bytes (180003) returned in event does
> not equal to handle_bytes (24) returned in name_to_handle_at(2)
>
> Note that this is not a regression, because FAN_REPORT_FID was not supported on
> overlayfs before 16aac5ad1fa9 ("ovl: support encoding non-decodable
> file handles"),
> so I do plan to fix ovl_dentry_to_fid(), but with the holidays coming
> up, it could take
> more time than usual.

This sounds good to me, there is no urgency in fixing it for us, we're
going to keep the revert in the kernel until there is a fix.

> If you have an urgency to fix the reported WARN_ONCE(), then do feel free
> to post a patch to remove this assertion, because my fix to ovl_dentry_to_fid()
> may be simplified to deal only with unlinked inodes, so it may not be enough
> fix the use case that you reported.

I mostly wanted to make sure this issue is a known one, as WARN*()s
are considered actual issues with the code, rather than just warnings
to the user (and they leak kernel addresses). So, probably it's worth
fixing somewhere during the release. Also the report could save
someone elses time, who may observe the same issue.

Thanks again Amir,
            Dmitry