[PATCH] evm: stop avoidably reading i_writecount in evm_file_release

[PATCH] evm: stop avoidably reading i_writecount in evm_file_release

[Mateusz Guzik]

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.

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@gmail.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;
 }
 
-- 
2.43.0

Re: [PATCH] evm: stop avoidably reading i_writecount in evm_file_release

[Roberto Sassu]

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@huawei.com>

Thanks

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@gmail.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;
>  }
>  

Re: [PATCH] evm: stop avoidably reading i_writecount in evm_file_release

[Mateusz Guzik]

On Fri, Aug 16, 2024 at 1:53 PM Roberto Sassu
<roberto.sassu@huaweicloud.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@huawei.com>
>
> Thanks

thanks for the review

are there plans to pick this up for this merge window?

>
> 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@gmail.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;
> >  }
> >
>


-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] evm: stop avoidably reading i_writecount in evm_file_release

[Roberto Sassu]

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@huaweicloud.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@huawei.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@gmail.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;
> > >  }
> > > 
> > 
> 
> 

Re: [PATCH] evm: stop avoidably reading i_writecount in evm_file_release

[Mimi Zohar]

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.
> 
> 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@gmail.com>

Sorry for the delay.  It's now queued in next-integrity.

thanks,

Mimi