| Message ID | 1453435644-32261-3-git-send-email-sandeen@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu 21-01-16 22:07:19, Eric Sandeen wrote: > Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it > will return quota information for the id equal to or greater > than the id requested. In other words, if the requested id has > no quota, the command will return quota information for the > next higher id which does have a quota set. If no higher id > has an active quota, -ESRCH is returned. > > This allows filesystems to do efficient iteration in kernelspace, > much like extN filesystems do in userspace when asked to report > all active quotas. > > The patch adds a d_id field to struct qc_dqblk so that we can > pass back the id of the quota which was found, and return it > to userspace. > > Today, filesystems such as XFS require getpwent-style iterations, > and for systems which have i.e. LDAP backends, this can be very > slow, or even impossible if iteration is not allowed in the > configuration. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> ... > diff --git a/fs/quota/quota.c b/fs/quota/quota.c > index ea66670..4bf8d40 100644 > --- a/fs/quota/quota.c > +++ b/fs/quota/quota.c > @@ -33,6 +33,7 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd, > /* allow to query information for dquots we "own" */ > case Q_GETQUOTA: > case Q_XGETQUOTA: > + case Q_XGETNEXTQUOTA: IMO you should require CAP_SYS_ADMIN for the quotactl. Definitely doing the UID and GID checks for GETNEXTQUOTA looks strange to me since the returned structure may be for a different ID. Or did you assume that existing user will have quota structure allocated so we always return quotas for that ID in that case? I'm not sure this is good to rely on... > + ret = sb->s_qcop->get_nextdqblk(sb, qid, &qdq); > + if (ret) > + return ret; > + copy_to_xfs_dqblk(&fdq, &qdq, type, qdq.d_id); > + if (copy_to_user(addr, &fdq, sizeof(fdq))) > + return -EFAULT; > + return ret; > +} So how about passing pointer to 'qid' to ->get_nextdqblk() and return the ID that way? That will also force you to fix the issue that you currently completely miss user-namespace conversions for the ID ;). I definitely dislike mixing d_id in the qdq structure with arguments of fs callbacks and that d_id doesn't get filled for most callbacks. That is going to cause confusion. Honza
On 1/22/16 2:55 AM, Jan Kara wrote: > On Thu 21-01-16 22:07:19, Eric Sandeen wrote: >> Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it >> will return quota information for the id equal to or greater >> than the id requested. In other words, if the requested id has >> no quota, the command will return quota information for the >> next higher id which does have a quota set. If no higher id >> has an active quota, -ESRCH is returned. >> >> This allows filesystems to do efficient iteration in kernelspace, >> much like extN filesystems do in userspace when asked to report >> all active quotas. >> >> The patch adds a d_id field to struct qc_dqblk so that we can >> pass back the id of the quota which was found, and return it >> to userspace. >> >> Today, filesystems such as XFS require getpwent-style iterations, >> and for systems which have i.e. LDAP backends, this can be very >> slow, or even impossible if iteration is not allowed in the >> configuration. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > ... >> diff --git a/fs/quota/quota.c b/fs/quota/quota.c >> index ea66670..4bf8d40 100644 >> --- a/fs/quota/quota.c >> +++ b/fs/quota/quota.c >> @@ -33,6 +33,7 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd, >> /* allow to query information for dquots we "own" */ >> case Q_GETQUOTA: >> case Q_XGETQUOTA: >> + case Q_XGETNEXTQUOTA: > > IMO you should require CAP_SYS_ADMIN for the quotactl. Definitely doing the > UID and GID checks for GETNEXTQUOTA looks strange to me since the returned > structure may be for a different ID. Or did you assume that existing user > will have quota structure allocated so we always return quotas for that ID > in that case? I'm not sure this is good to rely on... Oh whoops. OK that was dumb of me, thanks for catching it. No, I didn't intend to rely on the asking user having a quota, it was just a dumb mistake. :) >> + ret = sb->s_qcop->get_nextdqblk(sb, qid, &qdq); >> + if (ret) >> + return ret; >> + copy_to_xfs_dqblk(&fdq, &qdq, type, qdq.d_id); >> + if (copy_to_user(addr, &fdq, sizeof(fdq))) >> + return -EFAULT; >> + return ret; >> +} > > So how about passing pointer to 'qid' to ->get_nextdqblk() and return the ID > that way? That will also force you to fix the issue that you currently > completely miss user-namespace conversions for the ID ;). Ok. > I definitely dislike mixing d_id in the qdq structure with arguments of fs > callbacks and that d_id doesn't get filled for most callbacks. That is going > to cause confusion. Yeah, fair enough. I'll change it. Thanks, -Eric > Honza > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/quota/quota.c b/fs/quota/quota.c index ea66670..4bf8d40 100644 --- a/fs/quota/quota.c +++ b/fs/quota/quota.c @@ -33,6 +33,7 @@ static int check_quotactl_permission(struct super_block *sb, int type, int cmd, /* allow to query information for dquots we "own" */ case Q_GETQUOTA: case Q_XGETQUOTA: + case Q_XGETNEXTQUOTA: if ((type == USRQUOTA && uid_eq(current_euid(), make_kuid(current_user_ns(), id))) || (type == GRPQUOTA && in_egroup_p(make_kgid(current_user_ns(), id)))) break; @@ -625,6 +626,32 @@ static int quota_getxquota(struct super_block *sb, int type, qid_t id, return ret; } +/* + * Return quota for next active quota >= this id, if any exists, + * otherwise return -ESRCH via ->get_nextdqblk. + */ +static int quota_getnextxquota(struct super_block *sb, int type, qid_t id, + void __user *addr) +{ + struct fs_disk_quota fdq; + struct qc_dqblk qdq; + struct kqid qid; + int ret; + + if (!sb->s_qcop->get_nextdqblk) + return -ENOSYS; + qid = make_kqid(current_user_ns(), type, id); + if (!qid_valid(qid)) + return -EINVAL; + ret = sb->s_qcop->get_nextdqblk(sb, qid, &qdq); + if (ret) + return ret; + copy_to_xfs_dqblk(&fdq, &qdq, type, qdq.d_id); + if (copy_to_user(addr, &fdq, sizeof(fdq))) + return -EFAULT; + return ret; +} + static int quota_rmxquota(struct super_block *sb, void __user *addr) { __u32 flags; @@ -690,6 +717,8 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id, return quota_setxquota(sb, type, id, addr); case Q_XGETQUOTA: return quota_getxquota(sb, type, id, addr); + case Q_XGETNEXTQUOTA: + return quota_getnextxquota(sb, type, id, addr); case Q_XQUOTASYNC: if (sb->s_flags & MS_RDONLY) return -EROFS; @@ -712,6 +741,7 @@ static int quotactl_cmd_write(int cmd) case Q_XGETQSTAT: case Q_XGETQSTATV: case Q_XGETQUOTA: + case Q_XGETNEXTQUOTA: case Q_XQUOTASYNC: return 0; } diff --git a/include/linux/quota.h b/include/linux/quota.h index b2505ac..ac79c18 100644 --- a/include/linux/quota.h +++ b/include/linux/quota.h @@ -327,6 +327,7 @@ struct path; /* Structure for communicating via ->get_dqblk() & ->set_dqblk() */ struct qc_dqblk { + u32 d_id; /* user/group/project ID for this quota */ int d_fieldmask; /* mask of fields to change in ->set_dqblk() */ u64 d_spc_hardlimit; /* absolute limit on used space */ u64 d_spc_softlimit; /* preferred limit on used space */ @@ -425,6 +426,8 @@ struct quotactl_ops { int (*quota_sync)(struct super_block *, int); int (*set_info)(struct super_block *, int, struct qc_info *); int (*get_dqblk)(struct super_block *, struct kqid, struct qc_dqblk *); + int (*get_nextdqblk)(struct super_block *, struct kqid, + struct qc_dqblk *); int (*set_dqblk)(struct super_block *, struct kqid, struct qc_dqblk *); int (*get_state)(struct super_block *, struct qc_state *); int (*rm_xquota)(struct super_block *, unsigned int); diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h index dcd75cc..11b3b31 100644 --- a/include/uapi/linux/dqblk_xfs.h +++ b/include/uapi/linux/dqblk_xfs.h @@ -39,6 +39,7 @@ #define Q_XQUOTARM XQM_CMD(6) /* free disk space used by dquots */ #define Q_XQUOTASYNC XQM_CMD(7) /* delalloc flush, updates dquots */ #define Q_XGETQSTATV XQM_CMD(8) /* newer version of get quota */ +#define Q_XGETNEXTQUOTA XQM_CMD(9) /* get disk limits and usage >= ID */ /* * fs_disk_quota structure:
Q_XGETNEXTQUOTA is exactly like Q_XGETQUOTA, except that it will return quota information for the id equal to or greater than the id requested. In other words, if the requested id has no quota, the command will return quota information for the next higher id which does have a quota set. If no higher id has an active quota, -ESRCH is returned. This allows filesystems to do efficient iteration in kernelspace, much like extN filesystems do in userspace when asked to report all active quotas. The patch adds a d_id field to struct qc_dqblk so that we can pass back the id of the quota which was found, and return it to userspace. Today, filesystems such as XFS require getpwent-style iterations, and for systems which have i.e. LDAP backends, this can be very slow, or even impossible if iteration is not allowed in the configuration. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- fs/quota/quota.c | 30 ++++++++++++++++++++++++++++++ include/linux/quota.h | 3 +++ include/uapi/linux/dqblk_xfs.h | 1 + 3 files changed, 34 insertions(+), 0 deletions(-)