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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
