linux-ext4 - Re: [PATCH] ext4: allow directory holes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190702200459.GF3032@mit.edu>
Date:   Tue, 2 Jul 2019 16:04:59 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Artem Blagodarenko <artem.blagodarenko@...il.com>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        stable@...nel.org
Subject: Re: [PATCH] ext4: allow directory holes

On Sun, Jun 23, 2019 at 09:52:15PM -0600, Andreas Dilger wrote:
> > @@ -179,13 +178,6 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
> > 		}
> > 
> > 		if (!bh) {
> > -			if (!dir_has_error) {
> > -				EXT4_ERROR_FILE(file, 0,
> > -						"directory contains a "
> > -						"hole at offset %llu",
> > -					   (unsigned long long) ctx->pos);
> > -				dir_has_error = 1;
> > -			}
> 
> > 			/* corrupt size?  Maybe no more blocks to read */
> > 			if (ctx->pos > inode->i_blocks << 9)
> > 				break;
> >                         ctx->pos += sb->s_blocksize - offset;
> 
> It seems that ext4_map_blocks() will return m_len with the length of the hole,
> so it would make sense to skip all of the blocks in the hole rather than trying
> to read all of them, in case the directory is mostly sparse.  This could avoid
> a bunch of kernel spinning.
> 
> Also, there is a separate question of whether ext4_map_blocks() will return 0
> in the case of a hole, according to the function comment:
> 
>  * It returns 0 if plain look up failed (blocks have not been allocated), in
>  * that case, @map is returned as unmapped but we still do fill map->m_len to
>  * indicate the length of a hole starting at map->m_lblk.
> 
> in which case "bh" is not reset from the previous loop?

Good catch!  This is a pre-existing bug which you've spotted, and
which we'll want to fix regardless of whether or not the largedir
patch is applied.  I suspect we'll probably need to manually apply
this patch to older kernels, but fortunately directory holes are rare,
and the worst that we will happen is we'll send some duplicate
directory entries to userspace.

> > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> > index 4909ced4e672..f3140ff330c6 100644
> > --- a/fs/ext4/namei.c
> > +++ b/fs/ext4/namei.c
> > @@ -83,7 +83,7 @@ static int ext4_dx_csum_verify(struct inode *inode,
> > 			       struct ext4_dir_entry *dirent);
> > 
> > typedef enum {
> > -	EITHER, INDEX, DIRENT
> > +	EITHER, INDEX, DIRENT, DIRENT_HTREE
> 
> It would be useful to put these one-per-line with a comment explaining each.

What I've done instead is to add a much longer comment explaining why
these directory block types are getting are getting passed to
ext4_read_dirblcok() in the first place.  A comment saying "this is
expected to be an index block" doesn't actually add that much value,
but you're absolutely right that we should have better documentation
here.

						- Ted