On Tue, Nov 19, 2024 at 3:33 PM Vasiliy Kovalev <kovalev@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> 19.11.2024 12:05, Miklos Szeredi wrote:
> > On Mon, 18 Nov 2024 at 19:54, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> >> Can you analyse what went wrong with the reproducer?
> >> How did we get to a state where lowerstack of parent
> >> has a dentry which is !d_can_lookup?
> >
> > Theoretically we could still get a an S_ISDIR inode, because
> > ovl_get_inode() doesn't look at the is_dir value that lookup found.
> > I.e. lookup thinks it found a non-dir, but iget will create a dir
> > because of the backing inode's type.
> >
> > AFAICS this can only happen if i_op->lookup is not set on S_ISDIR for
> > the backing inode, which shouldn't happen on normal filesystems.
> > Reproducer seems to use bfs, which *should* be normal, and bfs_iget
> > certainly doesn't do anything weird in that case, so I still don't
> > understand what is happening.
>
> During testing, it was discovered that BFS can return a directory inode
> without a lookup operation. Adding the following check in bfs_iget:
>
> struct inode *bfs_iget(struct super_block *sb, unsigned long ino)
> {
>
> ...
> brelse(bh);
>
> + if (S_ISDIR(inode->i_mode) && !inode->i_op->lookup) {
> + printf("Directory inode missing lookup %s:%08lx\n",
> inode->i_sb->s_id, ino);
> + goto error;
> + }
> +
> unlock_new_inode(inode);
> return inode;
>
> error:
> iget_failed(inode);
> return ERR_PTR(-EIO);
> }
>
> prevents the error but exposes an invalid inode:
>
> loop0: detected capacity change from 0 to 64
> BFS-fs: bfs_iget(): Directory inode missing lookup loop0:00000002
> overlayfs: overlapping lowerdir path
>
> Would this be considered a valid workaround, or does BFS require further
> fixes?
>
> > In any case something like the following should filter out such weirdness:
> >
> > bool ovl_dentry_weird(struct dentry *dentry)
> > {
> > + if (!d_can_lookup(dentry) && !d_is_file(dentry) &&
> > !d_is_symlink(dentry))
> > + return true;
> > +
> > return dentry->d_flags & (DCACHE_NEED_AUTOMOUNT |
>
> I tested this addition successfully.
>
> Additionally, fixes for BFS seem to be discussed reluctantly.
> For instance, this patch set [1] has remained unanswered.
> Perhaps it would be worth considering discarding invalid inodes at the
> overlayfs level?
Sure. please send proper patch following Miklos' suggestion
Thanks,
Amir.
