Thanks Amir for all the detailed information,
On Wed, Dec 18, 2024 at 4:26 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Wed, Dec 18, 2024 at 1:10 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Wed, Dec 18, 2024 at 1:23 AM Dmitry Safonov <dima@xxxxxxxxxx> 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
