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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


Dave thanks for taking the time to take a detailed look at this code.

Dave Chinner <david@xxxxxxxxxxxxx> writes:

> On Tue, Aug 28, 2012 at 12:09:56PM -0700, Eric W. Biederman wrote:
>> 
>> Add the data type struct kqid which holds the kernel internal form of
>> the owning identifier of a quota.  struct kqid 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.
>> 
>> Along with the data type struct kqid comes the helper functions
>> qid_eq, qid_lt, from_kqid, from_kqid_munged, qid_valid, make_kqid,
>
> I think Jan's comment about from_kqid being named id_from_kgid is
> better, though I also think it would read better as kqid_to_id().
> ie:
>
> 	id = kqid_to_id(ns, qid);

kqid and qid are the same thing just in a different encoding.
Emphasizing the quota identifier instead of the kernel vs user encoding
change is paying attention to the wrong thing.

Using make_kqid and from_kqid follows the exact same conventions as I have
established for kuids and kgids.  So if you learn one you have learned
them all.

>> make_kqid_invalid, make_kqid_uid, make_kqid_gid.
>
> and these named something like uid_to_kqid()

The last two are indeed weird, and definitely not the common case,
since there is no precedent I can almost see doing something different
but I don't see a good case for a different name.

>> Change struct dquot dq_id to a struct kqid and remove the now
>> unecessary dq_type.
>> 
>> Update the signature of dqget, quota_send_warning, dquot_get_dqblk,
>> and dquot_set_dqblk to use struct kqid.
>> 
>> 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.
>
> How did you test that this all works?

By making it a compile error if you get a conversion wrong and making it
a rule not to make any logic changes.  That combined with code review
and running the code a bit to make certain I did not horribly mess up.

> e.g. run xfstests -g quota on
> each of those filesystems and check for no regressions? And if you
> wrote any tests, can you convert them to be part of xfstests so that
> namespace aware quotas get tested regularly?

I have not written any tests, and running the xfstests in a namespace
should roughly be a matter of "unshare -U xfstest -g quota"  It isn't
quite that easy because  /proc/self/uid_map and /proc/self/gid_map need
to be written first.

Right now only ext2 ext3 and ext4 compile with user namespace support
enabled.  xfs and gfs2 and ocfs2 still only compile with user namespace
support disabled because my patches to convert them are waiting in the
wings until I get the core subsystems primarily quota and posix acls
reviewed.

>> v4:
>>   - Rename struct qown struct kqid and associated changes
>>     to the naming of the helper functions.
>>   - Use qid_t to hold the userspace identifier representation
>>     of quota identifiers in all new code.
>> v3:
>>   - Add missing negation on qown_valid
>> 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.
>
> Q_XSETQLIM was modified to handle project quotas as well, I assume?

I didn't break the project quota support for Q_XSETQLIM.


