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  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Sun, 28 Jun 2020 16:05:34 -0600
From:   Andreas Dilger <>
To:     Yi Zhuang <>
Cc:     "Theodore Y. Ts'o" <>,
        Ext4 Developers List <>
Subject: Re: [PATCH] ext4: lost matching-pair of trace in ext4_unlink

On Jun 27, 2020, at 9:48 PM, Yi Zhuang <> wrote:
> If dquot_initialize() return non-zero and trace of ext4_unlink_enter/exit
> enabled then the matching-pair of trace_exit will lost in log.
> Signed-off-by: Yi Zhuang <>

Thank you for the patch.  It definitely looks like this is fixing the
problem with trace exit, but I would recommend to use a better name
than "out_unlink:" for the label.  I was looking at the ext4_unlink()
function, and found it confusing that there is already an "end_unlink:"
label and these two names would be easily confused.

Please change your new label to be "out_trace:", or similar, which
makes it more clear that it is undoing the "trace" part of the code.

It looks like there is another similar problem with the error handling
in this function:

	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
	if (IS_ERR(bh))
		return PTR_ERR(bh);
	if (!bh)
		goto end_unlink;

At this point the journal handle has not been started, and the PTR_ERR()
return is also missing the trace exit, so it would be better to change
these two cases to use the "out_trace:" label as well, like:

	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
	if (!bh) {
		retval = -ENOENT;
		goto out_trace;
	if (IS_ERR(bh)) {
		retval = PTR_ERR(bh);
		goto out_trace;

Could you please resubmit your patch with these small changes.

There could be a separate small patch to split up the "end_unlink:"
label to be two separate labels, and then remove the "if (handle)"
check, and then use out_bh: before the handle is started:

        if (le32_to_cpu(de->inode) != inode->i_ino) {
		retval = -EFSCORRUPTED;
                goto out_bh;

        handle = ext4_journal_start(dir, EXT4_HT_DIR,
        if (IS_ERR(handle)) {
                retval = PTR_ERR(handle);
                goto out_bh;

        trace_ext4_unlink_exit(dentry, retval);
        return retval;

Cheers, Andreas

> ---
> fs/ext4/namei.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 56738b538ddf..5e41d45915c9 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3193,10 +3193,10 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> 	 * in separate transaction */
> 	retval = dquot_initialize(dir);
> 	if (retval)
> -		return retval;
> +		goto out_unlink;
> 	retval = dquot_initialize(d_inode(dentry));
> 	if (retval)
> -		return retval;
> +		goto out_unlink;
> 	retval = -ENOENT;
> 	bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL);
> @@ -3255,6 +3255,7 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
> 	brelse(bh);
> 	if (handle)
> 		ext4_journal_stop(handle);
> +out_unlink:
> 	trace_ext4_unlink_exit(dentry, retval);
> 	return retval;
> }
> --

Cheers, Andreas

Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists