| Message ID | 1441962597-13543-4-git-send-email-shengyong1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/11/2015 05:09 PM, Sheng Yong wrote: > * initialize ACL when file is created > * get ACL through ubifs_getxattr > * set ACL through ubifs_setxattr. If the size of ACL is 0, the ACL is going > to be removed > * clear cached ACL in ubifs_removexattr if ACL xattr is removed > * modify ACL if file mode is changed > > Signed-off-by: Sheng Yong <shengyong1@huawei.com> > --- > fs/ubifs/dir.c | 16 ++++ > fs/ubifs/file.c | 6 ++ > fs/ubifs/xattr.c | 243 ++++++++++++++++++++++++++++++++++--------------------- > 3 files changed, 175 insertions(+), 90 deletions(-) > > diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c > index 69ccc78..db5dd45 100644 > --- a/fs/ubifs/dir.c > +++ b/fs/ubifs/dir.c > @@ -270,6 +270,10 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode, > goto out_budg; > } > > + err = ubifs_init_acl(dir, inode); > + if (err) > + goto out_inode; > + > err = ubifs_init_security(dir, inode, &dentry->d_name); > if (err) > goto out_inode; > @@ -731,6 +735,10 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) > goto out_budg; > } > > + err = ubifs_init_acl(dir, inode); > + if (err) > + goto out_inode; > + > err = ubifs_init_security(dir, inode, &dentry->d_name); > if (err) > goto out_inode; > @@ -810,6 +818,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry, > goto out_budg; > } > > + err = ubifs_init_acl(dir, inode); > + if (err) > + goto out_inode; > + > init_special_inode(inode, inode->i_mode, rdev); > inode->i_size = ubifs_inode(inode)->ui_size = devlen; > ui = ubifs_inode(inode); > @@ -898,6 +910,10 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry, > ui->data_len = len; > inode->i_size = ubifs_inode(inode)->ui_size = len; > > + err = ubifs_init_acl(dir, inode); > + if (err) > + goto out_inode; > + > err = ubifs_init_security(dir, inode, &dentry->d_name); > if (err) > goto out_inode; > diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c > index 58298f8..74f4c63 100644 > --- a/fs/ubifs/file.c > +++ b/fs/ubifs/file.c > @@ -52,6 +52,7 @@ > #include "ubifs.h" > #include <linux/mount.h> > #include <linux/slab.h> > +#include <linux/posix_acl.h> > > static int read_block(struct inode *inode, void *addr, unsigned int block, > struct ubifs_data_node *dn) > @@ -1246,6 +1247,11 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode, > mark_inode_dirty_sync(inode); > mutex_unlock(&ui->ui_mutex); > > + if (attr->ia_valid & ATTR_MODE) > + err = posix_acl_chmod(inode, inode->i_mode); > + if (err) > + return err; > + > if (release) > ubifs_release_budget(c, &req); > if (IS_SYNC(inode)) > diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c > index 6534b98..dad1070 100644 > --- a/fs/ubifs/xattr.c > +++ b/fs/ubifs/xattr.c > @@ -52,7 +52,6 @@ > * in the VFS inode cache. The xentries are cached in the LNC cache (see > * tnc.c). > * > - * ACL support is not implemented. > */ > > #include "ubifs.h" > @@ -78,6 +77,10 @@ enum { > USER_XATTR, > TRUSTED_XATTR, > SECURITY_XATTR, > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + POSIX_ACL_DEFAULT, > + POSIX_ACL_ACCESS, > +#endif > }; > > static const struct inode_operations empty_iops; > @@ -276,6 +279,18 @@ static int check_namespace(const struct qstr *nm) > if (nm->name[sizeof(XATTR_SECURITY_PREFIX) - 1] == '\0') > return -EINVAL; > type = SECURITY_XATTR; > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + } else if (!strncmp(nm->name, XATTR_NAME_POSIX_ACL_DEFAULT, > + sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1)) { > + if (nm->name[sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1] != '\0') > + return -EINVAL; > + type = POSIX_ACL_DEFAULT; > + } else if (!strncmp(nm->name, XATTR_NAME_POSIX_ACL_ACCESS, > + sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1)) { > + if (nm->name[sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1] != '\0') > + return -EINVAL; > + type = POSIX_ACL_ACCESS; > +#endif > } else > return -EOPNOTSUPP; > > @@ -299,6 +314,105 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum) > return ERR_PTR(-EINVAL); > } > > +static int remove_xattr(struct ubifs_info *c, struct inode *host, > + struct inode *inode, const struct qstr *nm) > +{ > + int err; > + struct ubifs_inode *host_ui = ubifs_inode(host); > + struct ubifs_inode *ui = ubifs_inode(inode); > + struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1, > + .dirtied_ino_d = ALIGN(host_ui->data_len, 8) }; > + > + ubifs_assert(ui->data_len == inode->i_size); > + > + err = ubifs_budget_space(c, &req); > + if (err) > + return err; > + > + mutex_lock(&host_ui->ui_mutex); > + host->i_ctime = ubifs_current_time(host); > + host_ui->xattr_cnt -= 1; > + host_ui->xattr_size -= CALC_DENT_SIZE(nm->len); > + host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); > + host_ui->xattr_names -= nm->len; > + > + err = ubifs_jnl_delete_xattr(c, host, inode, nm); > + if (err) > + goto out_cancel; > + mutex_unlock(&host_ui->ui_mutex); > + > + ubifs_release_budget(c, &req); > + return 0; > + > +out_cancel: > + host_ui->xattr_cnt += 1; > + host_ui->xattr_size += CALC_DENT_SIZE(nm->len); > + host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len); > + mutex_unlock(&host_ui->ui_mutex); > + ubifs_release_budget(c, &req); > + make_bad_inode(inode); > + return err; > +} > + > +int ubifs_removexattr(struct dentry *dentry, const char *name) > +{ > + struct inode *inode, *host = d_inode(dentry); > + struct ubifs_info *c = host->i_sb->s_fs_info; > + struct qstr nm = QSTR_INIT(name, strlen(name)); > + struct ubifs_dent_node *xent; > + union ubifs_key key; > + int type, err; > + > + dbg_gen("xattr '%s', ino %lu ('%pd')", name, > + host->i_ino, dentry); > + ubifs_assert(mutex_is_locked(&host->i_mutex)); > + > + type = check_namespace(&nm); > + if (type < 0) > + return type; > + > + xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS); > + if (!xent) > + return -ENOMEM; > + > + xent_key_init(c, &key, host->i_ino, &nm); > + err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); > + if (err) { > + if (err == -ENOENT) > + err = -ENODATA; > + goto out_free; > + } > + > + inode = iget_xattr(c, le64_to_cpu(xent->inum)); > + if (IS_ERR(inode)) { > + err = PTR_ERR(inode); > + goto out_free; > + } > + > + ubifs_assert(inode->i_nlink == 1); > + clear_nlink(inode); > + err = remove_xattr(c, host, inode, &nm); > + if (err) > + set_nlink(inode, 1); > + > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + if (!err) { > + if (type == POSIX_ACL_DEFAULT) > + set_cached_acl(host, ACL_TYPE_DEFAULT, NULL); > + else if (type == POSIX_ACL_ACCESS) > + set_cached_acl(host, ACL_TYPE_ACCESS, NULL); > + } > +#endif > + > + /* If @i_nlink is 0, 'iput()' will delete the inode */ > + iput(inode); > + > +out_free: > + kfree(xent); > + return err; > +} > + > + Why move it? If you just want to use them before the definitions, Just declare them before using. > int ubifs_do_setxattr(struct inode *host, const char *name, > const void *value, size_t size, int flags) > { > @@ -348,7 +462,19 @@ int ubifs_do_setxattr(struct inode *host, const char *name, > goto out_free; > } > > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + if (size == 0) { > + ubifs_assert(inode->i_nlink == 1); > + clear_nlink(inode); > + err = remove_xattr(c, host, inode, &nm); > + if (err) > + set_nlink(inode, 1); > + iput(inode); > + goto out_free; > + } > +#endif Is there a testcase for it? > err = change_xattr(c, host, inode, value, size); > + > iput(inode); > > out_free: > @@ -359,6 +485,9 @@ out_free: > int ubifs_setxattr(struct dentry *dentry, const char *name, > const void *value, size_t size, int flags) > { > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + const struct xattr_handler *handler; > +#endif > struct qstr nm = QSTR_INIT(name, strlen(name)); > int type; > > @@ -369,6 +498,16 @@ int ubifs_setxattr(struct dentry *dentry, const char *name, > if (type < 0) > return type; > > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { > + if (type == POSIX_ACL_DEFAULT) > + handler = &posix_acl_default_xattr_handler; > + if (type == POSIX_ACL_ACCESS) > + handler = &posix_acl_access_xattr_handler; > + return handler->set(dentry, name, value, size, flags, > + handler->flags); > + } > +#endif What about setting sb->s_xattr and calling generic_setxattr() here? > return ubifs_do_setxattr(d_inode(dentry), name, value, size, flags); > } > > @@ -428,6 +567,9 @@ out_unlock: > ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, > void *value, size_t size) > { > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + const struct xattr_handler *handler; > +#endif > struct qstr nm = QSTR_INIT(name, strlen(name)); > int type; > > @@ -438,6 +580,16 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, > if (type < 0) > return type; > > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { > + if (type == POSIX_ACL_DEFAULT) > + handler = &posix_acl_default_xattr_handler; > + if (type == POSIX_ACL_ACCESS) > + handler = &posix_acl_access_xattr_handler; > + return handler->get(dentry, name, value, size, > + handler->flags); > + } > +#endif Ditto Thanx Yang > return ubifs_do_getxattr(d_inode(dentry), name, value, size); > } > > @@ -505,95 +657,6 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) > return written; > } > > -static int remove_xattr(struct ubifs_info *c, struct inode *host, > - struct inode *inode, const struct qstr *nm) > -{ > - int err; > - struct ubifs_inode *host_ui = ubifs_inode(host); > - struct ubifs_inode *ui = ubifs_inode(inode); > - struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1, > - .dirtied_ino_d = ALIGN(host_ui->data_len, 8) }; > - > - ubifs_assert(ui->data_len == inode->i_size); > - > - err = ubifs_budget_space(c, &req); > - if (err) > - return err; > - > - mutex_lock(&host_ui->ui_mutex); > - host->i_ctime = ubifs_current_time(host); > - host_ui->xattr_cnt -= 1; > - host_ui->xattr_size -= CALC_DENT_SIZE(nm->len); > - host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); > - host_ui->xattr_names -= nm->len; > - > - err = ubifs_jnl_delete_xattr(c, host, inode, nm); > - if (err) > - goto out_cancel; > - mutex_unlock(&host_ui->ui_mutex); > - > - ubifs_release_budget(c, &req); > - return 0; > - > -out_cancel: > - host_ui->xattr_cnt += 1; > - host_ui->xattr_size += CALC_DENT_SIZE(nm->len); > - host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len); > - mutex_unlock(&host_ui->ui_mutex); > - ubifs_release_budget(c, &req); > - make_bad_inode(inode); > - return err; > -} > - > -int ubifs_removexattr(struct dentry *dentry, const char *name) > -{ > - struct inode *inode, *host = d_inode(dentry); > - struct ubifs_info *c = host->i_sb->s_fs_info; > - struct qstr nm = QSTR_INIT(name, strlen(name)); > - struct ubifs_dent_node *xent; > - union ubifs_key key; > - int err; > - > - dbg_gen("xattr '%s', ino %lu ('%pd')", name, > - host->i_ino, dentry); > - ubifs_assert(mutex_is_locked(&host->i_mutex)); > - > - err = check_namespace(&nm); > - if (err < 0) > - return err; > - > - xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS); > - if (!xent) > - return -ENOMEM; > - > - xent_key_init(c, &key, host->i_ino, &nm); > - err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); > - if (err) { > - if (err == -ENOENT) > - err = -ENODATA; > - goto out_free; > - } > - > - inode = iget_xattr(c, le64_to_cpu(xent->inum)); > - if (IS_ERR(inode)) { > - err = PTR_ERR(inode); > - goto out_free; > - } > - > - ubifs_assert(inode->i_nlink == 1); > - clear_nlink(inode); > - err = remove_xattr(c, host, inode, &nm); > - if (err) > - set_nlink(inode, 1); > - > - /* If @i_nlink is 0, 'iput()' will delete the inode */ > - iput(inode); > - > -out_free: > - kfree(xent); > - return err; > -} > - > static size_t security_listxattr(struct dentry *d, char *list, size_t list_size, > const char *name, size_t name_len, int flags) > { > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Dongsheng On 9/11/2015 1:01 PM, Dongsheng Yang wrote: > On 09/11/2015 05:09 PM, Sheng Yong wrote: [...] > > Why move it? If you just want to use them before the definitions, > Just declare them before using. OK. >> int ubifs_do_setxattr(struct inode *host, const char *name, >> const void *value, size_t size, int flags) >> { >> @@ -348,7 +462,19 @@ int ubifs_do_setxattr(struct inode *host, const char *name, >> goto out_free; >> } >> >> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >> + if (size == 0) { >> + ubifs_assert(inode->i_nlink == 1); >> + clear_nlink(inode); >> + err = remove_xattr(c, host, inode, &nm); >> + if (err) >> + set_nlink(inode, 1); >> + iput(inode); >> + goto out_free; >> + } >> +#endif > > Is there a testcase for it? I test `setfacl -b/-k'. I don't know how setfacl is implemented. But for ACL_TYPE_ACCESS, ubifs_setxattr() is called with size = 0 and value = NULL; while for ACL_TYPE_DEFAULT, ubifs_removexattr() is called. >> err = change_xattr(c, host, inode, value, size); >> + >> iput(inode); >> >> out_free: >> @@ -359,6 +485,9 @@ out_free: >> int ubifs_setxattr(struct dentry *dentry, const char *name, >> const void *value, size_t size, int flags) >> { >> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >> + const struct xattr_handler *handler; >> +#endif >> struct qstr nm = QSTR_INIT(name, strlen(name)); >> int type; >> >> @@ -369,6 +498,16 @@ int ubifs_setxattr(struct dentry *dentry, const char *name, >> if (type < 0) >> return type; >> >> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >> + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { >> + if (type == POSIX_ACL_DEFAULT) >> + handler = &posix_acl_default_xattr_handler; >> + if (type == POSIX_ACL_ACCESS) >> + handler = &posix_acl_access_xattr_handler; >> + return handler->set(dentry, name, value, size, flags, >> + handler->flags); >> + } >> +#endif > > What about setting sb->s_xattr and calling generic_setxattr() here? I have no idea if we should do this :( If we do, I think, we should call generic functions for all xattr. thanks, Sheng >> return ubifs_do_setxattr(d_inode(dentry), name, value, size, flags); >> } >> >> @@ -428,6 +567,9 @@ out_unlock: >> ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, >> void *value, size_t size) >> { >> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >> + const struct xattr_handler *handler; >> +#endif >> struct qstr nm = QSTR_INIT(name, strlen(name)); >> int type; >> >> @@ -438,6 +580,16 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, >> if (type < 0) >> return type; >> >> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >> + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { >> + if (type == POSIX_ACL_DEFAULT) >> + handler = &posix_acl_default_xattr_handler; >> + if (type == POSIX_ACL_ACCESS) >> + handler = &posix_acl_access_xattr_handler; >> + return handler->get(dentry, name, value, size, >> + handler->flags); >> + } >> +#endif > > Ditto > > Thanx > Yang >> return ubifs_do_getxattr(d_inode(dentry), name, value, size); >> } >> >> @@ -505,95 +657,6 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) >> return written; >> } >> >> -static int remove_xattr(struct ubifs_info *c, struct inode *host, >> - struct inode *inode, const struct qstr *nm) >> -{ >> - int err; >> - struct ubifs_inode *host_ui = ubifs_inode(host); >> - struct ubifs_inode *ui = ubifs_inode(inode); >> - struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1, >> - .dirtied_ino_d = ALIGN(host_ui->data_len, 8) }; >> - >> - ubifs_assert(ui->data_len == inode->i_size); >> - >> - err = ubifs_budget_space(c, &req); >> - if (err) >> - return err; >> - >> - mutex_lock(&host_ui->ui_mutex); >> - host->i_ctime = ubifs_current_time(host); >> - host_ui->xattr_cnt -= 1; >> - host_ui->xattr_size -= CALC_DENT_SIZE(nm->len); >> - host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); >> - host_ui->xattr_names -= nm->len; >> - >> - err = ubifs_jnl_delete_xattr(c, host, inode, nm); >> - if (err) >> - goto out_cancel; >> - mutex_unlock(&host_ui->ui_mutex); >> - >> - ubifs_release_budget(c, &req); >> - return 0; >> - >> -out_cancel: >> - host_ui->xattr_cnt += 1; >> - host_ui->xattr_size += CALC_DENT_SIZE(nm->len); >> - host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len); >> - mutex_unlock(&host_ui->ui_mutex); >> - ubifs_release_budget(c, &req); >> - make_bad_inode(inode); >> - return err; >> -} >> - >> -int ubifs_removexattr(struct dentry *dentry, const char *name) >> -{ >> - struct inode *inode, *host = d_inode(dentry); >> - struct ubifs_info *c = host->i_sb->s_fs_info; >> - struct qstr nm = QSTR_INIT(name, strlen(name)); >> - struct ubifs_dent_node *xent; >> - union ubifs_key key; >> - int err; >> - >> - dbg_gen("xattr '%s', ino %lu ('%pd')", name, >> - host->i_ino, dentry); >> - ubifs_assert(mutex_is_locked(&host->i_mutex)); >> - >> - err = check_namespace(&nm); >> - if (err < 0) >> - return err; >> - >> - xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS); >> - if (!xent) >> - return -ENOMEM; >> - >> - xent_key_init(c, &key, host->i_ino, &nm); >> - err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); >> - if (err) { >> - if (err == -ENOENT) >> - err = -ENODATA; >> - goto out_free; >> - } >> - >> - inode = iget_xattr(c, le64_to_cpu(xent->inum)); >> - if (IS_ERR(inode)) { >> - err = PTR_ERR(inode); >> - goto out_free; >> - } >> - >> - ubifs_assert(inode->i_nlink == 1); >> - clear_nlink(inode); >> - err = remove_xattr(c, host, inode, &nm); >> - if (err) >> - set_nlink(inode, 1); >> - >> - /* If @i_nlink is 0, 'iput()' will delete the inode */ >> - iput(inode); >> - >> -out_free: >> - kfree(xent); >> - return err; >> -} >> - >> static size_t security_listxattr(struct dentry *d, char *list, size_t list_size, >> const char *name, size_t name_len, int flags) >> { >> > > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2015 02:18 PM, Sheng Yong wrote: > Hi, Dongsheng > > On 9/11/2015 1:01 PM, Dongsheng Yang wrote: >> On 09/11/2015 05:09 PM, Sheng Yong wrote: > [...] >> >> Why move it? If you just want to use them before the definitions, >> Just declare them before using. > > OK. >>> int ubifs_do_setxattr(struct inode *host, const char *name, >>> const void *value, size_t size, int flags) >>> { >>> @@ -348,7 +462,19 @@ int ubifs_do_setxattr(struct inode *host, const char *name, >>> goto out_free; >>> } >>> >>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>> + if (size == 0) { >>> + ubifs_assert(inode->i_nlink == 1); >>> + clear_nlink(inode); >>> + err = remove_xattr(c, host, inode, &nm); >>> + if (err) >>> + set_nlink(inode, 1); >>> + iput(inode); >>> + goto out_free; >>> + } >>> +#endif >> >> Is there a testcase for it? > > I test `setfacl -b/-k'. I don't know how setfacl is implemented. But for ACL_TYPE_ACCESS, > ubifs_setxattr() is called with size = 0 and value = NULL; while for ACL_TYPE_DEFAULT, > ubifs_removexattr() is called. I mean, if we don't add the code above, what kind of problem we would meet? > >>> err = change_xattr(c, host, inode, value, size); >>> + >>> iput(inode); >>> >>> out_free: >>> @@ -359,6 +485,9 @@ out_free: >>> int ubifs_setxattr(struct dentry *dentry, const char *name, >>> const void *value, size_t size, int flags) >>> { >>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>> + const struct xattr_handler *handler; >>> +#endif >>> struct qstr nm = QSTR_INIT(name, strlen(name)); >>> int type; >>> >>> @@ -369,6 +498,16 @@ int ubifs_setxattr(struct dentry *dentry, const char *name, >>> if (type < 0) >>> return type; >>> >>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>> + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { >>> + if (type == POSIX_ACL_DEFAULT) >>> + handler = &posix_acl_default_xattr_handler; >>> + if (type == POSIX_ACL_ACCESS) >>> + handler = &posix_acl_access_xattr_handler; >>> + return handler->set(dentry, name, value, size, flags, >>> + handler->flags); >>> + } >>> +#endif >> >> What about setting sb->s_xattr and calling generic_setxattr() here? > > I have no idea if we should do this :( > If we do, I think, we should call generic functions for all xattr. No, only for POSIX_ACL_DEFAULT|POSIX_ACL_ACCESS currently. Then Something like that: if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) return generic_setxattr(dentry, name, value, size, flags); Yang > > thanks, > Sheng > >>> return ubifs_do_setxattr(d_inode(dentry), name, value, size, flags); >>> } >>> >>> @@ -428,6 +567,9 @@ out_unlock: >>> ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, >>> void *value, size_t size) >>> { >>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>> + const struct xattr_handler *handler; >>> +#endif >>> struct qstr nm = QSTR_INIT(name, strlen(name)); >>> int type; >>> >>> @@ -438,6 +580,16 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, >>> if (type < 0) >>> return type; >>> >>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>> + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { >>> + if (type == POSIX_ACL_DEFAULT) >>> + handler = &posix_acl_default_xattr_handler; >>> + if (type == POSIX_ACL_ACCESS) >>> + handler = &posix_acl_access_xattr_handler; >>> + return handler->get(dentry, name, value, size, >>> + handler->flags); >>> + } >>> +#endif >> >> Ditto >> >> Thanx >> Yang >>> return ubifs_do_getxattr(d_inode(dentry), name, value, size); >>> } >>> >>> @@ -505,95 +657,6 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) >>> return written; >>> } >>> >>> -static int remove_xattr(struct ubifs_info *c, struct inode *host, >>> - struct inode *inode, const struct qstr *nm) >>> -{ >>> - int err; >>> - struct ubifs_inode *host_ui = ubifs_inode(host); >>> - struct ubifs_inode *ui = ubifs_inode(inode); >>> - struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1, >>> - .dirtied_ino_d = ALIGN(host_ui->data_len, 8) }; >>> - >>> - ubifs_assert(ui->data_len == inode->i_size); >>> - >>> - err = ubifs_budget_space(c, &req); >>> - if (err) >>> - return err; >>> - >>> - mutex_lock(&host_ui->ui_mutex); >>> - host->i_ctime = ubifs_current_time(host); >>> - host_ui->xattr_cnt -= 1; >>> - host_ui->xattr_size -= CALC_DENT_SIZE(nm->len); >>> - host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); >>> - host_ui->xattr_names -= nm->len; >>> - >>> - err = ubifs_jnl_delete_xattr(c, host, inode, nm); >>> - if (err) >>> - goto out_cancel; >>> - mutex_unlock(&host_ui->ui_mutex); >>> - >>> - ubifs_release_budget(c, &req); >>> - return 0; >>> - >>> -out_cancel: >>> - host_ui->xattr_cnt += 1; >>> - host_ui->xattr_size += CALC_DENT_SIZE(nm->len); >>> - host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len); >>> - mutex_unlock(&host_ui->ui_mutex); >>> - ubifs_release_budget(c, &req); >>> - make_bad_inode(inode); >>> - return err; >>> -} >>> - >>> -int ubifs_removexattr(struct dentry *dentry, const char *name) >>> -{ >>> - struct inode *inode, *host = d_inode(dentry); >>> - struct ubifs_info *c = host->i_sb->s_fs_info; >>> - struct qstr nm = QSTR_INIT(name, strlen(name)); >>> - struct ubifs_dent_node *xent; >>> - union ubifs_key key; >>> - int err; >>> - >>> - dbg_gen("xattr '%s', ino %lu ('%pd')", name, >>> - host->i_ino, dentry); >>> - ubifs_assert(mutex_is_locked(&host->i_mutex)); >>> - >>> - err = check_namespace(&nm); >>> - if (err < 0) >>> - return err; >>> - >>> - xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS); >>> - if (!xent) >>> - return -ENOMEM; >>> - >>> - xent_key_init(c, &key, host->i_ino, &nm); >>> - err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); >>> - if (err) { >>> - if (err == -ENOENT) >>> - err = -ENODATA; >>> - goto out_free; >>> - } >>> - >>> - inode = iget_xattr(c, le64_to_cpu(xent->inum)); >>> - if (IS_ERR(inode)) { >>> - err = PTR_ERR(inode); >>> - goto out_free; >>> - } >>> - >>> - ubifs_assert(inode->i_nlink == 1); >>> - clear_nlink(inode); >>> - err = remove_xattr(c, host, inode, &nm); >>> - if (err) >>> - set_nlink(inode, 1); >>> - >>> - /* If @i_nlink is 0, 'iput()' will delete the inode */ >>> - iput(inode); >>> - >>> -out_free: >>> - kfree(xent); >>> - return err; >>> -} >>> - >>> static size_t security_listxattr(struct dentry *d, char *list, size_t list_size, >>> const char *name, size_t name_len, int flags) >>> { >>> >> >> >> . >> > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9/11/2015 2:25 PM, Dongsheng Yang wrote: > On 09/11/2015 02:18 PM, Sheng Yong wrote: >> Hi, Dongsheng >> >> On 9/11/2015 1:01 PM, Dongsheng Yang wrote: >>> On 09/11/2015 05:09 PM, Sheng Yong wrote: >> [...] >>> >>> Why move it? If you just want to use them before the definitions, >>> Just declare them before using. >> >> OK. >>>> int ubifs_do_setxattr(struct inode *host, const char *name, >>>> const void *value, size_t size, int flags) >>>> { >>>> @@ -348,7 +462,19 @@ int ubifs_do_setxattr(struct inode *host, const char *name, >>>> goto out_free; >>>> } >>>> >>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>> + if (size == 0) { >>>> + ubifs_assert(inode->i_nlink == 1); >>>> + clear_nlink(inode); >>>> + err = remove_xattr(c, host, inode, &nm); >>>> + if (err) >>>> + set_nlink(inode, 1); >>>> + iput(inode); >>>> + goto out_free; >>>> + } >>>> +#endif >>> >>> Is there a testcase for it? >> >> I test `setfacl -b/-k'. I don't know how setfacl is implemented. But for ACL_TYPE_ACCESS, >> ubifs_setxattr() is called with size = 0 and value = NULL; while for ACL_TYPE_DEFAULT, >> ubifs_removexattr() is called. > > I mean, if we don't add the code above, what kind of problem > we would meet? Then, a new xattr wich length 0 and value NULL, is written to flash, which is useless. I don't think we should keep this xattr. >> >>>> err = change_xattr(c, host, inode, value, size); >>>> + >>>> iput(inode); >>>> >>>> out_free: >>>> @@ -359,6 +485,9 @@ out_free: >>>> int ubifs_setxattr(struct dentry *dentry, const char *name, >>>> const void *value, size_t size, int flags) >>>> { >>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>> + const struct xattr_handler *handler; >>>> +#endif >>>> struct qstr nm = QSTR_INIT(name, strlen(name)); >>>> int type; >>>> >>>> @@ -369,6 +498,16 @@ int ubifs_setxattr(struct dentry *dentry, const char *name, >>>> if (type < 0) >>>> return type; >>>> >>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>> + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { >>>> + if (type == POSIX_ACL_DEFAULT) >>>> + handler = &posix_acl_default_xattr_handler; >>>> + if (type == POSIX_ACL_ACCESS) >>>> + handler = &posix_acl_access_xattr_handler; >>>> + return handler->set(dentry, name, value, size, flags, >>>> + handler->flags); >>>> + } >>>> +#endif >>> >>> What about setting sb->s_xattr and calling generic_setxattr() here? >> >> I have no idea if we should do this :( >> If we do, I think, we should call generic functions for all xattr. > I copied the xattr handler discussion from the other email, so that we could have it at one place :) > No, only for POSIX_ACL_DEFAULT|POSIX_ACL_ACCESS currently. Then > Something like that: > > if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) > return generic_setxattr(dentry, name, value, size, flags); Acutally, in my v1 patchset, I did like this. The URL is: http://lists.infradead.org/pipermail/linux-mtd/2015-March/058451.html >Security handler is different with acl handler. Please read the >security_getxattr(), it just return ubifs_getxattr(). That means >we can use ubifs_getxattr() immediately. So we can remove the >security_handler for ubifs. > >But acl is different, we need to call a special ubifs_get_acl() >here. Then I found you get the handler by youself in the >ubifs_get|set_xattr() and call handler->set() by youself. Right, they are different. I mean using the generic way means ubifs_[get|set]xattr should be modified. > >That's not good idea. Please Just set the handler and call >generic_setxattr(). But IMO, calling generic_setxattr means we have to add posix_acl_[access|default]_xattr_handler in ubifs_xattr_handlers, like const struct xattr_handler *ubifs_xattr_handlers[] = { &ubifs_xattr_security_handler, +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + &posix_acl_access_xattr_handler, + &posix_acl_default_xattr_handler, +#endif NULL, }; then, generic_setxattr() could call posix_acl_***_xattr_handler.set(), which will call ubifs_set_acl() to do the real setting. Since security xattr dead code is going to be removed, ubifs_xattr_handlers may go away too. So I agree with Andreas that if we could add all xattr handlers in ubifs_xattr_handlers (which means ubifs_[get|set]xattr should be modified and generic_[get|set]xattr should be set in inode_operations, e.g ext4, jffs2), it's better to use generic_setxattr. I'm not sure whether we should convert ubifs to use the xattr handler infrastructure, like what Andreas said. thanks, Sheng > > Yang >> >> thanks, >> Sheng >> >>>> return ubifs_do_setxattr(d_inode(dentry), name, value, size, flags); >>>> } >>>> >>>> @@ -428,6 +567,9 @@ out_unlock: >>>> ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, >>>> void *value, size_t size) >>>> { >>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>> + const struct xattr_handler *handler; >>>> +#endif >>>> struct qstr nm = QSTR_INIT(name, strlen(name)); >>>> int type; >>>> >>>> @@ -438,6 +580,16 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, >>>> if (type < 0) >>>> return type; >>>> >>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>> + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { >>>> + if (type == POSIX_ACL_DEFAULT) >>>> + handler = &posix_acl_default_xattr_handler; >>>> + if (type == POSIX_ACL_ACCESS) >>>> + handler = &posix_acl_access_xattr_handler; >>>> + return handler->get(dentry, name, value, size, >>>> + handler->flags); >>>> + } >>>> +#endif >>> >>> Ditto >>> >>> Thanx >>> Yang >>>> return ubifs_do_getxattr(d_inode(dentry), name, value, size); >>>> } >>>> >>>> @@ -505,95 +657,6 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) >>>> return written; >>>> } >>>> >>>> -static int remove_xattr(struct ubifs_info *c, struct inode *host, >>>> - struct inode *inode, const struct qstr *nm) >>>> -{ >>>> - int err; >>>> - struct ubifs_inode *host_ui = ubifs_inode(host); >>>> - struct ubifs_inode *ui = ubifs_inode(inode); >>>> - struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1, >>>> - .dirtied_ino_d = ALIGN(host_ui->data_len, 8) }; >>>> - >>>> - ubifs_assert(ui->data_len == inode->i_size); >>>> - >>>> - err = ubifs_budget_space(c, &req); >>>> - if (err) >>>> - return err; >>>> - >>>> - mutex_lock(&host_ui->ui_mutex); >>>> - host->i_ctime = ubifs_current_time(host); >>>> - host_ui->xattr_cnt -= 1; >>>> - host_ui->xattr_size -= CALC_DENT_SIZE(nm->len); >>>> - host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); >>>> - host_ui->xattr_names -= nm->len; >>>> - >>>> - err = ubifs_jnl_delete_xattr(c, host, inode, nm); >>>> - if (err) >>>> - goto out_cancel; >>>> - mutex_unlock(&host_ui->ui_mutex); >>>> - >>>> - ubifs_release_budget(c, &req); >>>> - return 0; >>>> - >>>> -out_cancel: >>>> - host_ui->xattr_cnt += 1; >>>> - host_ui->xattr_size += CALC_DENT_SIZE(nm->len); >>>> - host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len); >>>> - mutex_unlock(&host_ui->ui_mutex); >>>> - ubifs_release_budget(c, &req); >>>> - make_bad_inode(inode); >>>> - return err; >>>> -} >>>> - >>>> -int ubifs_removexattr(struct dentry *dentry, const char *name) >>>> -{ >>>> - struct inode *inode, *host = d_inode(dentry); >>>> - struct ubifs_info *c = host->i_sb->s_fs_info; >>>> - struct qstr nm = QSTR_INIT(name, strlen(name)); >>>> - struct ubifs_dent_node *xent; >>>> - union ubifs_key key; >>>> - int err; >>>> - >>>> - dbg_gen("xattr '%s', ino %lu ('%pd')", name, >>>> - host->i_ino, dentry); >>>> - ubifs_assert(mutex_is_locked(&host->i_mutex)); >>>> - >>>> - err = check_namespace(&nm); >>>> - if (err < 0) >>>> - return err; >>>> - >>>> - xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS); >>>> - if (!xent) >>>> - return -ENOMEM; >>>> - >>>> - xent_key_init(c, &key, host->i_ino, &nm); >>>> - err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); >>>> - if (err) { >>>> - if (err == -ENOENT) >>>> - err = -ENODATA; >>>> - goto out_free; >>>> - } >>>> - >>>> - inode = iget_xattr(c, le64_to_cpu(xent->inum)); >>>> - if (IS_ERR(inode)) { >>>> - err = PTR_ERR(inode); >>>> - goto out_free; >>>> - } >>>> - >>>> - ubifs_assert(inode->i_nlink == 1); >>>> - clear_nlink(inode); >>>> - err = remove_xattr(c, host, inode, &nm); >>>> - if (err) >>>> - set_nlink(inode, 1); >>>> - >>>> - /* If @i_nlink is 0, 'iput()' will delete the inode */ >>>> - iput(inode); >>>> - >>>> -out_free: >>>> - kfree(xent); >>>> - return err; >>>> -} >>>> - >>>> static size_t security_listxattr(struct dentry *d, char *list, size_t list_size, >>>> const char *name, size_t name_len, int flags) >>>> { >>>> >>> >>> >>> . >>> >> >> . >> > > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/12/2015 09:15 AM, Sheng Yong wrote: > > > On 9/11/2015 2:25 PM, Dongsheng Yang wrote: >> On 09/11/2015 02:18 PM, Sheng Yong wrote: >>> Hi, Dongsheng >>> >>> On 9/11/2015 1:01 PM, Dongsheng Yang wrote: >>>> On 09/11/2015 05:09 PM, Sheng Yong wrote: >>> [...] >>>> >>>> Why move it? If you just want to use them before the definitions, >>>> Just declare them before using. >>> >>> OK. >>>>> int ubifs_do_setxattr(struct inode *host, const char *name, >>>>> const void *value, size_t size, int flags) >>>>> { >>>>> @@ -348,7 +462,19 @@ int ubifs_do_setxattr(struct inode *host, const char *name, >>>>> goto out_free; >>>>> } >>>>> >>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>>> + if (size == 0) { >>>>> + ubifs_assert(inode->i_nlink == 1); >>>>> + clear_nlink(inode); >>>>> + err = remove_xattr(c, host, inode, &nm); >>>>> + if (err) >>>>> + set_nlink(inode, 1); >>>>> + iput(inode); >>>>> + goto out_free; >>>>> + } >>>>> +#endif >>>> >>>> Is there a testcase for it? >>> >>> I test `setfacl -b/-k'. I don't know how setfacl is implemented. But for ACL_TYPE_ACCESS, >>> ubifs_setxattr() is called with size = 0 and value = NULL; while for ACL_TYPE_DEFAULT, >>> ubifs_removexattr() is called. >> >> I mean, if we don't add the code above, what kind of problem >> we would meet? > > Then, a new xattr wich length 0 and value NULL, is written to flash, which is useless. > I don't think we should keep this xattr. > >>> >>>>> err = change_xattr(c, host, inode, value, size); >>>>> + >>>>> iput(inode); >>>>> >>>>> out_free: >>>>> @@ -359,6 +485,9 @@ out_free: >>>>> int ubifs_setxattr(struct dentry *dentry, const char *name, >>>>> const void *value, size_t size, int flags) >>>>> { >>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>>> + const struct xattr_handler *handler; >>>>> +#endif >>>>> struct qstr nm = QSTR_INIT(name, strlen(name)); >>>>> int type; >>>>> >>>>> @@ -369,6 +498,16 @@ int ubifs_setxattr(struct dentry *dentry, const char *name, >>>>> if (type < 0) >>>>> return type; >>>>> >>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>>> + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { >>>>> + if (type == POSIX_ACL_DEFAULT) >>>>> + handler = &posix_acl_default_xattr_handler; >>>>> + if (type == POSIX_ACL_ACCESS) >>>>> + handler = &posix_acl_access_xattr_handler; >>>>> + return handler->set(dentry, name, value, size, flags, >>>>> + handler->flags); >>>>> + } >>>>> +#endif >>>> >>>> What about setting sb->s_xattr and calling generic_setxattr() here? >>> >>> I have no idea if we should do this :( >>> If we do, I think, we should call generic functions for all xattr. >> > > I copied the xattr handler discussion from the other email, so that > we could have it at one place :) That's great :) > >> No, only for POSIX_ACL_DEFAULT|POSIX_ACL_ACCESS currently. Then >> Something like that: >> >> if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) >> return generic_setxattr(dentry, name, value, size, flags); > > Acutally, in my v1 patchset, I did like this. The URL is: > http://lists.infradead.org/pipermail/linux-mtd/2015-March/058451.html > >> Security handler is different with acl handler. Please read the >> security_getxattr(), it just return ubifs_getxattr(). That means >> we can use ubifs_getxattr() immediately. So we can remove the >> security_handler for ubifs. >> >> But acl is different, we need to call a special ubifs_get_acl() >> here. Then I found you get the handler by youself in the >> ubifs_get|set_xattr() and call handler->set() by youself. > > Right, they are different. I mean using the generic way means > ubifs_[get|set]xattr should be modified. > >> >> That's not good idea. Please Just set the handler and call >> generic_setxattr(). > > But IMO, calling generic_setxattr means we have to add > posix_acl_[access|default]_xattr_handler in ubifs_xattr_handlers, like > > const struct xattr_handler *ubifs_xattr_handlers[] = { > &ubifs_xattr_security_handler, > +#ifdef CONFIG_UBIFS_FS_POSIX_ACL > + &posix_acl_access_xattr_handler, > + &posix_acl_default_xattr_handler, > +#endif > NULL, > }; Yes, obviously. > > then, generic_setxattr() could call posix_acl_***_xattr_handler.set(), > which will call ubifs_set_acl() to do the real setting. Since security > xattr dead code is going to be removed, ubifs_xattr_handlers may go > away too. So there would be a conflict with the other patch from Richard to remove ubifs_xattr_handlers. But that's not a problem at all, just address it when merging.We do not use security_handler in ubifs, but that does not mean we can not use sb->xattr in ubifs. > So I agree with Andreas that if we could add all xattr handlers > in ubifs_xattr_handlers (which means ubifs_[get|set]xattr should be > modified and generic_[get|set]xattr should be set in inode_operations, > e.g ext4, jffs2),it's better to use generic_setxattr. I'm not sure > whether we should convert ubifs to use the xattr handler infrastructure, > like what Andreas said. Okey, I think I got your meaning now. You are proposing something like jffs2. Let me try to make our discussion more clear. There are some proposals here. (1). What you did in this patch, get the handler for acl and call handler->get() in ubifs_getxattr(). I disagree with this idea. (2). What you suggest above to implement all handlers and assign inode_ops->get_xattr with generic_getxattr().I think there would be a lot of duplication in this implementation. E.g, Why we need a check_namespace() in ubifs? That would solve the duplicated checking. So we need much more code to implement this idea and these code are almost same. (3). What you did in V1. Use xattr handler for necessary, currently only for acl handlers. Same with what Btrfs did. Hi Sheng, I vote (3). Thanx Yang > > thanks, > Sheng > >> >> Yang >>> >>> thanks, >>> Sheng >>> >>>>> return ubifs_do_setxattr(d_inode(dentry), name, value, size, flags); >>>>> } >>>>> >>>>> @@ -428,6 +567,9 @@ out_unlock: >>>>> ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, >>>>> void *value, size_t size) >>>>> { >>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>>> + const struct xattr_handler *handler; >>>>> +#endif >>>>> struct qstr nm = QSTR_INIT(name, strlen(name)); >>>>> int type; >>>>> >>>>> @@ -438,6 +580,16 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, >>>>> if (type < 0) >>>>> return type; >>>>> >>>>> +#ifdef CONFIG_UBIFS_FS_POSIX_ACL >>>>> + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { >>>>> + if (type == POSIX_ACL_DEFAULT) >>>>> + handler = &posix_acl_default_xattr_handler; >>>>> + if (type == POSIX_ACL_ACCESS) >>>>> + handler = &posix_acl_access_xattr_handler; >>>>> + return handler->get(dentry, name, value, size, >>>>> + handler->flags); >>>>> + } >>>>> +#endif >>>> >>>> Ditto >>>> >>>> Thanx >>>> Yang >>>>> return ubifs_do_getxattr(d_inode(dentry), name, value, size); >>>>> } >>>>> >>>>> @@ -505,95 +657,6 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) >>>>> return written; >>>>> } >>>>> >>>>> -static int remove_xattr(struct ubifs_info *c, struct inode *host, >>>>> - struct inode *inode, const struct qstr *nm) >>>>> -{ >>>>> - int err; >>>>> - struct ubifs_inode *host_ui = ubifs_inode(host); >>>>> - struct ubifs_inode *ui = ubifs_inode(inode); >>>>> - struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1, >>>>> - .dirtied_ino_d = ALIGN(host_ui->data_len, 8) }; >>>>> - >>>>> - ubifs_assert(ui->data_len == inode->i_size); >>>>> - >>>>> - err = ubifs_budget_space(c, &req); >>>>> - if (err) >>>>> - return err; >>>>> - >>>>> - mutex_lock(&host_ui->ui_mutex); >>>>> - host->i_ctime = ubifs_current_time(host); >>>>> - host_ui->xattr_cnt -= 1; >>>>> - host_ui->xattr_size -= CALC_DENT_SIZE(nm->len); >>>>> - host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); >>>>> - host_ui->xattr_names -= nm->len; >>>>> - >>>>> - err = ubifs_jnl_delete_xattr(c, host, inode, nm); >>>>> - if (err) >>>>> - goto out_cancel; >>>>> - mutex_unlock(&host_ui->ui_mutex); >>>>> - >>>>> - ubifs_release_budget(c, &req); >>>>> - return 0; >>>>> - >>>>> -out_cancel: >>>>> - host_ui->xattr_cnt += 1; >>>>> - host_ui->xattr_size += CALC_DENT_SIZE(nm->len); >>>>> - host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len); >>>>> - mutex_unlock(&host_ui->ui_mutex); >>>>> - ubifs_release_budget(c, &req); >>>>> - make_bad_inode(inode); >>>>> - return err; >>>>> -} >>>>> - >>>>> -int ubifs_removexattr(struct dentry *dentry, const char *name) >>>>> -{ >>>>> - struct inode *inode, *host = d_inode(dentry); >>>>> - struct ubifs_info *c = host->i_sb->s_fs_info; >>>>> - struct qstr nm = QSTR_INIT(name, strlen(name)); >>>>> - struct ubifs_dent_node *xent; >>>>> - union ubifs_key key; >>>>> - int err; >>>>> - >>>>> - dbg_gen("xattr '%s', ino %lu ('%pd')", name, >>>>> - host->i_ino, dentry); >>>>> - ubifs_assert(mutex_is_locked(&host->i_mutex)); >>>>> - >>>>> - err = check_namespace(&nm); >>>>> - if (err < 0) >>>>> - return err; >>>>> - >>>>> - xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS); >>>>> - if (!xent) >>>>> - return -ENOMEM; >>>>> - >>>>> - xent_key_init(c, &key, host->i_ino, &nm); >>>>> - err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); >>>>> - if (err) { >>>>> - if (err == -ENOENT) >>>>> - err = -ENODATA; >>>>> - goto out_free; >>>>> - } >>>>> - >>>>> - inode = iget_xattr(c, le64_to_cpu(xent->inum)); >>>>> - if (IS_ERR(inode)) { >>>>> - err = PTR_ERR(inode); >>>>> - goto out_free; >>>>> - } >>>>> - >>>>> - ubifs_assert(inode->i_nlink == 1); >>>>> - clear_nlink(inode); >>>>> - err = remove_xattr(c, host, inode, &nm); >>>>> - if (err) >>>>> - set_nlink(inode, 1); >>>>> - >>>>> - /* If @i_nlink is 0, 'iput()' will delete the inode */ >>>>> - iput(inode); >>>>> - >>>>> -out_free: >>>>> - kfree(xent); >>>>> - return err; >>>>> -} >>>>> - >>>>> static size_t security_listxattr(struct dentry *d, char *list, size_t list_size, >>>>> const char *name, size_t name_len, int flags) >>>>> { >>>>> >>>> >>>> >>>> . >>>> >>> >>> . >>> >> >> >> . >> > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c index 69ccc78..db5dd45 100644 --- a/fs/ubifs/dir.c +++ b/fs/ubifs/dir.c @@ -270,6 +270,10 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode, goto out_budg; } + err = ubifs_init_acl(dir, inode); + if (err) + goto out_inode; + err = ubifs_init_security(dir, inode, &dentry->d_name); if (err) goto out_inode; @@ -731,6 +735,10 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) goto out_budg; } + err = ubifs_init_acl(dir, inode); + if (err) + goto out_inode; + err = ubifs_init_security(dir, inode, &dentry->d_name); if (err) goto out_inode; @@ -810,6 +818,10 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry, goto out_budg; } + err = ubifs_init_acl(dir, inode); + if (err) + goto out_inode; + init_special_inode(inode, inode->i_mode, rdev); inode->i_size = ubifs_inode(inode)->ui_size = devlen; ui = ubifs_inode(inode); @@ -898,6 +910,10 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry, ui->data_len = len; inode->i_size = ubifs_inode(inode)->ui_size = len; + err = ubifs_init_acl(dir, inode); + if (err) + goto out_inode; + err = ubifs_init_security(dir, inode, &dentry->d_name); if (err) goto out_inode; diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c index 58298f8..74f4c63 100644 --- a/fs/ubifs/file.c +++ b/fs/ubifs/file.c @@ -52,6 +52,7 @@ #include "ubifs.h" #include <linux/mount.h> #include <linux/slab.h> +#include <linux/posix_acl.h> static int read_block(struct inode *inode, void *addr, unsigned int block, struct ubifs_data_node *dn) @@ -1246,6 +1247,11 @@ static int do_setattr(struct ubifs_info *c, struct inode *inode, mark_inode_dirty_sync(inode); mutex_unlock(&ui->ui_mutex); + if (attr->ia_valid & ATTR_MODE) + err = posix_acl_chmod(inode, inode->i_mode); + if (err) + return err; + if (release) ubifs_release_budget(c, &req); if (IS_SYNC(inode)) diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c index 6534b98..dad1070 100644 --- a/fs/ubifs/xattr.c +++ b/fs/ubifs/xattr.c @@ -52,7 +52,6 @@ * in the VFS inode cache. The xentries are cached in the LNC cache (see * tnc.c). * - * ACL support is not implemented. */ #include "ubifs.h" @@ -78,6 +77,10 @@ enum { USER_XATTR, TRUSTED_XATTR, SECURITY_XATTR, +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + POSIX_ACL_DEFAULT, + POSIX_ACL_ACCESS, +#endif }; static const struct inode_operations empty_iops; @@ -276,6 +279,18 @@ static int check_namespace(const struct qstr *nm) if (nm->name[sizeof(XATTR_SECURITY_PREFIX) - 1] == '\0') return -EINVAL; type = SECURITY_XATTR; +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + } else if (!strncmp(nm->name, XATTR_NAME_POSIX_ACL_DEFAULT, + sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1)) { + if (nm->name[sizeof(XATTR_NAME_POSIX_ACL_DEFAULT) - 1] != '\0') + return -EINVAL; + type = POSIX_ACL_DEFAULT; + } else if (!strncmp(nm->name, XATTR_NAME_POSIX_ACL_ACCESS, + sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1)) { + if (nm->name[sizeof(XATTR_NAME_POSIX_ACL_ACCESS) - 1] != '\0') + return -EINVAL; + type = POSIX_ACL_ACCESS; +#endif } else return -EOPNOTSUPP; @@ -299,6 +314,105 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum) return ERR_PTR(-EINVAL); } +static int remove_xattr(struct ubifs_info *c, struct inode *host, + struct inode *inode, const struct qstr *nm) +{ + int err; + struct ubifs_inode *host_ui = ubifs_inode(host); + struct ubifs_inode *ui = ubifs_inode(inode); + struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1, + .dirtied_ino_d = ALIGN(host_ui->data_len, 8) }; + + ubifs_assert(ui->data_len == inode->i_size); + + err = ubifs_budget_space(c, &req); + if (err) + return err; + + mutex_lock(&host_ui->ui_mutex); + host->i_ctime = ubifs_current_time(host); + host_ui->xattr_cnt -= 1; + host_ui->xattr_size -= CALC_DENT_SIZE(nm->len); + host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); + host_ui->xattr_names -= nm->len; + + err = ubifs_jnl_delete_xattr(c, host, inode, nm); + if (err) + goto out_cancel; + mutex_unlock(&host_ui->ui_mutex); + + ubifs_release_budget(c, &req); + return 0; + +out_cancel: + host_ui->xattr_cnt += 1; + host_ui->xattr_size += CALC_DENT_SIZE(nm->len); + host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len); + mutex_unlock(&host_ui->ui_mutex); + ubifs_release_budget(c, &req); + make_bad_inode(inode); + return err; +} + +int ubifs_removexattr(struct dentry *dentry, const char *name) +{ + struct inode *inode, *host = d_inode(dentry); + struct ubifs_info *c = host->i_sb->s_fs_info; + struct qstr nm = QSTR_INIT(name, strlen(name)); + struct ubifs_dent_node *xent; + union ubifs_key key; + int type, err; + + dbg_gen("xattr '%s', ino %lu ('%pd')", name, + host->i_ino, dentry); + ubifs_assert(mutex_is_locked(&host->i_mutex)); + + type = check_namespace(&nm); + if (type < 0) + return type; + + xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS); + if (!xent) + return -ENOMEM; + + xent_key_init(c, &key, host->i_ino, &nm); + err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); + if (err) { + if (err == -ENOENT) + err = -ENODATA; + goto out_free; + } + + inode = iget_xattr(c, le64_to_cpu(xent->inum)); + if (IS_ERR(inode)) { + err = PTR_ERR(inode); + goto out_free; + } + + ubifs_assert(inode->i_nlink == 1); + clear_nlink(inode); + err = remove_xattr(c, host, inode, &nm); + if (err) + set_nlink(inode, 1); + +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + if (!err) { + if (type == POSIX_ACL_DEFAULT) + set_cached_acl(host, ACL_TYPE_DEFAULT, NULL); + else if (type == POSIX_ACL_ACCESS) + set_cached_acl(host, ACL_TYPE_ACCESS, NULL); + } +#endif + + /* If @i_nlink is 0, 'iput()' will delete the inode */ + iput(inode); + +out_free: + kfree(xent); + return err; +} + + int ubifs_do_setxattr(struct inode *host, const char *name, const void *value, size_t size, int flags) { @@ -348,7 +462,19 @@ int ubifs_do_setxattr(struct inode *host, const char *name, goto out_free; } +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + if (size == 0) { + ubifs_assert(inode->i_nlink == 1); + clear_nlink(inode); + err = remove_xattr(c, host, inode, &nm); + if (err) + set_nlink(inode, 1); + iput(inode); + goto out_free; + } +#endif err = change_xattr(c, host, inode, value, size); + iput(inode); out_free: @@ -359,6 +485,9 @@ out_free: int ubifs_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags) { +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + const struct xattr_handler *handler; +#endif struct qstr nm = QSTR_INIT(name, strlen(name)); int type; @@ -369,6 +498,16 @@ int ubifs_setxattr(struct dentry *dentry, const char *name, if (type < 0) return type; +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { + if (type == POSIX_ACL_DEFAULT) + handler = &posix_acl_default_xattr_handler; + if (type == POSIX_ACL_ACCESS) + handler = &posix_acl_access_xattr_handler; + return handler->set(dentry, name, value, size, flags, + handler->flags); + } +#endif return ubifs_do_setxattr(d_inode(dentry), name, value, size, flags); } @@ -428,6 +567,9 @@ out_unlock: ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size) { +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + const struct xattr_handler *handler; +#endif struct qstr nm = QSTR_INIT(name, strlen(name)); int type; @@ -438,6 +580,16 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, if (type < 0) return type; +#ifdef CONFIG_UBIFS_FS_POSIX_ACL + if (type == POSIX_ACL_DEFAULT || type == POSIX_ACL_ACCESS) { + if (type == POSIX_ACL_DEFAULT) + handler = &posix_acl_default_xattr_handler; + if (type == POSIX_ACL_ACCESS) + handler = &posix_acl_access_xattr_handler; + return handler->get(dentry, name, value, size, + handler->flags); + } +#endif return ubifs_do_getxattr(d_inode(dentry), name, value, size); } @@ -505,95 +657,6 @@ ssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size) return written; } -static int remove_xattr(struct ubifs_info *c, struct inode *host, - struct inode *inode, const struct qstr *nm) -{ - int err; - struct ubifs_inode *host_ui = ubifs_inode(host); - struct ubifs_inode *ui = ubifs_inode(inode); - struct ubifs_budget_req req = { .dirtied_ino = 2, .mod_dent = 1, - .dirtied_ino_d = ALIGN(host_ui->data_len, 8) }; - - ubifs_assert(ui->data_len == inode->i_size); - - err = ubifs_budget_space(c, &req); - if (err) - return err; - - mutex_lock(&host_ui->ui_mutex); - host->i_ctime = ubifs_current_time(host); - host_ui->xattr_cnt -= 1; - host_ui->xattr_size -= CALC_DENT_SIZE(nm->len); - host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len); - host_ui->xattr_names -= nm->len; - - err = ubifs_jnl_delete_xattr(c, host, inode, nm); - if (err) - goto out_cancel; - mutex_unlock(&host_ui->ui_mutex); - - ubifs_release_budget(c, &req); - return 0; - -out_cancel: - host_ui->xattr_cnt += 1; - host_ui->xattr_size += CALC_DENT_SIZE(nm->len); - host_ui->xattr_size += CALC_XATTR_BYTES(ui->data_len); - mutex_unlock(&host_ui->ui_mutex); - ubifs_release_budget(c, &req); - make_bad_inode(inode); - return err; -} - -int ubifs_removexattr(struct dentry *dentry, const char *name) -{ - struct inode *inode, *host = d_inode(dentry); - struct ubifs_info *c = host->i_sb->s_fs_info; - struct qstr nm = QSTR_INIT(name, strlen(name)); - struct ubifs_dent_node *xent; - union ubifs_key key; - int err; - - dbg_gen("xattr '%s', ino %lu ('%pd')", name, - host->i_ino, dentry); - ubifs_assert(mutex_is_locked(&host->i_mutex)); - - err = check_namespace(&nm); - if (err < 0) - return err; - - xent = kmalloc(UBIFS_MAX_XENT_NODE_SZ, GFP_NOFS); - if (!xent) - return -ENOMEM; - - xent_key_init(c, &key, host->i_ino, &nm); - err = ubifs_tnc_lookup_nm(c, &key, xent, &nm); - if (err) { - if (err == -ENOENT) - err = -ENODATA; - goto out_free; - } - - inode = iget_xattr(c, le64_to_cpu(xent->inum)); - if (IS_ERR(inode)) { - err = PTR_ERR(inode); - goto out_free; - } - - ubifs_assert(inode->i_nlink == 1); - clear_nlink(inode); - err = remove_xattr(c, host, inode, &nm); - if (err) - set_nlink(inode, 1); - - /* If @i_nlink is 0, 'iput()' will delete the inode */ - iput(inode); - -out_free: - kfree(xent); - return err; -} - static size_t security_listxattr(struct dentry *d, char *list, size_t list_size, const char *name, size_t name_len, int flags) {
* initialize ACL when file is created * get ACL through ubifs_getxattr * set ACL through ubifs_setxattr. If the size of ACL is 0, the ACL is going to be removed * clear cached ACL in ubifs_removexattr if ACL xattr is removed * modify ACL if file mode is changed Signed-off-by: Sheng Yong <shengyong1@huawei.com> --- fs/ubifs/dir.c | 16 ++++ fs/ubifs/file.c | 6 ++ fs/ubifs/xattr.c | 243 ++++++++++++++++++++++++++++++++++--------------------- 3 files changed, 175 insertions(+), 90 deletions(-)