linux-kernel - Re: [PATCH] evm: stop avoidably reading i_writecount in evm_file

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: <d31e3e0e9e35523bb40b5dbbb29790c874e7627d.camel@huaweicloud.com>
Date: Tue, 24 Sep 2024 13:56:23 +0200
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: zohar@...ux.ibm.com, roberto.sassu@...wei.com, paul@...l-moore.com, 
	jmorris@...ei.org, serge@...lyn.com, linux-kernel@...r.kernel.org, 
	linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH] evm: stop avoidably reading i_writecount in
 evm_file_release

On Mon, 2024-09-23 at 07:26 +0200, Mateusz Guzik wrote:
> On Fri, Aug 16, 2024 at 1:53 PM Roberto Sassu
> <roberto.sassu@...weicloud.com> wrote:
> > 
> > On Tue, 2024-08-06 at 15:36 +0200, Mateusz Guzik wrote:
> > > The EVM_NEW_FILE flag is unset if the file already existed at the time
> > > of open and this can be checked without looking at i_writecount.
> > 
> > Agreed. EVM_NEW_FILE is not going to be set during the open(), only
> > before, in evm_post_path_mknod().
> > 
> > Looks good to me.
> > 
> > Reviewed-by: Roberto Sassu <roberto.sassu@...wei.com>
> > 
> > Thanks
> 
> thanks for the review
> 
> are there plans to pick this up for this merge window?

Welcome! Sorry, Mimi wants to do few more checks. It is more likely
that we will pick this for the next version.

Roberto

> > 
> > Roberto
> > 
> > > Not accessing it reduces traffic on the cacheline during parallel open
> > > of the same file and drop the evm_file_release routine from second place
> > > to bottom of the profile.
> > > 
> > > Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
> > > ---
> > > 
> > > The context is that I'm writing a patch which removes one lockref
> > > get/put cycle on parallel open. An operational WIP reduces ping-pong in
> > > that area and made do_dentry_open skyrocket along with evm_file_release,
> > > due to i_writecount access. With the patch they go down again and
> > > apparmor takes the rightful first place.
> > > 
> > > The patch accounts for about 5% speed up at 20 cores running open3 from
> > > will-it-scale on top of the above wip. (the apparmor + lockref thing
> > > really don't scale, that's next)
> > > 
> > > I would provide better measurements, but the wip is not ready (as the
> > > description suggests) and I need evm out of the way for the actual
> > > patch.
> > > 
> > >  security/integrity/evm/evm_main.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > > index 62fe66dd53ce..309630f319e2 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -1084,7 +1084,8 @@ static void evm_file_release(struct file *file)
> > >       if (!S_ISREG(inode->i_mode) || !(mode & FMODE_WRITE))
> > >               return;
> > > 
> > > -     if (iint && atomic_read(&inode->i_writecount) == 1)
> > > +     if (iint && iint->flags & EVM_NEW_FILE &&
> > > +         atomic_read(&inode->i_writecount) == 1)
> > >               iint->flags &= ~EVM_NEW_FILE;
> > >  }
> > > 
> > 
> 
>