| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87hatoy96u.fsf@dmbot.sw.ru>
Date: Tue, 03 Jul 2012 22:46:17 +0400
From: Dmitry Monakhov <dmonakhov@...nvz.org>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 3/5] ext4: Implement project ID support for ext4 filesystem
On Fri, 22 Jun 2012 01:51:46 +0200, Jan Kara <jack@...e.cz> wrote:
> On Thu 21-06-12 13:08:51, Dmitry Monakhov wrote:
> > * Abstract
> > A subtree of a directory tree T is a tree consisting of a directory
> > (the subtree root) in T and all of its descendants in T.
> >
> > *NOTE*: User is allowed to break pure subtree hierarchy via manual
> > id manipulation.
> >
> > Project subtrees assumptions:
> > (1) Each inode has an id. This id is persistently stored inside
> > inode (xattr, usually inside ibody)
> > (2) Project id is inherent from parent directory
> >
> > This feature is similar to project-id in XFS. One may assign some id to
> > a subtree. Each entry from the subtree may be accounted in directory
> > project quota. Will appear in later patches.
> >
> > * Disk layout
> > Project id is stored on disk inside xattr usually inside ibody.
> > Xattr is used only as a data storage, It has not user visible xattr
> > interface.
> >
> > * User interface
> > Project id is accessible via generic xattr interface "system.project_id"
> >
> > * Notes
> > ext4_setattr interface to prjid: Semantically prjid must being changed
> > similar to uid/gid, but project_id is stored inside xattr so on-disk
> > structures updates is not that trivial, so I've move prjid change
> > logic to separate function.
> Generally, I like the patch. Some comments are below.
Also reasonable, will redo.
>
> > Signed-off-by: Dmitry Monakhov <dmonakhov@...nvz.org>
> > ---
> > fs/ext4/Kconfig | 8 ++
> > fs/ext4/Makefile | 1 +
> > fs/ext4/ext4.h | 4 +
> > fs/ext4/ialloc.c | 6 ++
> > fs/ext4/inode.c | 4 +
> > fs/ext4/project.c | 213 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > fs/ext4/project.h | 45 +++++++++++
> > fs/ext4/super.c | 16 ++++
> > fs/ext4/xattr.c | 7 ++
> > fs/ext4/xattr.h | 2 +
> > 10 files changed, 306 insertions(+), 0 deletions(-)
> > create mode 100644 fs/ext4/project.c
> > create mode 100644 fs/ext4/project.h
> >
> > diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig
> > index c22f170..377af40 100644
> > --- a/fs/ext4/Kconfig
> > +++ b/fs/ext4/Kconfig
> > @@ -76,6 +76,14 @@ config EXT4_FS_SECURITY
> >
> > If you are not using a security module that requires using
> > extended attributes for file security labels, say N.
> > +config EXT4_PROJECT_ID
> > + bool "Ext4 project_id support"
> > + depends on PROJECT_ID
> > + depends on EXT4_FS_XATTR
> > + help
> > + Enables project inode identifier support for ext4 filesystem.
> > + This feature allow to assign some id to inodes similar to
> > + uid/gid.
> Is separate config option useful? The amount of code added for this is
> not really big and there is not other speed / space benefit in disabling
> this, is there?
>
> > config EXT4_DEBUG
> > bool "EXT4 debugging support"
> > diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
> > index 56fd8f8..692fe56 100644
> > --- a/fs/ext4/Makefile
> > +++ b/fs/ext4/Makefile
> > @@ -12,3 +12,4 @@ ext4-y := balloc.o bitmap.o dir.o file.o fsync.o ialloc.o inode.o page-io.o \
> > ext4-$(CONFIG_EXT4_FS_XATTR) += xattr.o xattr_user.o xattr_trusted.o
> > ext4-$(CONFIG_EXT4_FS_POSIX_ACL) += acl.o
> > ext4-$(CONFIG_EXT4_FS_SECURITY) += xattr_security.o
> > +ext4-$(CONFIG_EXT4_PROJECT_ID) += project.o
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index cfc4e01..c0e33d7 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -925,6 +925,9 @@ struct ext4_inode_info {
> >
> > /* Precomputed uuid+inum+igen checksum for seeding inode checksums */
> > __u32 i_csum_seed;
> > +#ifdef CONFIG_EXT4_PROJECT_ID
> > + __u32 i_prjid;
> > +#endif
> > };
> >
> > /*
> > @@ -962,6 +965,7 @@ struct ext4_inode_info {
> > #define EXT4_MOUNT_POSIX_ACL 0x08000 /* POSIX Access Control Lists */
> > #define EXT4_MOUNT_NO_AUTO_DA_ALLOC 0x10000 /* No auto delalloc mapping */
> > #define EXT4_MOUNT_BARRIER 0x20000 /* Use block barriers */
> > +#define EXT4_MOUNT_PROJECT_ID 0x40000 /* project owner id support */
> And I would even question the necessity of the mount option. Can we make
> the rule that no system.prjid xattr simply means prjid == 0. When someone
> sets prjid, it gets further automatically inherited and everything works
> fine. I don't see the need for special mount option for this - again no
> significant overhead is introduced when we always check whether we should
> inherit non-zero prjid... Thoughts?
Yep. this reasonable, Also it allows us to save mountopt bit space for
new crazy stuff.
>
> > #define EXT4_MOUNT_QUOTA 0x80000 /* Some quota option set */
> > #define EXT4_MOUNT_USRQUOTA 0x100000 /* "old" user quota */
> > #define EXT4_MOUNT_GRPQUOTA 0x200000 /* "old" group quota */
> > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> > index d48e8b1..d4b72e5 100644
> > --- a/fs/ext4/ialloc.c
> > +++ b/fs/ext4/ialloc.c
> > @@ -28,6 +28,7 @@
> > #include "ext4_jbd2.h"
> > #include "xattr.h"
> > #include "acl.h"
> > +#include "project.h"
> >
> > #include <trace/events/ext4.h>
> >
> > @@ -898,6 +899,8 @@ got:
> >
> > ei->i_extra_isize = EXT4_SB(sb)->s_want_extra_isize;
> >
> > + ext4_setprjid(inode, ext4_getprjid(dir));
> > +
> > ret = inode;
> > dquot_initialize(inode);
> > err = dquot_alloc_inode(inode);
> > @@ -911,6 +914,9 @@ got:
> > err = ext4_init_security(handle, inode, dir, qstr);
> > if (err)
> > goto fail_free_drop;
> > + err = ext4_prj_init(handle, inode);
> > + if (err)
> > + goto fail_free_drop;
> >
> > if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_EXTENTS)) {
> > /* set extent flag only for directory, file and normal symlink*/
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index 02bc8cb..c98d8d6 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -42,6 +42,7 @@
> > #include "xattr.h"
> > #include "acl.h"
> > #include "truncate.h"
> > +#include "project.h"
> >
> > #include <trace/events/ext4.h>
> >
> > @@ -3870,6 +3871,9 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> > }
> > if (ret)
> > goto bad_inode;
> > + ret = ext4_prj_read(inode);
> > + if (ret)
> > + goto bad_inode;
> >
> > if (S_ISREG(inode->i_mode)) {
> > inode->i_op = &ext4_file_inode_operations;
> > diff --git a/fs/ext4/project.c b/fs/ext4/project.c
> > new file mode 100644
> > index 0000000..a262a49
> > --- /dev/null
> > +++ b/fs/ext4/project.c
> > @@ -0,0 +1,213 @@
> > +/*
> > + * linux/fs/ext4/projectid.c
> > + *
> > + * Copyright (C) 2010 Parallels Inc
> > + * Dmitry Monakhov <dmonakhov@...nvz.org>
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/sched.h>
> > +#include <linux/slab.h>
> > +#include <linux/capability.h>
> > +#include <linux/fs.h>
> > +#include <linux/quotaops.h>
> > +#include "ext4_jbd2.h"
> > +#include "ext4.h"
> > +#include "xattr.h"
> > +#include "project.h"
> > +
> > +
> > +/*
> > + * PROJECT SUBTREE
> > + * A subtree of a directory tree T is a tree consisting of a directory
> > + * (the subtree root) in T and all of its descendants in T.
> > + *
> > + * Project Subtree's assumptions:
> > + * (1) Each inode has subtree id. This id is persistently stored inside
> > + * inode's xattr, usually inside ibody
> > + * (2) Subtree id is inherent from parent directory
> > + */
> > +
> > +/*
> > + * Read project_id id from inode's xattr
> > + * Locking: none
> > + */
> > +int ext4_prj_xattr_read(struct inode *inode, unsigned int *prjid)
> > +{
> > + __le32 dsk_prjid;
> > + int retval;
> Please add empty line between declarations and function body... Also
> holds for some functions below.
>
> > + retval = ext4_xattr_get(inode, EXT4_XATTR_INDEX_PROJECT_ID, "",
> > + &dsk_prjid, sizeof (dsk_prjid));
> > + if (retval > 0) {
> > + if (retval != sizeof(dsk_prjid))
> > + return -EIO;
> > + else
> > + retval = 0;
> > + }
> > + *prjid = le32_to_cpu(dsk_prjid);
> > + return retval;
> > +
> > +}
>
> Honza
> --
> Jan Kara <jack@...e.cz>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists