Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753076AbbHSHYP (ORCPT ); Wed, 19 Aug 2015 03:24:15 -0400 Received: from mx02.posteo.de ([89.146.194.165]:47506 "EHLO mx02.posteo.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752407AbbHSHYM (ORCPT ); Wed, 19 Aug 2015 03:24:12 -0400 Date: Wed, 19 Aug 2015 09:24:31 +0200 From: Dongsu Park To: Andrew Morton Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, Peter Hurley , Josh Triplett , Al Viro , David Howells , Alban Crequy Subject: Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t Message-ID: <20150819072431.GA2642@posteo.de> References: <882a878038efb5fed381be5d4817ff44d90703d5.1439904209.git.dpark@posteo.net> <692839ff7158dbb96dd20ce8e36c13f85fa64fd7.1439910753.git.dpark@posteo.net> <20150818164425.76b9df40f94bbd2a57d0d518@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150818164425.76b9df40f94bbd2a57d0d518@linux-foundation.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2910 Lines: 90 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 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/