>> index fed504f..96944c0 100644
>> --- a/fs/xfs/xfs_quotaops.c
>> +++ b/fs/xfs/xfs_quotaops.c
>> @@ -97,28 +97,29 @@ xfs_fs_set_xstate(
>>  STATIC int
>>  xfs_fs_get_dqblk(
>>  	struct super_block	*sb,
>> -	int			type,
>> -	qid_t			id,
>> +	struct kqid		qid,
>>  	struct fs_disk_quota	*fdq)
>>  {
>>  	struct xfs_mount	*mp = XFS_M(sb);
>> +	xfs_dqid_t		xfs_id;
>>  
>>  	if (!XFS_IS_QUOTA_RUNNING(mp))
>>  		return -ENOSYS;
>>  	if (!XFS_IS_QUOTA_ON(mp))
>>  		return -ESRCH;
>>  
>> -	return -xfs_qm_scall_getquota(mp, id, xfs_quota_type(type), fdq);
>> +	xfs_id = from_kqid(&init_user_ns, qid);
>> +	return -xfs_qm_scall_getquota(mp, xfs_id, xfs_quota_type(qid.type), fdq);
>>  }
>
> Why a temporary variable? Why not just:
>
> 	return -xfs_qm_scall_getquota(mp, from_kqid(&init_user_ns, qid),
> 				      xfs_quota_type(qid.type), fdq);
>

Because I am not fond of very long lines and because I did the basic
conversion of gfs2 first where using a separate variable a larger
difference.

>From a code review perspective I am still more comfortable with
introducing a temporary variable as the fundamental change is
easier to see.

> Indeed, why not drive the struct kqid down another level into
> xfs_qm_scall_getquota() where all they are used for is parameters to
> the xfs_qm_dqget() function?

I have not driven struct kqid down another level because change
needs to wait until I add user namespace support to xfs.

I only touch xfs in this case because making the implicit union explicit
is needed for type safety and sanity, and unfortunately that requires
the prototype of the quota operations to change.

>>  STATIC int
>>  xfs_fs_set_dqblk(
>>  	struct super_block	*sb,
>> -	int			type,
>> -	qid_t			id,
>> +	struct kqid		qid,
>>  	struct fs_disk_quota	*fdq)
>>  {
>>  	struct xfs_mount	*mp = XFS_M(sb);
>> +	xfs_dqid_t		xfs_id;
>>  
>>  	if (sb->s_flags & MS_RDONLY)
>>  		return -EROFS;
>> @@ -127,7 +128,8 @@ xfs_fs_set_dqblk(
>>  	if (!XFS_IS_QUOTA_ON(mp))
>>  		return -ESRCH;
>>  
>> -	return -xfs_qm_scall_setqlim(mp, id, xfs_quota_type(type), fdq);
>> +	xfs_id = from_kqid(&init_user_ns, qid);
>> +	return -xfs_qm_scall_setqlim(mp, xfs_id, xfs_quota_type(qid.type), fdq);
>>  }
>
> Same is true here....
>
>>  
>>  const struct quotactl_ops xfs_quotactl_operations = {
>> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
>> index bcb6054..46de393 100644
>> --- a/fs/xfs/xfs_trans_dquot.c
>> +++ b/fs/xfs/xfs_trans_dquot.c
>> @@ -575,12 +575,14 @@ xfs_quota_warn(
>>  	struct xfs_dquot	*dqp,
>>  	int			type)
>>  {
>> +	int qtype;
>> +	struct kqid qid;
>>  	/* no warnings for project quotas - we just return ENOSPC later */
>>  	if (dqp->dq_flags & XFS_DQ_PROJ)
>>  		return;
>> -	quota_send_warning((dqp->dq_flags & XFS_DQ_USER) ? USRQUOTA : GRPQUOTA,
>> -			   be32_to_cpu(dqp->q_core.d_id), mp->m_super->s_dev,
>> -			   type);
>> +	qtype = (dqp->dq_flags & XFS_DQ_USER) ? USRQUOTA : GRPQUOTA;
>> +	qid = make_kqid(&init_user_ns, qtype, be32_to_cpu(dqp->q_core.d_id));
>> +	quota_send_warning(qid, mp->m_super->s_dev, type);
>>  }
>>  
>>  /*
>> diff --git a/include/linux/quota.h b/include/linux/quota.h
>> index 524ede8..0e73250 100644
>> --- a/include/linux/quota.h
>> +++ b/include/linux/quota.h
>> @@ -181,10 +181,161 @@ enum {
>>  #include <linux/dqblk_v2.h>
>>  
>>  #include <linux/atomic.h>
>> +#include <linux/uidgid.h>
>>  
>>  typedef __kernel_uid32_t qid_t; /* Type in which we store ids in memory */
>>  typedef long long qsize_t;	/* Type in which we store sizes */
>
> From fs/xfs/xfs_types.h:
>
> typedef __uint32_t              prid_t;         /* project ID */
>
> Perhaps it would be better to have an official kprid_t definition
> here, i.e:

We can always improve.  For this patch I don't see it making a
useful difference.

prid_t isn't exported to non xfs code.  The project id is already
stored in an unsigned int or a qid_t.

>> +struct kqid {			/* Type in which we store the quota identifier */
>> +	union {
>> +		kuid_t uid;
>> +		kgid_t gid;
>> +		qid_t prj;
>
> 		kprid_t prid;
>

Hmm.  The naming here is interesting.  No one calls the project id prj.
So I have an avoidable inconsistency here.  xfs most commonly uses
projid and occassionally calls it prid.

So I am inclined to rename this field projid, but I don't know if it is
worth respinning the patch for something so trivial.

>> +	};
>> +	int type; /* USRQUOTA (uid) or GRPQUOTA (gid) or XQM_PRJQUOTA (prj) */
>> +};
>> +
>> +static inline bool qid_eq(struct kqid left, struct kqid right)
>> +{
>> +	if (left.type != right.type)
>> +		return false;
>> +	switch(left.type) {
>> +	case USRQUOTA:
>> +		return uid_eq(left.uid, right.uid);
>> +	case GRPQUOTA:
>> +		return gid_eq(left.gid, right.gid);
>> +	case XQM_PRJQUOTA:
>> +		return left.prj == right.prj;
>> +	default:
>> +		BUG();
>
> BUG()? Seriously? The most this justifies is a WARN_ON_ONCE() to
> indicate a potential programming error, not bringing down the entire
> machine.

Dead serious.  Any other type value is impossible and unsupported.

BUG is not panic.  BUG is an oops. BUG won't bring down the machine
unless the sysadmin wants it too.

Beyond that I am not interesting in supporting exploitable abuse
of this type.

>> +static inline bool qid_lt(struct kqid left, struct kqid right)
>> +{
>> +	if (left.type < right.type)
>> +		return true;
>> +	if (left.type > right.type)
>> +		return false;
>> +	switch (left.type) {
>> +	case USRQUOTA:
>> +		return uid_lt(left.uid, right.uid);
>> +	case GRPQUOTA:
>> +		return gid_lt(left.gid, right.gid);
>> +	case XQM_PRJQUOTA:
>> +		return left.prj < right.prj;
>> +	default:
>> +		BUG();
>> +	}
>> +}
>
> What is this function used for? it's not referenced at all by the
> patch, and there's no documentation/comments explaining why it
> exists or how it is intended to be used....

This function is introduced early, but it is needed for gfs2 as
performs a sort of quota identifiers.

>> +static inline qid_t from_kqid(struct user_namespace *user_ns, struct kqid qid)
>> +{
>> +	switch (qid.type) {
>> +	case USRQUOTA:
>> +		return from_kuid(user_ns, qid.uid);
>> +	case GRPQUOTA:
>> +		return from_kgid(user_ns, qid.gid);
>> +	case XQM_PRJQUOTA:
>> +		return (user_ns == &init_user_ns) ? qid.prj : -1;
>> +	default:
>> +		BUG();
>> +	}
>> +}
>
> Oh, this can return an error. That's only checked in a coupl eof
> places this function is called. it needs tobe checked everywhere,
> otherwise we now have the possibility of quota usage being accounted
> to uid/gid/prid 0xffffffff when namespace matches are not found.

No this is not an error condition.  Returning -1 is the mapping that is
used when there is not a mapping entry.

Depending on the circumstances not having a mapping can be an error,
but it can also be a don't care condition.

All in kernel values are defined as having a mapping into the initial
user namespace.

Looking at my tree the only calls of from_kqid not mapping the id
into the initial user namespace are from_kqid_munged and they report
to userspace.  from_kqid_munged returns fs_overflowuid or fs_overflowgid
in case of a mapping failure.

projects ids can only be accessed if capable(CAP_SYS_ADMIN) which
you can only have in the initial user namespace so for now there will
alwasy be a mapping for project ids.

>> +static inline qid_t from_kqid_munged(struct user_namespace *user_ns,
>> +				   struct kqid qid)
>
> What does munging do to the return value? how is it different to
> from_kqid()? Document your API....

This bit certainly certainly could use a bit more explanation.

Mapping of uids and gids from one domain to another is not new in linux.
It originates with the transition from 16bit identifiers to 32bit
identifiers.  In most places when there is a 32bit identifier that can
not be represented we return a fs_overflowid aka (u16)-2 aka nobody.

So in general when we want to pass a value to userspace from_kqid_munged
is called, and if there is a mapping failure we report the value that
userspace set fs_overflowuid or fs_overflowgid to.  For project ids it
which are restricted to the initial user namespace no mapping failures
can occur.

>> +static inline struct kqid make_kqid(struct user_namespace *user_ns,
>> +				    int type, qid_t qid)
>> +{
>> +	struct kqid kqid;
>> +
>> +	kqid.type = type;
>> +	switch (type) {
>> +	case USRQUOTA:
>> +		kqid.uid = make_kuid(user_ns, qid);
>> +		break;
>> +	case GRPQUOTA:
>> +		kqid.gid = make_kgid(user_ns, qid);
>> +		break;
>> +	case XQM_PRJQUOTA:
>> +		if (user_ns == &init_user_ns)
>> +			kqid.prj = qid;
>> +		else
>> +			kqid.prj = -1;
>> +		break;
>
> 		kqid.prj = (user_ns == &init_user_ns) ? qid : -1;

That is an interesting inconsitency. Given that implemented the others
on one line.

>> +	default:
>> +		BUG();
>> +	}
>> +	return kqid;
>> +}
>> +
>> +static inline struct kqid make_kqid_invalid(int type)
>> +{
>> +	struct kqid kqid;
>> +
>> +	kqid.type = type;
>> +	switch (type) {
>> +	case USRQUOTA:
>> +		kqid.uid = INVALID_UID;
>> +		break;
>> +	case GRPQUOTA:
>> +		kqid.gid = INVALID_GID;
>> +		break;
>> +	case XQM_PRJQUOTA:
>> +		kqid.prj = -1;
>> +		break;
>> +	default:
>> +		BUG();
>> +	}
>> +	return kqid;
>> +}
>> +
>> +static inline struct kqid make_kqid_uid(kuid_t uid)
>> +{
>> +	struct kqid kqid = {
>> +		.type = USRQUOTA,
>> +		.uid  = uid,
>> +	};
>> +	return kqid;
>> +}
>
> Isn't this sort of construct frowned upon? i.e. returning a
> structure out of scope? It may be inline code and hence work, but
> this strikes me as a landmine waiting for someone to tread on....

Not at all because I am returning the structure by value.

Return a structure by reference is the case that doesn't work.

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[Other Archives]     [Linux Kernel Newbies]     [Linux Driver Development]     [Linux Kbuild]     [Fedora Kernel]     [Linux Kernel Testers]     [Linux SH]     [Linux Omap]     [Linux Tape]     [Linux Input]     [Linux LEDS]     [Linux Kernel Janitors]     [Linux Kernel Packagers]     [Linux Doc]     [Linux Man Pages]     [Linux API]     [Linux Memory Management]     [Linux Modules]     [Linux Standards]     [Kernel Announce]     [Netdev]     [Git]     [Linux PCI]     Linux CAN Development     [Linux I2C]     [Linux RDMA]     [Linux NUMA]     [Netfilter]     [Netfilter Devel]     [SELinux]     [Bugtraq]     [FIO]     [Linux Perf Users]     [Linux Serial]     [Linux PPP]     [Linux ISDN]     [Linux Next]     [Kernel Stable Commits]     [Linux Tip Commits]     [Kernel MM Commits]     [Linux Security Module]     [AutoFS]     [Filesystem Development]     [Ext3 Filesystem]     [Linux bcache]     [Ext4 Filesystem]     [Linux BTRFS]     [Linux CEPH Filesystem]     [Linux XFS]     [XFS]     [Linux NFS]     [Linux CIFS]     [Ecryptfs]     [Linux NILFS]     [Linux Cachefs]     [Reiser FS]     [Initramfs]     [Linux FB Devel]     [Linux OpenGL]     [DRI Devel]     [Fastboot]     [Linux RT Users]     [Linux RT Stable]     [eCos]     [Corosync]     [Linux Clusters]     [LVS Devel]     [Hot Plug]     [Linux Virtualization]     [KVM]     [KVM PPC]     [KVM ia64]     [Linux Containers]     [Linux Hexagon]     [Linux Cgroups]     [Util Linux]     [Wireless]     [Linux Bluetooth]     [Bluez Devel]     [Ethernet Bridging]     [Embedded Linux]     [Barebox]     [Linux MMC]     [Linux IIO]     [Sparse]     [Smatch]     [Linux Arch]     [x86 Platform Driver]     [Linux ACPI]     [Linux IBM ACPI]     [LM Sensors]     [CPU Freq]     [Linux Power Management]     [Linmodems]     [Linux DCCP]     [Linux SCTP]     [ALSA Devel]     [Linux USB]     [Linux PA RISC]     [Linux Samsung SOC]     [MIPS Linux]     [IBM S/390 Linux]     [ARM Linux]     [ARM Kernel]     [ARM MSM]     [Tegra Devel]     [Sparc Linux]     [Linux Security]     [Linux Sound]     [Linux Media]     [Video 4 Linux]     [Linux IRDA Users]     [Linux for the blind]     [Linux RAID]     [Linux ATA RAID]     [Device Mapper]     [Linux SCSI]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Linux IDE]     [Linux SMP]     [Linux AXP]     [Linux Alpha]     [Linux M68K]     [Linux ia64]     [Linux 8086]     [Linux x86_64]     [Linux Config]     [Linux Apps]     [Linux MSDOS]     [Linux X.25]     [Linux Crypto]     [DM Crypt]     [Linux Trace Users]     [Linux Btrace]     [Linux Watchdog]     [Utrace Devel]     [Linux C Programming]     [Linux Assembly]     [Dash]     [DWARVES]     [Hail Devel]     [Linux Kernel Debugger]     [Linux gcc]     [Gcc Help]     [X.Org]     [Wine]

Add to Google Powered by Linux

[Older Kernel Discussion]     [Yosemite National Park Forum]     [Large Format Photos]     [Gimp]     [Yosemite Photos]     [Stuff]