Re: [v8 4/5] ext4: adds FS_IOC_FSSETXATTR/FS_IOC_FSGETXATTR interface support

Started by Dave Chinner on Jan 23, 2015 · 4 messages from 4 authors · Last reply Jan 23, 2015

open the first message on its own page

Message details and related content
On Thu, Jan 22, 2015 at 06:28:51PM +0300, Konstantin Khlebnikov wrote:
On 09.12.2014 08:22, Li Xi wrote:
quoted
+static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
+{
+	struct inode *inode = file_inode(filp);
+	struct super_block *sb = inode->i_sb;
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	int err;
+	handle_t *handle;
+	kprojid_t kprojid;
+	struct ext4_iloc iloc;
+	struct ext4_inode *raw_inode;
+
+	struct dquot *transfer_to[EXT4_MAXQUOTAS] = { };
+
+	/* Make sure caller can change project. */
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
+	if (projid != EXT4_DEF_PROJID
+	    && !EXT4_HAS_RO_COMPAT_FEATURE(sb,
+			EXT4_FEATURE_RO_COMPAT_PROJECT))
+		return -EOPNOTSUPP;
+
+	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
+			EXT4_FEATURE_RO_COMPAT_PROJECT)) {
+		BUG_ON(__kprojid_val(EXT4_I(inode)->i_projid)
+		       != EXT4_DEF_PROJID);
+		if (projid != EXT4_DEF_PROJID)
+			return -EOPNOTSUPP;
+		else
+			return 0;
+	}
+
+	kprojid = make_kprojid(&init_user_ns, (projid_t)projid);
Maybe current_user_ns()?
This code should be user-namespace aware from the beginning.
No, the code is correct. Project quotas have nothing to do with
UIDs and so should never have been included in the uid/gid
namespace mapping infrastructure in the first place.

Point in case: directory subtree quotas can be used as a resource
controller for limiting space usage within separate containers that
share the same underlying (large) filesystem via mount namespaces.

Cheers,

Dave.

PS: can you please trim your reply to just the sections of the
patch you are commenting to? Finding replies like this in a large
patch is like searching for a needle in a haystack...
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
Message details and related content
On 23.01.2015 04:53, Dave Chinner wrote:
On Thu, Jan 22, 2015 at 06:28:51PM +0300, Konstantin Khlebnikov wrote:
quoted
On 09.12.2014 08:22, Li Xi wrote:
quoted
+static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
+{
+	struct inode *inode = file_inode(filp);
+	struct super_block *sb = inode->i_sb;
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	int err;
+	handle_t *handle;
+	kprojid_t kprojid;
+	struct ext4_iloc iloc;
+	struct ext4_inode *raw_inode;
+
+	struct dquot *transfer_to[EXT4_MAXQUOTAS] = { };
+
+	/* Make sure caller can change project. */
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
+	if (projid != EXT4_DEF_PROJID
+	    && !EXT4_HAS_RO_COMPAT_FEATURE(sb,
+			EXT4_FEATURE_RO_COMPAT_PROJECT))
+		return -EOPNOTSUPP;
+
+	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
+			EXT4_FEATURE_RO_COMPAT_PROJECT)) {
+		BUG_ON(__kprojid_val(EXT4_I(inode)->i_projid)
+		       != EXT4_DEF_PROJID);
+		if (projid != EXT4_DEF_PROJID)
+			return -EOPNOTSUPP;
+		else
+			return 0;
+	}
+
+	kprojid = make_kprojid(&init_user_ns, (projid_t)projid);
Maybe current_user_ns()?
This code should be user-namespace aware from the beginning.
No, the code is correct. Project quotas have nothing to do with

2 more quoted lines

Right, but user-namespace provides id mapping for project-id too.
This infrastructure adds support for nested project quotas with
virtualized ids in sub-containers. I couldn't say that this is
must have feature but implementation is trivial because whole
infrastructure is already here.
Point in case: directory subtree quotas can be used as a resource
controller for limiting space usage within separate containers that
share the same underlying (large) filesystem via mount namespaces.
That's exactly my use-case: 'sub-volumes' for containers with
quota for space usage/inodes count.
Message details and related content
On Fri, Jan 23, 2015 at 02:58:09PM +0300, Konstantin Khlebnikov wrote:
On 23.01.2015 04:53, Dave Chinner wrote:
quoted
On Thu, Jan 22, 2015 at 06:28:51PM +0300, Konstantin Khlebnikov wrote:
quoted
quoted
+	kprojid = make_kprojid(&init_user_ns, (projid_t)projid);
Maybe current_user_ns()?
This code should be user-namespace aware from the beginning.
No, the code is correct. Project quotas have nothing to do with
UIDs and so should never have been included in the uid/gid
namespace mapping infrastructure in the first place.
Right, but user-namespace provides id mapping for project-id too.
This infrastructure adds support for nested project quotas with
virtualized ids in sub-containers. I couldn't say that this is
must have feature but implementation is trivial because whole
infrastructure is already here.
This is an extremely common misunderstanding of project IDs. Project
IDs are completely separate to the UID/GID namespace.  Project
quotas were originally designed specifically for
accounting/enforcing quotas in situations where uid/gid
accounting/enforcing is not possible. This design intent goes back
25 years - it predates XFS...

