Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <a0913021426ead2fc5e2a3db013335a67cdd4322.camel@huaweicloud.com>
Subject: Re: [PATCH v3 25/25] integrity: Switch from rbtree to LSM-managed
 blob for integrity_iint_cache
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
To:     Stefan Berger <stefanb@linux.ibm.com>, viro@zeniv.linux.org.uk,
        brauner@kernel.org, chuck.lever@oracle.com, jlayton@kernel.org,
        neilb@suse.de, kolga@netapp.com, Dai.Ngo@oracle.com,
        tom@talpey.com, zohar@linux.ibm.com, dmitry.kasatkin@gmail.com,
        paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com,
        dhowells@redhat.com, jarkko@kernel.org,
        stephen.smalley.work@gmail.com, eparis@parisplace.org,
        casey@schaufler-ca.com
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-nfs@vger.kernel.org, linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org, keyrings@vger.kernel.org,
        selinux@vger.kernel.org, Roberto Sassu <roberto.sassu@huawei.com>
Date:   Fri, 15 Sep 2023 11:39:44 +0200
In-Reply-To: <82486de4-2917-afb6-2ae3-6ea7f1346dc0@linux.ibm.com>
References: <20230904133415.1799503-1-roberto.sassu@huaweicloud.com>
         <20230904134049.1802006-6-roberto.sassu@huaweicloud.com>
         <82486de4-2917-afb6-2ae3-6ea7f1346dc0@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-09-12 at 12:19 -0400, Stefan Berger wrote:
> On 9/4/23 09:40, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Before the security field of kernel objects could be shared among LSMs with
> > the LSM stacking feature, IMA and EVM had to rely on an alternative storage
> > of inode metadata. The association between inode metadata and inode is
> > maintained through an rbtree.
> > 
> > With the reservation mechanism offered by the LSM infrastructure, the
> > rbtree is no longer necessary, as each LSM could reserve a space in the
> > security blob for each inode. Thus, request from the 'integrity' LSM a
> > space in the security blob for the pointer of inode metadata
> > (integrity_iint_cache structure).
> > 
> > Prefer this to allocating the integrity_iint_cache structure directly, as
> > IMA would require it only for a subset of inodes. Always allocating it
> > would cause a waste of memory.
> > 
> > Introduce two primitives for getting and setting the pointer of
> > integrity_iint_cache in the security blob, respectively
> > integrity_inode_get_iint() and integrity_inode_set_iint(). This would make
> > the code more understandable, as they directly replace rbtree operations.
> > 
> > Locking is not needed, as access to inode metadata is not shared, it is per
> > inode.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> > ---
> > 
> > @@ -145,10 +91,8 @@ static void integrity_inode_free(struct inode *inode)
> >   	if (!IS_IMA(inode))
> >   		return;
> 
> I think you can remove this check !IS_IMA()  as well since the next 
> function called here integrity_iint_find() already has this check:
> 
> struct integrity_iint_cache *integrity_iint_find(struct inode *inode)
> {
>          if (!IS_IMA(inode))
>                  return NULL;
> 
>          return integrity_inode_get_iint(inode);
> }

I agree, thanks!

Roberto

> >   
> > -	write_lock(&integrity_iint_lock);
> > -	iint = __integrity_iint_find(inode);
> > -	rb_erase(&iint->rb_node, &integrity_iint_tree);
> > -	write_unlock(&integrity_iint_lock);
> > +	iint = integrity_iint_find(inode);         <--------------
> > +	integrity_inode_set_iint(inode, NULL);
> >   
> >   	iint_free(iint);
> >   }