| Message ID | 692839ff7158dbb96dd20ce8e36c13f85fa64fd7.1439910753.git.dpark@posteo.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 18 Aug 2015 17:18:19 +0200 Dongsu Park <dpark@posteo.net> wrote: > To allow devpts to be mounted with options of uid/gid of uint32_t, > use kstrtouint() instead of match_int(). Doing that, mounting devpts > with uid or gid > (2^31 - 1) will work as expected, e.g.: > > # mount -t devpts devpts /tmp/devptsdir -o \ > newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693 > > It was originally by reported on systemd github issues: > https://github.com/systemd/systemd/issues/956 > > --- a/fs/devpts/inode.c > +++ b/fs/devpts/inode.c > @@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts) > token = match_token(p, tokens, args); > switch (token) { > case Opt_uid: > - if (match_int(&args[0], &option)) > + { It might be neater to lay this out as case Opt_uid: { > + char *uidstr = args[0].from; > + uid_t uidval; > + int rc = kstrtouint(uidstr, 0, &uidval); This assumes that the architecture/config uses a uint for uid_t. We have no business assuming this - it's an opaque type for a reason. It would be safer to do unsigned long uidl; rc = kstrtoul(uidstr, 0, &uidl); uidval = uidl; > + if (rc) > return -EINVAL; I don't get it. From my reading, kstrtouint->parse_integer() returns "number of characters parsed or -E". So this code won't work. But presumably it *does* work, so why? Also, we should probably return `rc' here if it's negative, to propagate the error which kstrtouint() detected. That's a minor non-back-compatible change but it shouldn't matter. otoh, kstrtouint() likes to return -ERANGE when things go wrong. ERANGE means "Math result not representable", which is a nonsenscal error code in this context. Sigh, why do people keep doing this. > - uid = make_kuid(current_user_ns(), option); > + uid = make_kuid(current_user_ns(), uidval); > if (!uid_valid(uid)) > return -EINVAL; > opts->uid = uid; > opts->setuid = 1; > break; > > ... > -- 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
Hi, thanks for the review. On 18.08.2015 16:44, Andrew Morton wrote: > On Tue, 18 Aug 2015 17:18:19 +0200 Dongsu Park <dpark@posteo.net> wrote: > > > To allow devpts to be mounted with options of uid/gid of uint32_t, > > use kstrtouint() instead of match_int(). Doing that, mounting devpts > > with uid or gid > (2^31 - 1) will work as expected, e.g.: > > > > # mount -t devpts devpts /tmp/devptsdir -o \ > > newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693 > > > > It was originally by reported on systemd github issues: > > https://github.com/systemd/systemd/issues/956 > > > > --- a/fs/devpts/inode.c > > +++ b/fs/devpts/inode.c > > @@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts) > > token = match_token(p, tokens, args); > > switch (token) { > > case Opt_uid: > > - if (match_int(&args[0], &option)) > > + { > > It might be neater to lay this out as > > case Opt_uid: { I'll do it. > > + char *uidstr = args[0].from; > > + uid_t uidval; > > + int rc = kstrtouint(uidstr, 0, &uidval); > > This assumes that the architecture/config uses a uint for uid_t. We > have no business assuming this - it's an opaque type for a reason. It > would be safer to do > > unsigned long uidl; > > rc = kstrtoul(uidstr, 0, &uidl); > uidval = uidl; That's a good point. I'll do it. > > + if (rc) > > return -EINVAL; > > I don't get it. From my reading, kstrtouint->parse_integer() returns > "number of characters parsed or -E". So this code won't work. But > presumably it *does* work, so why? It's probably because kstrtouint() returns just 0 on success. That's what functions in the call chain of kstrtouint() -> kstrtoull() -> _kstrtoull() -> _parse_integer() are actually doing. _parse_integer() actually returns rv, i.e. number of characters parsed. But after that, if there's no error, _kstrtoull() simply returns 0. > Also, we should probably return `rc' here if it's negative, to > propagate the error which kstrtouint() detected. That's a minor > non-back-compatible change but it shouldn't matter. Okay, I also think that we should return rc. I'll do it. > otoh, kstrtouint() likes to return -ERANGE when things go wrong. > ERANGE means "Math result not representable", which is a nonsenscal > error code in this context. Sigh, why do people keep doing this. Hmm, good to know. Thanks, Dongsu > > - uid = make_kuid(current_user_ns(), option); > > + uid = make_kuid(current_user_ns(), uidval); > > if (!uid_valid(uid)) > > return -EINVAL; > > opts->uid = uid; > > opts->setuid = 1; > > break; > > > > ... > > -- 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
On Wed, 19 Aug 2015 09:24:31 +0200 Dongsu Park <dpark@posteo.net> wrote: > > unsigned long uidl; > > > > rc = kstrtoul(uidstr, 0, &uidl); > > uidval = uidl; > > That's a good point. I'll do it. > > > > + if (rc) > > > return -EINVAL; > > > > I don't get it. From my reading, kstrtouint->parse_integer() returns > > "number of characters parsed or -E". So this code won't work. But > > presumably it *does* work, so why? > > It's probably because kstrtouint() returns just 0 on success. > That's what functions in the call chain of kstrtouint() -> kstrtoull() -> > _kstrtoull() -> _parse_integer() are actually doing. > _parse_integer() actually returns rv, i.e. number of characters parsed. > But after that, if there's no error, _kstrtoull() simply returns 0. whoa, wait, I was looking at the -mm tree which changes kstrtouint(): static inline int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res) { return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res); } and * Return number of characters parsed or -E. ... */ #define parse_integer(s, base, val) \ Alexey, doesn't this mean that code which does if (kstrtouint(...)) return -EFOO; will break? Is it intended that parse_integer-convert-*.patch will fix every callsite in the kernel? If so, how do we know there haven't been concurrent additions in -next which need review/conversion? Let alone out-of-tree things... -- 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
On Wed, Aug 19 2015, Andrew Morton <akpm@linux-foundation.org> wrote: > > whoa, wait, I was looking at the -mm tree which changes kstrtouint(): > > static inline int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res) > { > return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res); > } > > and > > * Return number of characters parsed or -E. > ... > */ > #define parse_integer(s, base, val) \ > > > Alexey, doesn't this mean that code which does > > if (kstrtouint(...)) > return -EFOO; > > will break? No, because PARSE_INTEGER_NEWLINE means more than just accepting a trailing newline. It also requires the entire string to be consumed, and changes the return semantics. I suggested splitting those three things into separate flags and letting PARSE_INTEGER_KSTRTOX be a shorthand for those. <http://thread.gmane.org/gmane.linux.kernel/1949066/focus=1949239> Rasmus -- 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
On Wed, Aug 19, 2015 at 10:47 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 19 Aug 2015 09:24:31 +0200 Dongsu Park <dpark@posteo.net> wrote: > >> > unsigned long uidl; >> > >> > rc = kstrtoul(uidstr, 0, &uidl); >> > uidval = uidl; >> >> That's a good point. I'll do it. >> >> > > + if (rc) >> > > return -EINVAL; >> > >> > I don't get it. From my reading, kstrtouint->parse_integer() returns >> > "number of characters parsed or -E". So this code won't work. But >> > presumably it *does* work, so why? >> >> It's probably because kstrtouint() returns just 0 on success. >> That's what functions in the call chain of kstrtouint() -> kstrtoull() -> >> _kstrtoull() -> _parse_integer() are actually doing. >> _parse_integer() actually returns rv, i.e. number of characters parsed. >> But after that, if there's no error, _kstrtoull() simply returns 0. > > whoa, wait, I was looking at the -mm tree which changes kstrtouint(): > > static inline int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res) > { > return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res); > } > > and > > * Return number of characters parsed or -E. > ... > */ > #define parse_integer(s, base, val) \ > > > > Alexey, doesn't this mean that code which does > > if (kstrtouint(...)) > return -EFOO; > > will break? Nothing is supposed to break (even between patches in that series). kstrto*() still returns 0 on success or -EINVAL/-ERANGE on error. Commenting on other things in this thread: > This assumes that the architecture/config > uses a uint for uid_t. We have no business > assuming this - it's an opaque type for a reason. > It would be safer to do > > unsigned long uidl; > > rc = kstrtoul(uidstr, 0, &uidl); > uidval = uidl; This code is not safe at all because unsigned long is wider than unsigned int. "4294967296" will be silently parsed as 0. kstrtou32() should be used in this case. Yes, the names do not match, but C types do. If uid_t as a type is changed, compiler will notice immediately making kstrtou32() more safe. > kstrtouint() likes to return -ERANGE when things go > wrong. ERANGE means "Math result not representable", > which is a nonsenscal error code in this context. ERANGE is there to distinguish between "invalid" and "valid but too big". Typical integer parsing code will accept 289367492873894273894729837428937489273 (a _very_ common bug). C doesn't have bignums, so ERANGE exists. And to teach people that -EINVAL is not the only error value, so they won't hardcode it because in the future we might add new error value because who knows why. > PARSE_INTEGER_NEWLINE means more than > just accepting a trailing newline. Well, maybe it is misnamed, but it is internal implementation detail. People aren't supposed to use it directly. Name can be changed if it is so disturbing. -- 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
On 08/18/2015 11:18 AM, Dongsu Park wrote: > To allow devpts to be mounted with options of uid/gid of uint32_t, > use kstrtouint() instead of match_int(). Doing that, mounting devpts > with uid or gid > (2^31 - 1) will work as expected, e.g.: > > # mount -t devpts devpts /tmp/devptsdir -o \ > newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693 > > It was originally by reported on systemd github issues: > https://github.com/systemd/systemd/issues/956 > > from v1: fix patch format correctly > > Reported-by: Alban Crequy <alban@endocode.com> > Signed-off-by: Dongsu Park <dpark@posteo.net> > --- > fs/devpts/inode.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c > index c35ffdc12bba..49272fae40a7 100644 > --- a/fs/devpts/inode.c > +++ b/fs/devpts/inode.c > @@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts) > token = match_token(p, tokens, args); > switch (token) { > case Opt_uid: > - if (match_int(&args[0], &option)) match_int() => make_kuid/kgid is a widespread pattern in filesystems for handling uid/gid mount parameters. How about adding a for-purpose string-to-uid/gid function, rather than open-coding? Regards, Peter Hurley > + { > + char *uidstr = args[0].from; > + uid_t uidval; > + int rc = kstrtouint(uidstr, 0, &uidval); > + > + if (rc) > return -EINVAL; > - uid = make_kuid(current_user_ns(), option); > + uid = make_kuid(current_user_ns(), uidval); > if (!uid_valid(uid)) > return -EINVAL; > opts->uid = uid; > opts->setuid = 1; > break; > + } > case Opt_gid: > - if (match_int(&args[0], &option)) > + { > + char *gidstr = args[0].from; > + gid_t gidval; > + int rc = kstrtouint(gidstr, 0, &gidval); > + > + if (rc) > return -EINVAL; > - gid = make_kgid(current_user_ns(), option); > + gid = make_kgid(current_user_ns(), gidval); > if (!gid_valid(gid)) > return -EINVAL; > opts->gid = gid; > opts->setgid = 1; > break; > + } > case Opt_mode: > if (match_octal(&args[0], &option)) > return -EINVAL; > -- 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/devpts/inode.c b/fs/devpts/inode.c index c35ffdc12bba..49272fae40a7 100644 --- a/fs/devpts/inode.c +++ b/fs/devpts/inode.c @@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts) token = match_token(p, tokens, args); switch (token) { case Opt_uid: - if (match_int(&args[0], &option)) + { + char *uidstr = args[0].from; + uid_t uidval; + int rc = kstrtouint(uidstr, 0, &uidval); + + if (rc) return -EINVAL; - uid = make_kuid(current_user_ns(), option); + uid = make_kuid(current_user_ns(), uidval); if (!uid_valid(uid)) return -EINVAL; opts->uid = uid; opts->setuid = 1; break; + } case Opt_gid: - if (match_int(&args[0], &option)) + { + char *gidstr = args[0].from; + gid_t gidval; + int rc = kstrtouint(gidstr, 0, &gidval); + + if (rc) return -EINVAL; - gid = make_kgid(current_user_ns(), option); + gid = make_kgid(current_user_ns(), gidval); if (!gid_valid(gid)) return -EINVAL; opts->gid = gid; opts->setgid = 1; break; + } case Opt_mode: if (match_octal(&args[0], &option)) return -EINVAL;
To allow devpts to be mounted with options of uid/gid of uint32_t, use kstrtouint() instead of match_int(). Doing that, mounting devpts with uid or gid > (2^31 - 1) will work as expected, e.g.: # mount -t devpts devpts /tmp/devptsdir -o \ newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693 It was originally by reported on systemd github issues: https://github.com/systemd/systemd/issues/956 from v1: fix patch format correctly Reported-by: Alban Crequy <alban@endocode.com> Signed-off-by: Dongsu Park <dpark@posteo.net> --- fs/devpts/inode.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)