IOWs, mapping prids via user namespaces defeats the purpose
for which prids were originally intended for.
quoted
Point in case: directory subtree quotas can be used as a resource
controller for limiting space usage within separate containers that
share the same underlying (large) filesystem via mount namespaces.
That's exactly my use-case: 'sub-volumes' for containers with
quota for space usage/inodes count.
That doesn't require mapped project IDs. Hard container space limits
can only be controlled by the init namespace, and because inodes can
hold only one project ID the current ns cannot be allowed to change
the project ID on the inode because that allows them to escape the
resource limits set on the project ID associated with the sub-mount
set up by the init namespace...

i.e.

/mnt			prid = 0, default for entire fs.
/mnt/container1/	prid = 1, inherit, 10GB space limit
/mnt/container2/	prid = 2, inherit, 50GB space limit
.....
/mnt/containerN/	prid = N, inherit, 20GB space limit

And you clone the mount namespace for each container so the root is
at the appropriate /mnt/containerX/.  Now the containers have a
fixed amount of space they can use in the parent filesystem they
know nothing about, and it is enforced by directory subquotas
controlled by the init namespace.  This "fixed amount of space" is
reflected in the container namespace when "df" is run as it will
report the project quota space limits. Adding or removing space to a
container is as simple as changing the project quota limits from the
init namespace. i.e. an admin operation controlled by the host, not
the container....

Allowing the container to modify the prid and/or the inherit bit of
inodes in it's namespace then means the user can define their own
space usage limits, even turn them off. It's not a resource
container at that point because the user can define their own
limits.  Hence, only if the current_ns cannot change project quotas
will we have a hard fence on space usage that the container *cannot
exceed*.

Yes, I know there are other use cases for project quotas *within* a
container as controlled by the user (same as existing project quota
usages), but we don't have the capability of storing multiple
project IDs on each inode, nor accounting/enforcement across
multiple project IDs on an inode. Nor, really, do we want to (on

33 more lines

Message details and related content
On Fri, Jan 23, 2015 at 3:30 PM, Dave Chinner [off-list ref] wrote:
On Fri, Jan 23, 2015 at 02:58:09PM +0300, Konstantin Khlebnikov wrote:
quoted
On 23.01.2015 04:53, Dave Chinner wrote:
quoted
On Thu, Jan 22, 2015 at 06:28:51PM +0300, Konstantin Khlebnikov wrote:
quoted
quoted
+  kprojid = make_kprojid(&init_user_ns, (projid_t)projid);
Maybe current_user_ns()?
This code should be user-namespace aware from the beginning.
No, the code is correct. Project quotas have nothing to do with
UIDs and so should never have been included in the uid/gid
namespace mapping infrastructure in the first place.
Right, but user-namespace provides id mapping for project-id too.
This infrastructure adds support for nested project quotas with
virtualized ids in sub-containers. I couldn't say that this is
must have feature but implementation is trivial because whole
infrastructure is already here.
This is an extremely common misunderstanding of project IDs. Project
IDs are completely separate to the UID/GID namespace.  Project
quotas were originally designed specifically for
accounting/enforcing quotas in situations where uid/gid
accounting/enforcing is not possible. This design intent goes back
25 years - it predates XFS...

IOWs, mapping prids via user namespaces defeats the purpose
for which prids were originally intended for.
quoted
quoted
Point in case: directory subtree quotas can be used as a resource
controller for limiting space usage within separate containers that
share the same underlying (large) filesystem via mount namespaces.
That's exactly my use-case: 'sub-volumes' for containers with
quota for space usage/inodes count.
That doesn't require mapped project IDs. Hard container space limits
can only be controlled by the init namespace, and because inodes can
hold only one project ID the current ns cannot be allowed to change
the project ID on the inode because that allows them to escape the
resource limits set on the project ID associated with the sub-mount

28 more quoted lines

I think I must be missing something simple here.  In a hypothetical
world where the code used nsown_capable, if an admin wants to stick a
container in /mnt/container1 with associated prid 1 and a userns,
shouldn't it just map only prid 1 into the user ns?  Then a user in
that userns can't try to change the prid of a file to 2 because the
number "2" is unmapped for that user and translation will fail.

--Andy
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help