[PATCH 1/1] Smack:- Remove mutex lock "smk_lock" from inode_smack structure.

[PATCH 1/1] Smack:- Remove mutex lock "smk_lock" from inode_smack structure.

[Vishal Goel]

"smk_lock" mutex is used during inode instantiation in
smack_d_instantiate()function. It has been used to avoid
simultaneous access on same inode security structure.
Since smack related initialization is done only once i.e during
inode creation. If the inode has already been instantiated then
smack_d_instantiate() function just returns without doing
anything.

So it means mutex lock is required only during inode creation.
But since 2 processes can't create same inodes or files
simultaneously. Also linking or some other file operation can't
be done simultaneously when the file is getting created since
file lookup will fail before dentry inode linkup which is done
after smack initialization.
So no mutex lock is required in inode_smack structure.

It will save memory as well as improve some performance.
If 40000 inodes are created in system, it will save 1.5 MB on
32-bit systems & 2.8 MB on 64-bit systems.

Signed-off-by: Vishal Goel <vishal.goel@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 security/smack/smack.h     | 1 -
 security/smack/smack_lsm.c | 8 ++------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 62529f3..fd09dc8 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -109,7 +109,6 @@ struct inode_smack {
 	struct smack_known	*smk_inode;	/* label of the fso */
 	struct smack_known	*smk_task;	/* label of the task */
 	struct smack_known	*smk_mmap;	/* label of the mmap domain */
-	struct mutex		smk_lock;	/* initialization lock */
 	int			smk_flags;	/* smack inode flags */
 	struct rcu_head         smk_rcu;	/* for freeing inode_smack */
 };
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2862fc3..10c2bdd 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -317,7 +317,6 @@ static void init_inode_smack(struct inode *inode, struct smack_known *skp)
 
 	isp->smk_inode = skp;
 	isp->smk_flags = 0;
-	mutex_init(&isp->smk_lock);
 }
 
 /**
@@ -3274,13 +3273,12 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
 
 	isp = smack_inode(inode);
 
-	mutex_lock(&isp->smk_lock);
 	/*
 	 * If the inode is already instantiated
 	 * take the quick way out
 	 */
 	if (isp->smk_flags & SMK_INODE_INSTANT)
-		goto unlockandout;
+		return;
 
 	sbp = inode->i_sb;
 	sbsp = sbp->s_security;
@@ -3331,7 +3329,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
 			break;
 		}
 		isp->smk_flags |= SMK_INODE_INSTANT;
-		goto unlockandout;
+		return;
 	}
 
 	/*
@@ -3466,8 +3464,6 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
 
 	isp->smk_flags |= (SMK_INODE_INSTANT | transflag);
 
-unlockandout:
-	mutex_unlock(&isp->smk_lock);
 	return;
 }
 
-- 
1.9.1

Re: [PATCH 1/1] Smack:- Remove mutex lock "smk_lock" from inode_smack structure.

[Casey Schaufler]

On 4/24/2020 5:55 AM, Vishal Goel wrote:
> "smk_lock" mutex is used during inode instantiation in
> smack_d_instantiate()function. It has been used to avoid
> simultaneous access on same inode security structure.
> Since smack related initialization is done only once i.e during
> inode creation. If the inode has already been instantiated then
> smack_d_instantiate() function just returns without doing
> anything.
>
> So it means mutex lock is required only during inode creation.
> But since 2 processes can't create same inodes or files
> simultaneously. Also linking or some other file operation can't
> be done simultaneously when the file is getting created since
> file lookup will fail before dentry inode linkup which is done
> after smack initialization.
> So no mutex lock is required in inode_smack structure.
>
> It will save memory as well as improve some performance.
> If 40000 inodes are created in system, it will save 1.5 MB on
> 32-bit systems & 2.8 MB on 64-bit systems.
>
> Signed-off-by: Vishal Goel <vishal.goel@samsung.com>
> Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>

I am taking this for 5.8. Thank you.

> ---
>  security/smack/smack.h     | 1 -
>  security/smack/smack_lsm.c | 8 ++------
>  2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 62529f3..fd09dc8 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -109,7 +109,6 @@ struct inode_smack {
>  	struct smack_known	*smk_inode;	/* label of the fso */
>  	struct smack_known	*smk_task;	/* label of the task */
>  	struct smack_known	*smk_mmap;	/* label of the mmap domain */
> -	struct mutex		smk_lock;	/* initialization lock */
>  	int			smk_flags;	/* smack inode flags */
>  	struct rcu_head         smk_rcu;	/* for freeing inode_smack */
>  };
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2862fc3..10c2bdd 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -317,7 +317,6 @@ static void init_inode_smack(struct inode *inode, struct smack_known *skp)
>  
>  	isp->smk_inode = skp;
>  	isp->smk_flags = 0;
> -	mutex_init(&isp->smk_lock);
>  }
>  
>  /**
> @@ -3274,13 +3273,12 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>  
>  	isp = smack_inode(inode);
>  
> -	mutex_lock(&isp->smk_lock);
>  	/*
>  	 * If the inode is already instantiated
>  	 * take the quick way out
>  	 */
>  	if (isp->smk_flags & SMK_INODE_INSTANT)
> -		goto unlockandout;
> +		return;
>  
>  	sbp = inode->i_sb;
>  	sbsp = sbp->s_security;
> @@ -3331,7 +3329,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>  			break;
>  		}
>  		isp->smk_flags |= SMK_INODE_INSTANT;
> -		goto unlockandout;
> +		return;
>  	}
>  
>  	/*
> @@ -3466,8 +3464,6 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
>  
>  	isp->smk_flags |= (SMK_INODE_INSTANT | transflag);
>  
> -unlockandout:
> -	mutex_unlock(&isp->smk_lock);
>  	return;
>  }
>