Re: [PATCH 4/5] [fs/9P] Add acl mount option

["Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com>]

On 1/26/2011 2:06 AM, Aneesh Kumar K. V wrote:
> On Tue, 25 Jan 2011 17:12:42 -0800, "Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com> wrote:
>> The mount option access=client is overloaded as it assumes acl too.
>> Adding acl=on option to enable ACL, anyother option or absense of this
>> flag turns off ACLs at the client.
>>
>> Ideally, the access mode 'client' should be just like V9FS_ACCESS_USER
>> except it underscores the location of access check.
>> Traditional 9P protocol lets the server perform access checks but with
>> this mode, all the access checks will be performed on the client itself.
>> Server just follows the client's directive.
>>
>> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
>> ---
>>  fs/9p/acl.c       |   10 +++++-----
>>  fs/9p/v9fs.c      |   34 +++++++++++++++++++++++++++-------
>>  fs/9p/v9fs.h      |    6 +++++-
>>  fs/9p/vfs_super.c |    2 +-
>>  4 files changed, 38 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/9p/acl.c b/fs/9p/acl.c
>> index 0a2e480..48be5c3 100644
>> --- a/fs/9p/acl.c
>> +++ b/fs/9p/acl.c
>> @@ -59,7 +59,7 @@ int v9fs_get_acl(struct inode *inode, struct p9_fid *fid)
>>  	struct v9fs_session_info *v9ses;
>>
>>  	v9ses = v9fs_inode2v9ses(inode);
>> -	if ((v9ses->flags & V9FS_ACCESS_MASK) != V9FS_ACCESS_CLIENT) {
>> +	if ((v9ses->flags & V9FS_ACL_MASK) != V9FS_ACL_ON) {
> 
> I guess what we need is 
> 
> 	if (((v9ses->flags & V9FS_ACCESS_MASK) != V9FS_ACCESS_CLIENT) &&
>             ((v9ses->flags & V9FS_ACL_MASK) != V9FS_ACL_ON)) {
> 
> the current feature should restrict acl option only with access=client,
> and access=client should be default enabled for dotl.

It does; if the access is not client I turn off the acl bit. See below
v9fs_session_init() change.

> 
> 
>>  		set_cached_acl(inode, ACL_TYPE_DEFAULT, NULL);
>>  		set_cached_acl(inode, ACL_TYPE_ACCESS, NULL);
>>  		return 0;
>> @@ -104,9 +104,9 @@ int v9fs_check_acl(struct inode *inode, int mask, unsigned int flags)
>>  		return -ECHILD;
>>
>>  	v9ses = v9fs_inode2v9ses(inode);
>> -	if ((v9ses->flags & V9FS_ACCESS_MASK) != V9FS_ACCESS_CLIENT) {
>> +	if ((v9ses->flags & V9FS_ACL_MASK) != V9FS_ACL_ON) {
>>  		/*
>> -		 * On access = client mode get the acl
>> +		 * On access = client  and acl = on mode get the acl
>>  		 * values from the server
>>  		 */
>>  		return 0;
>> @@ -264,7 +264,7 @@ static int v9fs_xattr_get_acl(struct dentry *dentry, const char *name,
>>  	/*
>>  	 * We allow set/get/list of acl when access=client is not specified
>>  	 */
>> -	if ((v9ses->flags & V9FS_ACCESS_MASK) != V9FS_ACCESS_CLIENT)
>> +	if ((v9ses->flags & V9FS_ACL_MASK) != V9FS_ACL_ON)
>>  		return v9fs_remote_get_acl(dentry, name, buffer, size, type);
>>
>>  	acl = v9fs_get_cached_acl(dentry->d_inode, type);
>> @@ -315,7 +315,7 @@ static int v9fs_xattr_set_acl(struct dentry *dentry, const char *name,
>>  	 * set the attribute on the remote. Without even looking at the
>>  	 * xattr value. We leave it to the server to validate
>>  	 */
>> -	if ((v9ses->flags & V9FS_ACCESS_MASK) != V9FS_ACCESS_CLIENT)
>> +	if ((v9ses->flags & V9FS_ACL_MASK) != V9FS_ACL_ON)
>>  		return v9fs_remote_set_acl(dentry, name,
>>  					   value, size, flags, type);
>>
>> diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
>> index d34f293..f936433 100644
>> --- a/fs/9p/v9fs.c
>> +++ b/fs/9p/v9fs.c
>> @@ -55,7 +55,7 @@ enum {
>>  	/* Cache options */
>>  	Opt_cache_loose, Opt_fscache,
>>  	/* Access options */
>> -	Opt_access,
>> +	Opt_access, Opt_acl,
>>  	/* Error token */
>>  	Opt_err
>>  };
>> @@ -73,6 +73,7 @@ static const match_table_t tokens = {
>>  	{Opt_fscache, "fscache"},
>>  	{Opt_cachetag, "cachetag=%s"},
>>  	{Opt_access, "access=%s"},
>> +	{Opt_acl, "acl=%s"},
> 
> why not just say -o posix_acl ?. That way i can later say -o richacl -o
> selinux etc.

Good point. How about no underscore posixacl?

> 
> 
>>  	{Opt_err, NULL}
>>  };
>>
>> @@ -194,13 +195,7 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts)
>>  			else if (strcmp(s, "any") == 0)
>>  				v9ses->flags |= V9FS_ACCESS_ANY;
>>  			else if (strcmp(s, "client") == 0) {
>> -#ifdef CONFIG_9P_FS_POSIX_ACL
>>  				v9ses->flags |= V9FS_ACCESS_CLIENT;
>> -#else
>> -				P9_DPRINTK(P9_DEBUG_ERROR,
>> -					"Not defined CONFIG_9P_FS_POSIX_ACL. "
>> -					"Ignoring access=client option\n");
>> -#endif
>>  			} else {
>>  				v9ses->flags |= V9FS_ACCESS_SINGLE;
>>  				v9ses->uid = simple_strtoul(s, &e, 10);
>> @@ -210,6 +205,27 @@ static int v9fs_parse_options(struct v9fs_session_info *v9ses, char *opts)
>>  			kfree(s);
>>  			break;
>>
>> +		case Opt_acl:
>> +			s = match_strdup(&args[0]);
>> +			if (!s) {
>> +				ret = -ENOMEM;
>> +				P9_DPRINTK(P9_DEBUG_ERROR,
>> +				  "problem allocating copy of acl arg\n");
>> +				goto free_and_return;
>> +			}
>> +			v9ses->flags &= ~V9FS_ACL_MASK;
> 
> is this to support acl=off ? Local file system needs a method to disable
> acl because most of them support changing default mount options, For
> 9p i guess default is what we have in the code, so if default is
> disabled acl, then we need an option to enable. or if decide to enable 
> acl by default we need to have an option to disable it. 
> 
> We also need to make sure this is option is available only for dotl . 

By default we are off; this statement is just for completeness.

> 
>> +			if (strcmp(s, "on") == 0) {
>> +#ifdef CONFIG_9P_FS_POSIX_ACL
>> +				v9ses->flags |= V9FS_ACL_ON;
> 
> 
> It would be better 
> 				v9ses->flags |= V9FS_POSIX_ACL;

yes; will change it.

> 
> 
> Presence of the bit indicate whether acl is enabled or not, why do
> we need the #define to say an "_ON" ?
> 
> 
>> +#else
>> +				P9_DPRINTK(P9_DEBUG_ERROR,
>> +					"Not defined CONFIG_9P_FS_POSIX_ACL. "
>> +					"Ignoring acl=on option\n");
>> +#endif
>> +			}
>> +			kfree(s);
>> +			break;
>> +
>>  		default:
>>  			continue;
>>  		}
>> @@ -294,6 +310,10 @@ struct p9_fid *v9fs_session_init(struct v9fs_session_info *v9ses,
>>  		 */
>>  		v9ses->flags &= ~V9FS_ACCESS_MASK;
>>  		v9ses->flags |= V9FS_ACCESS_USER;
>> +		/*
>> +		 * We support ACLs only in dotl and V9FS_ACCESS_CLIENT
>> +		 */
>> +		v9ses->flags &= ~V9FS_ACL_MASK;
>>  	}
>>  	/*FIXME !! */
>>  	/* for legacy mode, fall back to V9FS_ACCESS_ANY */
>> diff --git a/fs/9p/v9fs.h b/fs/9p/v9fs.h
>> index c4b5d88..f3bad79 100644
>> --- a/fs/9p/v9fs.h
>> +++ b/fs/9p/v9fs.h
>> @@ -28,8 +28,10 @@
>>   * @V9FS_PROTO_2000L: whether or not to use 9P2000.l extensions
>>   * @V9FS_ACCESS_SINGLE: only the mounting user can access the hierarchy
>>   * @V9FS_ACCESS_USER: a new attach will be issued for every user (default)
>> + * @V9FS_ACCESS_CLIENT: Just like user, but access check is performed on client.
>>   * @V9FS_ACCESS_ANY: use a single attach for all users
>>   * @V9FS_ACCESS_MASK: bit mask of different ACCESS options
>> + * @V9FS_ACL_ON: If ACLs are enforced
>>   *
>>   * Session flags reflect options selected by users at mount time
>>   */
>> @@ -37,13 +39,15 @@
>>  			 V9FS_ACCESS_USER |   \
>>  			 V9FS_ACCESS_CLIENT)
>>  #define V9FS_ACCESS_MASK V9FS_ACCESS_ANY
>> +#define V9FS_ACL_MASK V9FS_ACL_ON
>>
>>  enum p9_session_flags {
>>  	V9FS_PROTO_2000U	= 0x01,
>>  	V9FS_PROTO_2000L	= 0x02,
>>  	V9FS_ACCESS_SINGLE	= 0x04,
>>  	V9FS_ACCESS_USER	= 0x08,
>> -	V9FS_ACCESS_CLIENT	= 0x10
>> +	V9FS_ACCESS_CLIENT	= 0x10,
>> +	V9FS_ACL_ON		= 0x20
>>  };
>>
>>  /* possible values of ->cache */
>> diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
>> index dbaabe3..357d3b4 100644
>> --- a/fs/9p/vfs_super.c
>> +++ b/fs/9p/vfs_super.c
>> @@ -91,7 +91,7 @@ v9fs_fill_super(struct super_block *sb, struct v9fs_session_info *v9ses,
>>  	    MS_NOATIME;
>>
>>  #ifdef CONFIG_9P_FS_POSIX_ACL
>> -	if ((v9ses->flags & V9FS_ACCESS_MASK) == V9FS_ACCESS_CLIENT)
>> +	if ((v9ses->flags & V9FS_ACL_MASK) == V9FS_ACL_ON)
>>  		sb->s_flags |= MS_POSIXACL;
>>  #endif
>>
> 
> Also in the patch is would be nice if we could be explicit about posix
> acl. 

Yes; Also I will send another patch making access=client default for 9P2000.L
for all other versions we can leave the default to user.

Thanks for your comments.

- JV

> 
> -aneesh