[PATCH v2] ovl: drop WARN_ON() dentry is NULL in ovl_encode_fh()

[PATCH v2] ovl: drop WARN_ON() dentry is NULL in ovl_encode_fh()

[Jiachen Zhang]

Some code paths cannot guarantee the inode have any dentry alias. So
WARN_ON() all !dentry may flood the kernel logs.

For example, when an overlayfs inode is watched by inotifywait (1), and
someone is trying to read the /proc/$(pidof inotifywait)/fdinfo/INOTIFY_FD,
at that time if the dentry has been reclaimed by kernel (such as
echo 2 > /proc/sys/vm/drop_caches), there will be a WARN_ON(). The
printed call stack would be like:

    ? show_mark_fhandle+0xf0/0xf0
    show_mark_fhandle+0x4a/0xf0
    ? show_mark_fhandle+0xf0/0xf0
    ? seq_vprintf+0x30/0x50
    ? seq_printf+0x53/0x70
    ? show_mark_fhandle+0xf0/0xf0
    inotify_fdinfo+0x70/0x90
    show_fdinfo.isra.4+0x53/0x70
    seq_show+0x130/0x170
    seq_read+0x153/0x440
    vfs_read+0x94/0x150
    ksys_read+0x5f/0xe0
    do_syscall_64+0x59/0x1e0
    entry_SYSCALL_64_after_hwframe+0x44/0xa9

So let's drop WARN_ON() to avoid kernel log flooding.

Reported-by: Hongbo Yin <yinhongbo@bytedance.com>
Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>
Signed-off-by: Tianci Zhang <zhangtianci.1997@bytedance.com>
---
 fs/overlayfs/export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 2eada97bbd23..e065a5b9a442 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -259,7 +259,7 @@ static int ovl_encode_fh(struct inode *inode, u32 *fid, int *max_len,
 		return FILEID_INVALID;
 
 	dentry = d_find_any_alias(inode);
-	if (WARN_ON(!dentry))
+	if (!dentry)
 		return FILEID_INVALID;
 
 	bytes = ovl_dentry_to_fid(ofs, dentry, fid, buflen);
-- 
2.20.1

Re: [PATCH v2] ovl: drop WARN_ON() dentry is NULL in ovl_encode_fh()

[Miklos Szeredi]

On Thu, 28 Jul 2022 at 13:49, Jiachen Zhang
<zhangjiachen.jaycee@bytedance.com> wrote:
>
> Some code paths cannot guarantee the inode have any dentry alias. So
> WARN_ON() all !dentry may flood the kernel logs.
>
> For example, when an overlayfs inode is watched by inotifywait (1), and
> someone is trying to read the /proc/$(pidof inotifywait)/fdinfo/INOTIFY_FD,
> at that time if the dentry has been reclaimed by kernel (such as
> echo 2 > /proc/sys/vm/drop_caches), there will be a WARN_ON(). The
> printed call stack would be like:
>
>     ? show_mark_fhandle+0xf0/0xf0
>     show_mark_fhandle+0x4a/0xf0
>     ? show_mark_fhandle+0xf0/0xf0
>     ? seq_vprintf+0x30/0x50
>     ? seq_printf+0x53/0x70
>     ? show_mark_fhandle+0xf0/0xf0
>     inotify_fdinfo+0x70/0x90
>     show_fdinfo.isra.4+0x53/0x70
>     seq_show+0x130/0x170
>     seq_read+0x153/0x440
>     vfs_read+0x94/0x150
>     ksys_read+0x5f/0xe0
>     do_syscall_64+0x59/0x1e0
>     entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> So let's drop WARN_ON() to avoid kernel log flooding.


Applied, thanks.

Miklos

Re: [PATCH v2] ovl: drop WARN_ON() dentry is NULL in ovl_encode_fh()

[Amir Goldstein]

On Thu, Jul 28, 2022 at 3:06 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 28 Jul 2022 at 13:49, Jiachen Zhang
> <zhangjiachen.jaycee@bytedance.com> wrote:
> >
> > Some code paths cannot guarantee the inode have any dentry alias. So
> > WARN_ON() all !dentry may flood the kernel logs.
> >
> > For example, when an overlayfs inode is watched by inotifywait (1), and
> > someone is trying to read the /proc/$(pidof inotifywait)/fdinfo/INOTIFY_FD,
> > at that time if the dentry has been reclaimed by kernel (such as
> > echo 2 > /proc/sys/vm/drop_caches), there will be a WARN_ON(). The
> > printed call stack would be like:
> >
> >     ? show_mark_fhandle+0xf0/0xf0
> >     show_mark_fhandle+0x4a/0xf0
> >     ? show_mark_fhandle+0xf0/0xf0
> >     ? seq_vprintf+0x30/0x50
> >     ? seq_printf+0x53/0x70
> >     ? show_mark_fhandle+0xf0/0xf0
> >     inotify_fdinfo+0x70/0x90
> >     show_fdinfo.isra.4+0x53/0x70
> >     seq_show+0x130/0x170
> >     seq_read+0x153/0x440
> >     vfs_read+0x94/0x150
> >     ksys_read+0x5f/0xe0
> >     do_syscall_64+0x59/0x1e0
> >     entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >
> > So let's drop WARN_ON() to avoid kernel log flooding.
>
>
> Applied, thanks.

FWIW, no objection to this fix, but for the record, encode_fh
is basically an inode operation, so it shouldn't require an alias.

The only thing in the call chain down from ovl_encode_fh()
that needs the ovl dentry is ovl_connect_layer(). The rest of the
referenced to ovl dentry can use an ovl inode instead.

In some cases (e.g. non-dir or pure upper dir), ovl_connect_layer()
will not be called at all, but even if it would need to be called, it is
better to skip it and encode the lower inode if there is no ovl dentry
available.

The possible eventual outcome of an fh changing after disconnected
dir copy up is probably better than failing encode_fh out right.

No need to make any of those changes for this corner case IMO.
Just wanted to add this analysis to the thread.

Thanks,
Amir.