linux-kernel - Re: [fs] faf99b5635: will-it-scale.per_thread

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: <20220815100044.7j2u2yjlkanhkrfg@wittgenstein>
Date:   Mon, 15 Aug 2022 12:00:44 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Stefan Roesch <shr@...com>, Christoph Hellwig <hch@....de>,
        Jan Kara <jack@...e.cz>
Cc:     Stefan Roesch <shr@...com>, Jens Axboe <axboe@...nel.dk>,
        "Darrick J. Wong" <djwong@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org, lkp@...ts.01.org, lkp@...el.com,
        ying.huang@...el.com, feng.tang@...el.com,
        zhengjun.xing@...ux.intel.com, fengwei.yin@...el.com,
        kernel test robot <oliver.sang@...el.com>
Subject: Re: [fs]  faf99b5635:  will-it-scale.per_thread_ops -9.0% regression

On Mon, Aug 15, 2022 at 12:32:25PM +0800, kernel test robot wrote:
> 
> 
> Greeting,
> 
> FYI, we noticed a -9.0% regression of will-it-scale.per_thread_ops due to commit:
> 
> 
> commit: faf99b563558f74188b7ca34faae1c1da49a7261 ("fs: add __remove_file_privs() with flags parameter")
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master

This seems overall pretty odd tbh at least it's not immediately obvious
how that specific commit would've caused this. But fwiw, I think there's
one issue in this change which we originally overlooked which might
explain this.

Before faf99b563558 ("fs: add __remove_file_privs() with flags
parameter") inode_has_no_xattr() was called when
dentry_needs_remove_privs() returned 0.

	int error = 0
	[...]
	kill = dentry_needs_remove_privs(dentry);
	if (kill < 0)
		return kill;
	if (kill)
		error = __remove_privs(file_mnt_user_ns(file), dentry, kill);
	if (!error)
		inode_has_no_xattr(inode);

but now we do:

	kill = dentry_needs_remove_privs(dentry);
	if (kill <= 0)
		return kill;

which means we don't call inode_has_no_xattr(). I don't think that we
did this intentionally. inode_has_no_xattr() just sets S_NOSEC which
means next time we call into __file_remove_privs() we can return earlier
instead of hitting dentry_needs_remove_privs() again:

if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode))
	return 0;

So I think that needs to be fixed?

Christian