Re: [PATCH] userns: Add basic quota support v2

Jan Kara [off-list ref] writes:

On Mon 27-08-12 17:12:16, Eric W. Biederman wrote: 

quoted

Add the data type struct qown which holds the owning identifier of a
quota.  struct qown is a replacement for the implicit union of uid,
gid and project stored in an unsigned int and the quota type field
that is was used in the quota data structures.  Making the data type
explicit allows the kuid_t and kgid_t type safety to propogate more
thoroughly through the code, revealing more places where uid/gid
conversions need be made.

Allong with the data type struct qown comes the helper functions

  ^^^^ Along

quoted

qown_eq, qown_lt, from_qown, from_qown_munged, qown_valid, make_qown,
make_qown_invalid, make_qown_uid, make_qown_gid.

Replace struct dquot dq_id and dq_type with dq_own a struct qown.

Update the signature of dqget, quota_send_warning, dquot_get_dqblk,
and dquot_set_dqblk to use struct qown.

Make minimal changes to ext3, ext4, gfs2, ocfs2, and xfs to deal with
the change in quota structures and signatures.  The ocfs2 changes are
larger than most because of the extensive tracing throughout the ocfs2
quota code that prints out dq_id.

v2:
 - Renamed qown_t struct qown
 - Added the quota type to struct qown.
 - Removed enum quota_type (In this patch it was just noise)
 - Added qown_lt, make_qown_invalid, make_qown_uid, make_qown_gid
 - Taught qown to handle xfs project ids (but only in init_user_ns). 
   Q_XGETQUOTA calls .get_quotblk with project ids.

  Just a couple one minor comments below...

quoted

@@ -130,13 +130,17 @@ static void copy_to_if_dqblk(struct if_dqblk *dst, struct fs_disk_quota *src)
 static int quota_getquota(struct super_block *sb, int type, qid_t id,
 			  void __user *addr)
 {
+	struct qown qown;
 	struct fs_disk_quota fdq;
 	struct if_dqblk idq;
 	int ret;
 
 	if (!sb->s_qcop->get_dqblk)
 		return -ENOSYS;
-	ret = sb->s_qcop->get_dqblk(sb, type, id, &fdq);
+	qown = make_qown(current_user_ns(), type, id);
+	if (qown_valid(qown))

            ^ missing '!'

Good catch thank you.  

quoted

+		return -EINVAL;
+	ret = sb->s_qcop->get_dqblk(sb, qown, &fdq);
 	if (ret)
 		return ret;
 	copy_to_if_dqblk(&idq, &fdq);

...

quoted

+static inline u32 from_qown(struct user_namespace *user_ns, struct qown qown)
+{
+	switch (qown.type) {
+	case USRQUOTA:
+		return from_kuid(user_ns, qown.uid);
+	case GRPQUOTA:
+		return from_kgid(user_ns, qown.gid);
+	case XQM_PRJQUOTA:
+		return (user_ns == &init_user_ns) ? qown.prj : -1;
+	default:
+		BUG();
+	}
+}

  I would like a bit more if the function somehow expressed in its name
that it returns id. id_from_qown() might be a bit too long given how often
it is used. qown2id() would be OK but it would be inconsistent with how
names of other functions you've added are formed. So I'm somewhat
undecided...

The qown vs id distinction bothers me a little bit.

I almost want to name it struct kid, and the functions make_kid,
from_kid etc.  Where the emphasis is that we are transforming in and out
of the kernel internal form.  I don't really like make_kid because id as
a base name seems to generic and it barely tells you it is.  Perhaps
make_kqid.  Where we call the quota ids and qid for short?

I am a little uncomfortable calling them kqids because the userspace
code also places format_ids in a plain qid_t.  But make_kqid and
from_kqid seems the best alternate set of names I can come up with.

Eric

On Tue 28-08-12 10:34:04, Eric W. Biederman wrote:

Jan Kara [off-list ref] writes:

quoted

quoted

+		return -EINVAL;
+	ret = sb->s_qcop->get_dqblk(sb, qown, &fdq);
 	if (ret)
 		return ret;
 	copy_to_if_dqblk(&idq, &fdq);

...

quoted

+static inline u32 from_qown(struct user_namespace *user_ns, struct qown qown)
+{
+	switch (qown.type) {
+	case USRQUOTA:
+		return from_kuid(user_ns, qown.uid);
+	case GRPQUOTA:
+		return from_kgid(user_ns, qown.gid);
+	case XQM_PRJQUOTA:
+		return (user_ns == &init_user_ns) ? qown.prj : -1;
+	default:
+		BUG();
+	}
+}

  I would like a bit more if the function somehow expressed in its name
that it returns id. id_from_qown() might be a bit too long given how often
it is used. qown2id() would be OK but it would be inconsistent with how
names of other functions you've added are formed. So I'm somewhat
undecided...

The qown vs id distinction bothers me a little bit.

I almost want to name it struct kid, and the functions make_kid,
from_kid etc.  Where the emphasis is that we are transforming in and out
of the kernel internal form.  I don't really like make_kid because id as
a base name seems to generic and it barely tells you it is.  Perhaps
make_kqid.  Where we call the quota ids and qid for short?

I am a little uncomfortable calling them kqids because the userspace
code also places format_ids in a plain qid_t.  But make_kqid and
from_kqid seems the best alternate set of names I can come up with.

  OK, these would seem reasonable to me.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR