devcgroup: skip superfluous checks when found the DEV_ALL elem

PATCH In review Li Zefan 5 participants 6 messages Apr 7, 2009 – Apr 10, 2009 2 messages not in the archive Security (LSM)

lore

Share & tools

lore

Li Zefan

author

2009-04-07 05:46 UTC #0 lore raw

PATCH 1/1 devcgroup: skip superfluous checks when found the DEV_ALL elem

While walking through the whitelist, if the DEV_ALL item is found, no more check is needed.

Signed-off-by Li Zefan <[email protected]>

---
 security/device_cgroup.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

security/device_cgroup.c +6 -4

index 5fda7df..b8186ba 100644
@@ -490,7 +490,7 @@ int devcgroup_inode_permission(struct inode *inode, int mask)
 
 	list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
 		if (wh->type & DEV_ALL)
-			goto acc_check;
+			goto found;
 		if ((wh->type & DEV_BLOCK) && !S_ISBLK(inode->i_mode))
 			continue;
 		if ((wh->type & DEV_CHAR) && !S_ISCHR(inode->i_mode))

@@ -499,11 +499,12 @@ int devcgroup_inode_permission(struct inode *inode, int mask)
 			continue;
 		if (wh->minor != ~0 && wh->minor != iminor(inode))
 			continue;
-acc_check:
+
 		if ((mask & MAY_WRITE) && !(wh->access & ACC_WRITE))
 			continue;
 		if ((mask & MAY_READ) && !(wh->access & ACC_READ))
 			continue;
+found:
 		rcu_read_unlock();
 		return 0;
 	}

@@ -527,7 +528,7 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 
 	list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
 		if (wh->type & DEV_ALL)
-			goto acc_check;
+			goto found;
 		if ((wh->type & DEV_BLOCK) && !S_ISBLK(mode))
 			continue;
 		if ((wh->type & DEV_CHAR) && !S_ISCHR(mode))

@@ -536,9 +537,10 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 			continue;
 		if (wh->minor != ~0 && wh->minor != MINOR(dev))
 			continue;
-acc_check:
+
 		if (!(wh->access & ACC_MKNOD))
 			continue;
+found:
 		rcu_read_unlock();
 		return 0;
 	}

Signature · 1 line

1.5.4.rc3

A message that is not in the archive (sent off-list or not yet received): [email protected]

Serge E. Hallyn

Acked-by

2009-04-08 02:55 UTC #2 lore raw

Quoted 2 lines · Li Zefan

While walking through the whitelist, if the DEV_ALL item is found, no more check is needed.

Right, because the DEV_ALL item always has all permissions.

Quoted 1 line

Signed-off-by Li Zefan <[email protected]>

Acked-by Serge Hallyn <serue-r/[email protected]>

thanks,
-serge

Quoted 54 lines

---
 security/device_cgroup.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

security/device_cgroup.c +6 -4

index 5fda7df..b8186ba 100644
@@ -490,7 +490,7 @@ int devcgroup_inode_permission(struct inode *inode, int mask)
 
 	list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
 		if (wh->type & DEV_ALL)
-			goto acc_check;
+			goto found;
 		if ((wh->type & DEV_BLOCK) && !S_ISBLK(inode->i_mode))
 			continue;
 		if ((wh->type & DEV_CHAR) && !S_ISCHR(inode->i_mode))

@@ -499,11 +499,12 @@ int devcgroup_inode_permission(struct inode *inode, int mask)
 			continue;
 		if (wh->minor != ~0 && wh->minor != iminor(inode))
 			continue;
-acc_check:
+
 		if ((mask & MAY_WRITE) && !(wh->access & ACC_WRITE))
 			continue;
 		if ((mask & MAY_READ) && !(wh->access & ACC_READ))
 			continue;
+found:
 		rcu_read_unlock();
 		return 0;
 	}

@@ -527,7 +528,7 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 
 	list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
 		if (wh->type & DEV_ALL)
-			goto acc_check;
+			goto found;
 		if ((wh->type & DEV_BLOCK) && !S_ISBLK(mode))
 			continue;
 		if ((wh->type & DEV_CHAR) && !S_ISCHR(mode))

@@ -536,9 +537,10 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 			continue;
 		if (wh->minor != ~0 && wh->minor != MINOR(dev))
 			continue;
-acc_check:
+
 		if (!(wh->access & ACC_MKNOD))
 			continue;
+found:
 		rcu_read_unlock();
 		return 0;
 	}

Signature · 1 line

1.5.4.rc3

Serge E. Hallyn

Acked-by on v1 1/1

2009-04-08 02:55 UTC #3 lore raw

In reply to Li Zefan

Quoted 2 lines · Li Zefan

While walking through the whitelist, if the DEV_ALL item is found, no more check is needed.

Right, because the DEV_ALL item always has all permissions.

Quoted 1 line

Signed-off-by Li Zefan <[email protected]>

Acked-by Serge Hallyn <[email protected]>

