Re: [PATCH] userns: Add basic quota support v4
Dave thank you earlier taking some time to do review. It made me realize that my code was not as mature as it needed to be. That said you missed a lot of important details, and I will aim at to address some of the highlights. Precedent in naming. that has been merged since 3.5, earlier there is make_pte. Much of your review sounds like you are unhappy with the general infrastructure that was built for user namespaces, and already merged. Short of an overwhelmingly better suggestion at this point I am not going to revisit naming. Passing structures by value is a core part of the design and a necessity for strong type safety in C, and again already in common use in the kernel. Using BUG on code paths that can not be triggered, simply means that in that rare and unlikely event there is a clear trace to follow, and xfs already has plenty of invocations of BUG, some of which I managed to trigger with the xfs test suite. So I can not find much sense in your arguments about being clear that something is a BUG. Your comments on where xfs uses projid are just laughable. >> 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. > > I'd like to think you are joking, given that the patch series is all > about using consistent, verifiable identifiers... :/ The patch series is all about implementing the user namespace. To make it hard to miss places where conversions need to between the internal kernel form and the form stored in filesystems or the form that userspace uses I use types that are not assignment compatible. Pushing the types deeper in the code keeps me honest and let's me find cases I was not expecting like the ioctl interface of xfs that largely duplicates much of the vfs functionality. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] userns: Add basic quota support v4
Dave thank you earlier taking some time to do review. It made me realize that my code was not as mature as it needed to be. That said you missed a lot of important details, and I will aim at to address some of the highlights. Precedent in naming. uidgid.h that has been merged since 3.5, earlier there is make_pte. Much of your review sounds like you are unhappy with the general infrastructure that was built for user namespaces, and already merged. Short of an overwhelmingly better suggestion at this point I am not going to revisit naming. Passing structures by value is a core part of the design and a necessity for strong type safety in C, and again already in common use in the kernel. Using BUG on code paths that can not be triggered, simply means that in that rare and unlikely event there is a clear trace to follow, and xfs already has plenty of invocations of BUG, some of which I managed to trigger with the xfs test suite. So I can not find much sense in your arguments about being clear that something is a BUG. Your comments on where xfs uses projid are just laughable. 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. I'd like to think you are joking, given that the patch series is all about using consistent, verifiable identifiers... :/ The patch series is all about implementing the user namespace. To make it hard to miss places where conversions need to between the internal kernel form and the form stored in filesystems or the form that userspace uses I use types that are not assignment compatible. Pushing the types deeper in the code keeps me honest and let's me find cases I was not expecting like the ioctl interface of xfs that largely duplicates much of the vfs functionality. Eric -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] userns: Add basic quota support v4
Dave Chinner writes: > On Wed, Aug 29, 2012 at 02:31:26AM -0700, Eric W. Biederman wrote: >> >> Dave thanks for taking the time to take a detailed look at this code. >> >> Dave Chinner writes: >> >> > On Tue, Aug 28, 2012 at 12:09:56PM -0700, Eric W. Biederman wrote: >> > 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. > > But no actual regression testing. You're messing with code that I > will have to triage when it goes wrong for a user, so IMO your code > has to pass the same bar as the code I write has to pass for review > - please regression test your code and write new regression tests > for new functionality. I like the idea of regression tests. In practice and also with xfstests I find that I spend lots of time debugging and fixing and improving tests and at the end of the day I find regression tests tell me very little. But I did figure I should give them a try since I have a rather substantial xfs patch in my queue. I added tests 111 and 232 to the expunged file because the don't run to completion. ltp/rwtest.sh needs to be run with #!/bin/bash instead of #!/bin/sh as it contains serveral bashisms. You need to have gawk installed instead of mawk because of a non-posix call to asort somewhere in the test framework. On my branch userns-always-map-user-v53 or on v3.6-rc1+ xfs: check for possible overflow in xfs_ioc_trim xfs: unlock the AGI buffer when looping in xfs_dialloc xfs: fix uninitialised variable in xfs_rtbuf_get() When I run ./check in the from xfstests I get Tue Sep 4 05:06:12 PDT 2012 001 002 003 004 005 006 007 008 009 010 011 012 013 014 015 016 017 018 019 020 021 022 023 024 025 026 027 028 029 030 031 032 033 034 035 036 037 038 039 040 041 042 043 044 045 046 047 048 049 050 051 052 053 054 055 056 057 058 059 060 061 062 063 064 065 066 067 068 069 070 071 072 073 074 075 076 077 078 079 080 081 082 083 084 085 086 087 088 089 090 091 092 093 094 095 096 097 098 099 100 101 102 103 104 105 106 107 108 109 110 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 233 234 235 236 237 238 239 240 241 242 243 244 245 246 247 248 249 250 251 252 253 254 255 256 257 258 259 260 261 262 263 264 265 266 267 268 269 270 271 272 273 274 275 276 277 278 279 280 281 282 283 284 285 286 Not run:2 Failures: 018 081 082 106 107 136 167 171 206 219 229 234 250 280 Failed 14 of 165 tests But since the results came back the same either way I think the tests told me all they can. The 14 failed tests and 1 bug don't seem to say good things about xfs in general though. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] userns: Add basic quota support v4
Dave Chinner da...@fromorbit.com writes: On Wed, Aug 29, 2012 at 02:31:26AM -0700, Eric W. Biederman wrote: Dave thanks for taking the time to take a detailed look at this code. Dave Chinner da...@fromorbit.com writes: On Tue, Aug 28, 2012 at 12:09:56PM -0700, Eric W. Biederman wrote: 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. But no actual regression testing. You're messing with code that I will have to triage when it goes wrong for a user, so IMO your code has to pass the same bar as the code I write has to pass for review - please regression test your code and write new regression tests for new functionality. I like the idea of regression tests. In practice and also with xfstests I find that I spend lots of time debugging and fixing and improving tests and at the end of the day I find regression tests tell me very little. But I did figure I should give them a try since I have a rather substantial xfs patch in my queue. I added tests 111 and 232 to the expunged file because the don't run to completion. ltp/rwtest.sh needs to be run with #!/bin/bash instead of #!/bin/sh as it contains serveral bashisms. You need to have gawk installed instead of mawk because of a non-posix call to asort somewhere in the test framework. On my branch userns-always-map-user-v53 or on v3.6-rc1+ xfs: check for possible overflow in xfs_ioc_trim xfs: unlock the AGI buffer when looping in xfs_dialloc xfs: fix uninitialised variable in xfs_rtbuf_get() When I run ./check in the from xfstests I get Tue Sep 4 05:06:12 PDT 2012 001 002 003 004 005 006 007 008 009 010 011 012 013 014 015 016 017 018 019 020 021 022 023 024 025 026 027 028 029 030 031 032 033 034 035 036 037 038 039 040 041 042 043 044 045 046 047 048 049 050 051 052 053 054 055 056 057 058 059 060 061 062 063 064 065 066 067 068 069 070 071 072 073 074 075 076 077 078 079 080 081 082 083 084 085 086 087 088 089 090 091 092 093 094 095 096 097 098 099 100 101 102 103 104 105 106 107 108 109 110 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 233 234 235 236 237 238 239 240 241 242 243 244 245 246 247 248 249 250 251 252 253 254 255 256 257 258 259 260 261 262 263 264 265 266 267 268 269 270 271 272 273 274 275 276 277 278 279 280 281 282 283 284 285 286 Not run:2 Failures: 018 081 082 106 107 136 167 171 206 219 229 234 250 280 Failed 14 of 165 tests But since the results came back the same either way I think the tests told me all they can. The 14 failed tests and 1 bug don't seem to say good things about xfs in general though. Eric -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] userns: Add basic quota support v4
On Wed, Aug 29, 2012 at 02:31:26AM -0700, Eric W. Biederman wrote: > > Dave thanks for taking the time to take a detailed look at this code. > > Dave Chinner 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. Not from a quota perspective. The only thing the quota code really cares about is the quota identifier, not the encoding. Fundamentally, from_kqid() doen't tell me anything about what I'm getting from the kqid. There's code all over the place that used the "_to_" convention because it's obvious what is being converted from/to. e.g. cpu_to_beXX, compat_to_ptr, dma_to_phys, pfn_to_page, etc. Best practises say "follow existing conventions". > 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. For those of us that have to look at it once every few months, following the same conventions as all the other code in the kernel (i.e. kqid_to_id()) tells me everything I need to know without having to go through the process of looking up the unusual from_kqid() function and then from_kuid() to find out what it is actually doing > >> 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. There's plenty of precendence in other code that converts format. A very common convention that is used everywhere is DEFINE_...(). That would be make the code easier to grasp than "make...". > >> 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. But no actual regression testing. You're messing with code that I will have to triage when it goes wrong for a user, so IMO your code has to pass the same bar as the code I write has to pass for review - please regression test your code and write new regression tests for new functionality. > > 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 Asking people to run the entire regression test suite differently and with special setup magic won't get the code tested regularly. Writing a new, self contained test that exercises quota in multiple namespaces simultaneously is what is needed - that way people who don't even know that namespaces exist will be regression testing it... > >> --- a/include/linux/quota.h > >> +++ b/include/linux/quota.h > >> @@ -181,10 +181,161 @@ enum { > >> #include > >> > >> #include > >> +#include > >> > >> 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
Re: [PATCH] userns: Add basic quota support v4
On Wed, Aug 29, 2012 at 02:31:26AM -0700, Eric W. Biederman wrote: Dave thanks for taking the time to take a detailed look at this code. Dave Chinner da...@fromorbit.com 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. Not from a quota perspective. The only thing the quota code really cares about is the quota identifier, not the encoding. Fundamentally, from_kqid() doen't tell me anything about what I'm getting from the kqid. There's code all over the place that used the format_to_other format convention because it's obvious what is being converted from/to. e.g. cpu_to_beXX, compat_to_ptr, dma_to_phys, pfn_to_page, etc. Best practises say follow existing conventions. 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. For those of us that have to look at it once every few months, following the same conventions as all the other code in the kernel (i.e. kqid_to_id()) tells me everything I need to know without having to go through the process of looking up the unusual from_kqid() function and then from_kuid() to find out what it is actually doing 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. There's plenty of precendence in other code that converts format. A very common convention that is used everywhere is DEFINE_...(). That would be make the code easier to grasp than make 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. But no actual regression testing. You're messing with code that I will have to triage when it goes wrong for a user, so IMO your code has to pass the same bar as the code I write has to pass for review - please regression test your code and write new regression tests for new functionality. 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 Asking people to run the entire regression test suite differently and with special setup magic won't get the code tested regularly. Writing a new, self contained test that exercises quota in multiple namespaces simultaneously is what is needed - that way people who don't even know that namespaces exist will be regression testing it... --- 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
Re: [PATCH] userns: Add basic quota support v4
Dave thanks for taking the time to take a detailed look at this code. Dave Chinner 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(_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(_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
Re: [PATCH] userns: Add basic quota support v4
Dave thanks for taking the time to take a detailed look at this code. Dave Chinner da...@fromorbit.com 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.
Re: [PATCH] userns: Add basic quota support v4
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); > make_kqid_invalid, make_kqid_uid, make_kqid_gid. and these named something like uid_to_kqid() > 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? 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? > > 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? > 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(_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(_user_ns, qid), xfs_quota_type(qid.type), fdq); 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? > > 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(_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 :
[PATCH] userns: Add basic quota support v4
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, make_kqid_invalid, make_kqid_uid, make_kqid_gid. 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. 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. Cc: Steven Whitehouse Cc: Mark Fasheh Cc: Joel Becker Cc: Ben Myers Cc: Alex Elder Cc: Jan Kara Cc: Dmitry Monakhov Signed-off-by: Eric W. Biederman --- fs/ext3/super.c |2 +- fs/ext4/super.c |2 +- fs/gfs2/quota.c | 52 +-- fs/ocfs2/file.c |6 +- fs/ocfs2/quota_global.c | 43 +++- fs/ocfs2/quota_local.c | 15 +++-- fs/quota/dquot.c | 116 fs/quota/netlink.c | 10 ++- fs/quota/quota.c | 28 ++-- fs/quota/quota_tree.c| 23 --- fs/quota/quota_v1.c | 12 ++-- fs/quota/quota_v2.c | 26 --- fs/xfs/xfs_quotaops.c| 14 ++-- fs/xfs/xfs_trans_dquot.c |8 ++- include/linux/quota.h| 162 -- include/linux/quotaops.h |6 +- init/Kconfig |2 - 17 files changed, 364 insertions(+), 163 deletions(-) diff --git a/fs/ext3/super.c b/fs/ext3/super.c index ff9bcdc..73e42f5 100644 --- a/fs/ext3/super.c +++ b/fs/ext3/super.c @@ -2814,7 +2814,7 @@ static int ext3_statfs (struct dentry * dentry, struct kstatfs * buf) static inline struct inode *dquot_to_inode(struct dquot *dquot) { - return sb_dqopt(dquot->dq_sb)->files[dquot->dq_type]; + return sb_dqopt(dquot->dq_sb)->files[dquot->dq_id.type]; } static int ext3_write_dquot(struct dquot *dquot) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index d76ec82..78e6036 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -4796,7 +4796,7 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf) static inline struct inode *dquot_to_inode(struct dquot *dquot) { - return sb_dqopt(dquot->dq_sb)->files[dquot->dq_type]; + return sb_dqopt(dquot->dq_sb)->files[dquot->dq_id.type]; } static int ext4_write_dquot(struct dquot *dquot) diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c index a3bde91..e27f8d6 100644 --- a/fs/gfs2/quota.c +++ b/fs/gfs2/quota.c @@ -1057,6 +1057,8 @@ int gfs2_quota_check(struct gfs2_inode *ip, u32 uid, u32 gid) return 0; for (x = 0; x < ip->i_res->rs_qa_qd_num; x++) { + int qtype; + struct kqid qid; qd = ip->i_res->rs_qa_qd[x]; if (!((qd->qd_id == uid && test_bit(QDF_USER, >qd_flags)) || @@ -1068,11 +1070,12 @@ int gfs2_quota_check(struct gfs2_inode *ip, u32 uid, u32 gid) value += qd->qd_change; spin_unlock(_lru_lock); + qtype = test_bit(QDF_USER, >qd_flags) ? USRQUOTA : GRPQUOTA; + qid = make_kqid(_user_ns, qtype, qd->qd_id); if (be64_to_cpu(qd->qd_qb.qb_limit) && (s64)be64_to_cpu(qd->qd_qb.qb_limit) < value) { print_message(qd, "exceeded"); - quota_send_warning(test_bit(QDF_USER, >qd_flags) ? - USRQUOTA : GRPQUOTA, qd->qd_id, - sdp->sd_vfs->s_dev, QUOTA_NL_BHARDWARN); + quota_send_warning(qid, sdp->sd_vfs->s_dev, + QUOTA_NL_BHARDWARN); error = -EDQUOT; break; @@ -1081,9 +1084,8 @@ int gfs2_quota_check(struct gfs2_inode *ip, u32 uid,
[PATCH] userns: Add basic quota support v4
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, make_kqid_invalid, make_kqid_uid, make_kqid_gid. 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. 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. Cc: Steven Whitehouse swhit...@redhat.com Cc: Mark Fasheh mfas...@suse.com Cc: Joel Becker jl...@evilplan.org Cc: Ben Myers b...@sgi.com Cc: Alex Elder el...@kernel.org Cc: Jan Kara j...@suse.cz Cc: Dmitry Monakhov dmonak...@openvz.org Signed-off-by: Eric W. Biederman ebied...@xmission.com --- fs/ext3/super.c |2 +- fs/ext4/super.c |2 +- fs/gfs2/quota.c | 52 +-- fs/ocfs2/file.c |6 +- fs/ocfs2/quota_global.c | 43 +++- fs/ocfs2/quota_local.c | 15 +++-- fs/quota/dquot.c | 116 fs/quota/netlink.c | 10 ++- fs/quota/quota.c | 28 ++-- fs/quota/quota_tree.c| 23 --- fs/quota/quota_v1.c | 12 ++-- fs/quota/quota_v2.c | 26 --- fs/xfs/xfs_quotaops.c| 14 ++-- fs/xfs/xfs_trans_dquot.c |8 ++- include/linux/quota.h| 162 -- include/linux/quotaops.h |6 +- init/Kconfig |2 - 17 files changed, 364 insertions(+), 163 deletions(-) diff --git a/fs/ext3/super.c b/fs/ext3/super.c index ff9bcdc..73e42f5 100644 --- a/fs/ext3/super.c +++ b/fs/ext3/super.c @@ -2814,7 +2814,7 @@ static int ext3_statfs (struct dentry * dentry, struct kstatfs * buf) static inline struct inode *dquot_to_inode(struct dquot *dquot) { - return sb_dqopt(dquot-dq_sb)-files[dquot-dq_type]; + return sb_dqopt(dquot-dq_sb)-files[dquot-dq_id.type]; } static int ext3_write_dquot(struct dquot *dquot) diff --git a/fs/ext4/super.c b/fs/ext4/super.c index d76ec82..78e6036 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -4796,7 +4796,7 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf) static inline struct inode *dquot_to_inode(struct dquot *dquot) { - return sb_dqopt(dquot-dq_sb)-files[dquot-dq_type]; + return sb_dqopt(dquot-dq_sb)-files[dquot-dq_id.type]; } static int ext4_write_dquot(struct dquot *dquot) diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c index a3bde91..e27f8d6 100644 --- a/fs/gfs2/quota.c +++ b/fs/gfs2/quota.c @@ -1057,6 +1057,8 @@ int gfs2_quota_check(struct gfs2_inode *ip, u32 uid, u32 gid) return 0; for (x = 0; x ip-i_res-rs_qa_qd_num; x++) { + int qtype; + struct kqid qid; qd = ip-i_res-rs_qa_qd[x]; if (!((qd-qd_id == uid test_bit(QDF_USER, qd-qd_flags)) || @@ -1068,11 +1070,12 @@ int gfs2_quota_check(struct gfs2_inode *ip, u32 uid, u32 gid) value += qd-qd_change; spin_unlock(qd_lru_lock); + qtype = test_bit(QDF_USER, qd-qd_flags) ? USRQUOTA : GRPQUOTA; + qid = make_kqid(init_user_ns, qtype, qd-qd_id); if (be64_to_cpu(qd-qd_qb.qb_limit) (s64)be64_to_cpu(qd-qd_qb.qb_limit) value) { print_message(qd, exceeded); - quota_send_warning(test_bit(QDF_USER, qd-qd_flags) ? - USRQUOTA : GRPQUOTA, qd-qd_id, - sdp-sd_vfs-s_dev, QUOTA_NL_BHARDWARN); + quota_send_warning(qid, sdp-sd_vfs-s_dev, + QUOTA_NL_BHARDWARN); error =
Re: [PATCH] userns: Add basic quota support v4
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); make_kqid_invalid, make_kqid_uid, make_kqid_gid. and these named something like uid_to_kqid() 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? 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? 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? 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); 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? 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)); +