linux-ext4 - Re: [PATCH] ext4: lost matching-pair of trace in ext4

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] [day] [month] [year] [list]

Message-Id: <100913BE-A164-4904-B2F0-C8558B5A1397@dilger.ca>
Date:   Sun, 28 Jun 2020 16:05:34 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Yi Zhuang <zhuangyi1@...wei.com>
Cc:     "Theodore Y. Ts'o" <tytso@....edu>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: lost matching-pair of trace in ext4_unlink

On Jun 27, 2020, at 9:48 PM, Yi Zhuang <zhuangyi1@...wei.com> 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 <zhuangyi1@...wei.com>

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,
                                    EXT4_DATA_TRANS_BLOCKS(dir->i_sb));
        if (IS_ERR(handle)) {
                retval = PTR_ERR(handle);
                goto out_bh;
        }
	:
	:

out_handle:
	ext4_journal_stop(handle);
out_bh:
	brelse(bh);
out_trace:
        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;
> }
> --
> 2.26.0.106.g9fadedd
> 


Cheers, Andreas






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