| Message ID | 20240319161646.2153867-3-lizetao1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | ubifs: Support POSIX Access Control Lists (ACLs) | expand |
在 2024/3/20 0:16, Li Zetao 写道: > There are two scenarios where ACL needs to be updated, the first one > is when creating the inode, and the second one is in the chmod process. > When creating directories/files/device node/tmpfile, ACLs needs to be > initialized, but symlink do not.Why not support symlink? It looks like many filesystems(eg. ext4, f2fs, btrfs) support it, except xfs. > > Signed-off-by: Li Zetao <lizetao1@huawei.com> > --- > fs/ubifs/dir.c | 16 ++++++++++++++++ > fs/ubifs/file.c | 4 ++++ > 2 files changed, 20 insertions(+) > > diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c > index 551148de66cd..dfb6823cc953 100644 > --- a/fs/ubifs/dir.c > +++ b/fs/ubifs/dir.c > @@ -316,6 +316,10 @@ static int ubifs_create(struct mnt_idmap *idmap, struct inode *dir, > goto out_fname; > } > > + err = ubifs_init_acl(inode, dir); > + if (err) > + goto out_inode; > + Attention, a new inconsistent problem point is imported by acl xattr creation. See https://bugzilla.kernel.org/show_bug.cgi?id=218309. @Richard > err = ubifs_init_security(dir, inode, &dentry->d_name); > if (err) > goto out_inode; > @@ -466,6 +470,10 @@ static int ubifs_tmpfile(struct mnt_idmap *idmap, struct inode *dir, > } > ui = ubifs_inode(inode); > > + err = ubifs_init_acl(inode, dir); > + if (err) > + goto out_inode; > + > err = ubifs_init_security(dir, inode, &dentry->d_name); > if (err) > goto out_inode; > @@ -1013,6 +1021,10 @@ static int ubifs_mkdir(struct mnt_idmap *idmap, struct inode *dir, > goto out_fname; > } > > + err = ubifs_init_acl(inode, dir); > + if (err) > + goto out_inode; > + > err = ubifs_init_security(dir, inode, &dentry->d_name); > if (err) > goto out_inode; > @@ -1108,6 +1120,10 @@ static int ubifs_mknod(struct mnt_idmap *idmap, struct inode *dir, > ui->data = dev; > ui->data_len = devlen; > > + err = ubifs_init_acl(inode, dir); > + if (err) > + goto out_inode; > + > err = ubifs_init_security(dir, inode, &dentry->d_name); > if (err) > goto out_inode; The whiteout inode is not set acl for rename(WHITEOUT) operation. It looks like many filesystems(eg. ext4, f2fs, btrfs) support it, except xfs. In my opinion, whiteout is a char dev, since char/block device is supported, why not support whiteout? If we support whiteout, we should make sure that the whiteout renameing operation is atomic[1]. But I cannot come up with an idea how to combine whiteout xattr(acl) creation and whiteout file creation into an atomic operation, just like problem mentioned in [2], [1] https://lore.kernel.org/linux-mtd/20211227032246.2886878-6-chengzhihao1@huawei.com/ [2] https://bugzilla.kernel.org/show_bug.cgi?id=218309 > diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c > index 5029eb3390a5..8f964f8b0f96 100644 > --- a/fs/ubifs/file.c > +++ b/fs/ubifs/file.c > @@ -41,6 +41,7 @@ > #include <linux/mount.h> > #include <linux/slab.h> > #include <linux/migrate.h> > +#include <linux/posix_acl.h> > > static int read_block(struct inode *inode, void *addr, unsigned int block, > struct ubifs_data_node *dn) > @@ -1298,6 +1299,9 @@ int ubifs_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > else > err = do_setattr(c, inode, attr); > > + if (!err && (attr->ia_valid & ATTR_MODE)) > + err = posix_acl_chmod(idmap, dentry, inode->i_mode); > + > return err; > } > >
Hi, On 2024/3/21 11:47, Zhihao Cheng wrote: > 在 2024/3/20 0:16, Li Zetao 写道: >> There are two scenarios where ACL needs to be updated, the first one >> is when creating the inode, and the second one is in the chmod process. >> When creating directories/files/device node/tmpfile, ACLs needs to be >> initialized, but symlink do not.Why not support symlink? It looks like >> many filesystems(eg. ext4, f2fs, > btrfs) support it, except xfs. Thanks for the reviews, but this is inconsistent with my understanding. I think most file systems in Linux do not support it, because most file systems do not register the get/set functions of ACLs for symlink operations. And the posix_acl_create() will determine that it is a symlink type inode, and then skip the creation process. But except for bcachefs, it may be to solve the problem of certain scenarios, so it would be nice if anyone could explain it to us. >> >> Signed-off-by: Li Zetao <lizetao1@huawei.com> >> --- >> fs/ubifs/dir.c | 16 ++++++++++++++++ >> fs/ubifs/file.c | 4 ++++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c >> index 551148de66cd..dfb6823cc953 100644 >> --- a/fs/ubifs/dir.c >> +++ b/fs/ubifs/dir.c >> @@ -316,6 +316,10 @@ static int ubifs_create(struct mnt_idmap *idmap, >> struct inode *dir, >> goto out_fname; >> } >> + err = ubifs_init_acl(inode, dir); >> + if (err) >> + goto out_inode; >> + > Attention, a new inconsistent problem point is imported by acl xattr > creation. See https://bugzilla.kernel.org/show_bug.cgi?id=218309. @Richard This problem is indeed a bit tricky. >> err = ubifs_init_security(dir, inode, &dentry->d_name); >> if (err) >> goto out_inode; >> @@ -466,6 +470,10 @@ static int ubifs_tmpfile(struct mnt_idmap *idmap, >> struct inode *dir, >> } >> ui = ubifs_inode(inode); >> + err = ubifs_init_acl(inode, dir); >> + if (err) >> + goto out_inode; >> + >> err = ubifs_init_security(dir, inode, &dentry->d_name); >> if (err) >> goto out_inode; >> @@ -1013,6 +1021,10 @@ static int ubifs_mkdir(struct mnt_idmap *idmap, >> struct inode *dir, >> goto out_fname; >> } >> + err = ubifs_init_acl(inode, dir); >> + if (err) >> + goto out_inode; >> + >> err = ubifs_init_security(dir, inode, &dentry->d_name); >> if (err) >> goto out_inode; >> @@ -1108,6 +1120,10 @@ static int ubifs_mknod(struct mnt_idmap *idmap, >> struct inode *dir, >> ui->data = dev; >> ui->data_len = devlen; >> + err = ubifs_init_acl(inode, dir); >> + if (err) >> + goto out_inode; >> + >> err = ubifs_init_security(dir, inode, &dentry->d_name); >> if (err) >> goto out_inode; > The whiteout inode is not set acl for rename(WHITEOUT) operation. It > looks like many filesystems(eg. ext4, f2fs, btrfs) support it, except > xfs. In my opinion, whiteout is a char dev, since char/block device is > supported, why not support whiteout? > > If we support whiteout, we should make sure that the whiteout renameing > operation is atomic[1]. But I cannot come up with an idea how to combine > whiteout xattr(acl) creation and whiteout file creation into an atomic > operation, just like problem mentioned in [2], Yes, thanks, I have fixed it in v2 version. > > [1] > https://lore.kernel.org/linux-mtd/20211227032246.2886878-6-chengzhihao1@huawei.com/ > [2] https://bugzilla.kernel.org/show_bug.cgi?id=218309 >> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c >> index 5029eb3390a5..8f964f8b0f96 100644 >> --- a/fs/ubifs/file.c >> +++ b/fs/ubifs/file.c >> @@ -41,6 +41,7 @@ >> #include <linux/mount.h> >> #include <linux/slab.h> >> #include <linux/migrate.h> >> +#include <linux/posix_acl.h> >> static int read_block(struct inode *inode, void *addr, unsigned int >> block, >> struct ubifs_data_node *dn) >> @@ -1298,6 +1299,9 @@ int ubifs_setattr(struct mnt_idmap *idmap, >> struct dentry *dentry, >> else >> err = do_setattr(c, inode, attr); >> + if (!err && (attr->ia_valid & ATTR_MODE)) >> + err = posix_acl_chmod(idmap, dentry, inode->i_mode); >> + >> return err; >> } >> >
在 2024/3/22 19:57, Li Zetao 写道: > Hi, > > On 2024/3/21 11:47, Zhihao Cheng wrote: >> 在 2024/3/20 0:16, Li Zetao 写道: >>> There are two scenarios where ACL needs to be updated, the first one >>> is when creating the inode, and the second one is in the chmod process. >>> When creating directories/files/device node/tmpfile, ACLs needs to be >>> initialized, but symlink do not.Why not support symlink? It looks >>> like many filesystems(eg. ext4, f2fs, >> btrfs) support it, except xfs. > Thanks for the reviews, but this is inconsistent with my understanding. > I think most file systems in Linux do not support it, because most file > systems do not register the get/set functions of ACLs for symlink > operations. And the posix_acl_create() will determine that it is a > symlink type inode, and then skip the creation process. But except for > bcachefs, it may be to solve the problem of certain scenarios, so it > would be nice if anyone could explain it to us. You are right, only bcachefs support acl for symlink.
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c index 551148de66cd..dfb6823cc953 100644 --- a/fs/ubifs/dir.c +++ b/fs/ubifs/dir.c @@ -316,6 +316,10 @@ static int ubifs_create(struct mnt_idmap *idmap, struct inode *dir, goto out_fname; } + err = ubifs_init_acl(inode, dir); + if (err) + goto out_inode; + err = ubifs_init_security(dir, inode, &dentry->d_name); if (err) goto out_inode; @@ -466,6 +470,10 @@ static int ubifs_tmpfile(struct mnt_idmap *idmap, struct inode *dir, } ui = ubifs_inode(inode); + err = ubifs_init_acl(inode, dir); + if (err) + goto out_inode; + err = ubifs_init_security(dir, inode, &dentry->d_name); if (err) goto out_inode; @@ -1013,6 +1021,10 @@ static int ubifs_mkdir(struct mnt_idmap *idmap, struct inode *dir, goto out_fname; } + err = ubifs_init_acl(inode, dir); + if (err) + goto out_inode; + err = ubifs_init_security(dir, inode, &dentry->d_name); if (err) goto out_inode; @@ -1108,6 +1120,10 @@ static int ubifs_mknod(struct mnt_idmap *idmap, struct inode *dir, ui->data = dev; ui->data_len = devlen; + err = ubifs_init_acl(inode, dir); + 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 5029eb3390a5..8f964f8b0f96 100644 --- a/fs/ubifs/file.c +++ b/fs/ubifs/file.c @@ -41,6 +41,7 @@ #include <linux/mount.h> #include <linux/slab.h> #include <linux/migrate.h> +#include <linux/posix_acl.h> static int read_block(struct inode *inode, void *addr, unsigned int block, struct ubifs_data_node *dn) @@ -1298,6 +1299,9 @@ int ubifs_setattr(struct mnt_idmap *idmap, struct dentry *dentry, else err = do_setattr(c, inode, attr); + if (!err && (attr->ia_valid & ATTR_MODE)) + err = posix_acl_chmod(idmap, dentry, inode->i_mode); + return err; }
There are two scenarios where ACL needs to be updated, the first one is when creating the inode, and the second one is in the chmod process. When creating directories/files/device node/tmpfile, ACLs needs to be initialized, but symlink do not. Signed-off-by: Li Zetao <lizetao1@huawei.com> --- fs/ubifs/dir.c | 16 ++++++++++++++++ fs/ubifs/file.c | 4 ++++ 2 files changed, 20 insertions(+)