Re: [PATCH] device_cgroup: do not use rule acceptance function to validate access

[Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

Hello,

On Mon, Apr 14, 2014 at 10:47:54AM -0400, Aristeu Rozanski wrote:
> may_access() is currently used to validate both new exceptions from children
> groups and to check if an access is allowed. This not only makes it hard to
> understand and maintain, but it's also incorrect.
> 
> It currently allows one to:
> 
> 	# mkdir new_group
> 	# cd new_group
> 	# echo $$ >tasks
> 	# echo "c 1:3 w" >devices.deny
> 	# echo >/dev/null
> 	# echo $?
> 	0

It'd be nice to explain how this is broken and why the code paths
can't be shared.

> This patch implements the device file access check separately and fixes
> the issue.
> 
> This is broken since c39a2a3018f8065cb5ea38b0314c1bbedb2cfa0d

Please use 12-digits-of-SHA1 ("Subject of the patch") format.

> After review, this should be considered for stable series.
> 
> Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Can you please cc stable on the next posting?

> -	rc = may_access(dev_cgroup, &ex, dev_cgroup->behavior);
> +	behavior = dev_cgroup->behavior;
> +	list_for_each_entry_rcu(ex, &dev_cgroup->exceptions, list) {
> +		if (type == DEV_BLOCK && !(ex->type & DEV_BLOCK))
> +			continue;
> +		if (type == DEV_CHAR && !(ex->type & DEV_CHAR))
> +			continue;
> +		if (ex->major != ~0 && major != ex->major)
> +			continue;
> +		if (ex->minor != ~0 && minor != ex->minor)
> +			continue;
> +		if (access & (~ex->access))
> +			continue;
> +		match = true;
> +		break;

Can't we at least factor out the above part and share it between the
two functions?  Currently, there's a lot of duplication in rather
delicate code and it's not clear where they differ and why.

Thanks.

-- 
tejun