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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
