All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <djwong@kernel.org>
To: Theodore Tso <tytso@mit.edu>
Cc: Daniel Tang <danielzgtg.opensource@gmail.com>,
	linux-ext4@vger.kernel.org
Subject: Re: [GIT PULL] e4defrag inline data segfault fix
Date: Fri, 16 Jan 2026 18:35:59 -0800	[thread overview]
Message-ID: <20260117023559.GC15522@frogsfrogsfrogs> (raw)
In-Reply-To: <20260116234405.GG19200@macsyma.local>

On Fri, Jan 16, 2026 at 01:44:05PM -1000, Theodore Tso wrote:
> On Fri, Jan 16, 2026 at 09:21:39AM -0800, Darrick J. Wong wrote:
> > On Fri, Jan 16, 2026 at 05:25:35AM -0500, Daniel Tang wrote:
> > > Please sign-off on, and apply, the patch at
> > > https://gist.github.com/tytso/609572aed4d3f789742a50356567e929 . It
> > > fixes the bug at https://github.com/tytso/e2fsprogs/issues/260 .
> > 
> > Perhaps that patch should get posted to the list for a proper review?
> 
> The context was that Daniel had proposed a pull request on github:
> 
>    https://github.com/tytso/e2fsprogs/pull/261
> 
> I had reviewed the change on github, and counter-proposed a better
> fix, which was what Daniel was referring to on the gist.github.com,
> and asked him to confirm that this fixed the issue that he was
> concerned about.
> 
> This took place in early Ddecember, and I lost track of it because of
> the holidays.  (Daniel, that's because my primary workflow is e-mail,
> and github issues and pull requests are things that I look at on a
> best-efforts basis, whereas with e-mail I have things like Patchwork
> to make sure I don't lose track of patch discussions.  It also means
> that other people can more easily review proposed fixes.)
> 
> Anyway, for folks on the ext4 list who might be curious, here's the
> fix.  As it turns out, this is one where the description of the fix
> takes a lot more space than the actual fix itself.  Which is why I
> hadn't bothered to write it all up before asking Daniel to test it to
> make sure it fixed the issue that he had run into.

<nod> bugfixes are fun like that :D

>       	    	       	    	       - Ted
> 
> commit 23785e90554b301b90076568fd33eb76dc930fba
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Fri Jan 16 18:01:09 2026 -0500
> 
>     e4defrag: don't try to defragment files which have 0 or 1 blocks
>     
>     This fixes a crash in e4defrag when the file is using the inline_data
>     feature, and so the file data is stored in the inode table.
>     Technically speaking, such a file does not consume any data blocks,
>     but when an application program calls stat(2) on such a file, and
>     st_blocks is set to 0, it might confuse the program into thinking the
>     file did not contain any data.  For this reason, ext4 returns 1 in
>     st_blocks.  (For other files or directories, st_blocks will be a
>     multiple of the file system block size divided by 512, since st_blocks
>     is denominated in units of 512 sectors on Linux --- and most other Unix
>     systems with the notable exception of HP-UX.)
>     
>     Unfortunately, when e4defrag tries to defragment a inline data file,
>     it divides st_blocks by (fs->block_size / 512), and this results in
>     e4defrag thinking that the file 0 data blocks --- and since the file
>     is not skipped because st_blocks != 0, this results in crash when
>     dividing by zero.
>     
>     As it turns out, it's pointless to try to defrag a file with 0 or 1
>     data blocks.  So fix this by skipping any file where st_blocks <=
>     block_size / 512.

Excellent description.  I glanced at the github issue this morning and
wondered what was going on with all the /512s...

>     Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> diff --git a/misc/e4defrag.c b/misc/e4defrag.c
> index bbeb5b167..68e937fdb 100644
> --- a/misc/e4defrag.c
> +++ b/misc/e4defrag.c
> @@ -1091,11 +1091,11 @@ static int file_statistic(const char *file, const struct stat64 *buf,
>  		return 0;
>  	}
>  
> -	/* Has no blocks */
> -	if (buf->st_blocks == 0) {
> +	/* Has 0 or 1 blocks, no point to defragment */
> +	if (buf->st_blocks <= buf->st_blksize / 512) {

...because can't you call FS_IOC_GETFLAGS and look for
EXT4_INLINE_DATA_FL?

--D

>  		if (mode_flag & DETAIL) {
>  			PRINT_FILE_NAME(file);
> -			STATISTIC_ERR_MSG("File has no blocks");
> +			STATISTIC_ERR_MSG("# of file blocks <= 1");
>  		}
>  		return 0;
>  	}
> @@ -1452,11 +1452,11 @@ static int file_defrag(const char *file, const struct stat64 *buf,
>  		return 0;
>  	}
>  
> -	/* Has no blocks */
> -	if (buf->st_blocks == 0) {
> +	/* Has 0 or 1 blocks, no point to defragment */
> +	if (buf->st_blocks <= buf->st_blksize / 512) {
>  		if (mode_flag & DETAIL) {
>  			PRINT_FILE_NAME(file);
> -			STATISTIC_ERR_MSG("File has no blocks");
> +			IN_FTW_PRINT_ERR_MSG("# of file blocks <= 1");
>  		}
>  		return 0;
>  	}
> 

  reply	other threads:[~2026-01-17  2:36 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-16 10:25 [GIT PULL] e4defrag inline data segfault fix Daniel Tang
2026-01-16 17:21 ` Darrick J. Wong
2026-01-16 23:44   ` Theodore Tso
2026-01-17  2:35     ` Darrick J. Wong [this message]
2026-01-17  3:47       ` Theodore Tso
2026-01-17  4:49         ` Darrick J. Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260117023559.GC15522@frogsfrogsfrogs \
    --to=djwong@kernel.org \
    --cc=danielzgtg.opensource@gmail.com \
    --cc=linux-ext4@vger.kernel.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.