thanks,
-serge

Quoted 54 lines

---
 security/device_cgroup.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

security/device_cgroup.c +6 -4

index 5fda7df..b8186ba 100644
@@ -490,7 +490,7 @@ int devcgroup_inode_permission(struct inode *inode, int mask)
 
 	list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
 		if (wh->type & DEV_ALL)
-			goto acc_check;
+			goto found;
 		if ((wh->type & DEV_BLOCK) && !S_ISBLK(inode->i_mode))
 			continue;
 		if ((wh->type & DEV_CHAR) && !S_ISCHR(inode->i_mode))

@@ -499,11 +499,12 @@ int devcgroup_inode_permission(struct inode *inode, int mask)
 			continue;
 		if (wh->minor != ~0 && wh->minor != iminor(inode))
 			continue;
-acc_check:
+
 		if ((mask & MAY_WRITE) && !(wh->access & ACC_WRITE))
 			continue;
 		if ((mask & MAY_READ) && !(wh->access & ACC_READ))
 			continue;
+found:
 		rcu_read_unlock();
 		return 0;
 	}

@@ -527,7 +528,7 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 
 	list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
 		if (wh->type & DEV_ALL)
-			goto acc_check;
+			goto found;
 		if ((wh->type & DEV_BLOCK) && !S_ISBLK(mode))
 			continue;
 		if ((wh->type & DEV_CHAR) && !S_ISCHR(mode))

@@ -536,9 +537,10 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 			continue;
 		if (wh->minor != ~0 && wh->minor != MINOR(dev))
 			continue;
-acc_check:
+
 		if (!(wh->access & ACC_MKNOD))
 			continue;
+found:
 		rcu_read_unlock();
 		return 0;
 	}

Signature · 1 line

1.5.4.rc3

Andrew Morton

2009-04-10 00:06 UTC #4 lore raw

In reply to Li Zefan

Quoted 3 lines · On Tue, 07 Apr 2009 13:47:16 +0800 Li Zefan…

While walking through the whitelist, if the DEV_ALL item is found, no more check is needed.

It's unobvious whether this is a behavioural change, a bugfix or just a speedup?

Quoted 49 lines

security/device_cgroup.c +6 -4

index 5fda7df..b8186ba 100644
@@ -490,7 +490,7 @@ int devcgroup_inode_permission(struct inode *inode, int mask)
 
 	list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
 		if (wh->type & DEV_ALL)
-			goto acc_check;
+			goto found;
 		if ((wh->type & DEV_BLOCK) && !S_ISBLK(inode->i_mode))
 			continue;
 		if ((wh->type & DEV_CHAR) && !S_ISCHR(inode->i_mode))

@@ -499,11 +499,12 @@ int devcgroup_inode_permission(struct inode *inode, int mask)
 			continue;
 		if (wh->minor != ~0 && wh->minor != iminor(inode))
 			continue;
-acc_check:
+
 		if ((mask & MAY_WRITE) && !(wh->access & ACC_WRITE))
 			continue;
 		if ((mask & MAY_READ) && !(wh->access & ACC_READ))
 			continue;
+found:
 		rcu_read_unlock();
 		return 0;
 	}

@@ -527,7 +528,7 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 
 	list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
 		if (wh->type & DEV_ALL)
-			goto acc_check;
+			goto found;
 		if ((wh->type & DEV_BLOCK) && !S_ISBLK(mode))
 			continue;
 		if ((wh->type & DEV_CHAR) && !S_ISCHR(mode))

@@ -536,9 +537,10 @@ int devcgroup_inode_mknod(int mode, dev_t dev)
 			continue;
 		if (wh->minor != ~0 && wh->minor != MINOR(dev))
 			continue;
-acc_check:
+
 		if (!(wh->access & ACC_MKNOD))
 			continue;
+found:
 		rcu_read_unlock();
 		return 0;
 	}

Li Zefan

author

2009-04-10 00:19 UTC #5 lore raw

Quoted 10 lines · Andrew Morton

Quoted 3 lines · On Tue, 07 Apr 2009 13:47:16 +0800 Li Zefan <[email protected]> wrote:
While walking through the whitelist, if the DEV_ALL item is found, no more check is needed.
It's unobvious whether this is a behavioural change, a bugfix or just a speedup?

It's a speedup. I thought it's obvious by saying "skip superfluous checks"..

A message that is not in the archive (sent off-list or not yet received): 20090409170610.9ee6e8e4.akpm-de/[email protected]

Li Zefan

2009-04-10 00:20 UTC #7 lore raw

Quoted 10 lines · Andrew Morton

Quoted 3 lines · On Tue, 07 Apr 2009 13:47:16 +0800 Li Zefan…
While walking through the whitelist, if the DEV_ALL item is found, no more check is needed.
It's unobvious whether this is a behavioural change, a bugfix or just a speedup?

It's a speedup. I thought it's obvious by saying "skip superfluous checks"..