Messages in this thread | | | Date | Wed, 19 Aug 2015 09:24:31 +0200 | From | Dongsu Park <> | Subject | Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t |
| |
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; > > > > ... > >
| |