Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753053AbbHSKrN (ORCPT ); Wed, 19 Aug 2015 06:47:13 -0400 Received: from mail-io0-f181.google.com ([209.85.223.181]:34450 "EHLO mail-io0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750935AbbHSKrL (ORCPT ); Wed, 19 Aug 2015 06:47:11 -0400 MIME-Version: 1.0 In-Reply-To: <20150819004714.c06e4a70.akpm@linux-foundation.org> References: <882a878038efb5fed381be5d4817ff44d90703d5.1439904209.git.dpark@posteo.net> <692839ff7158dbb96dd20ce8e36c13f85fa64fd7.1439910753.git.dpark@posteo.net> <20150818164425.76b9df40f94bbd2a57d0d518@linux-foundation.org> <20150819072431.GA2642@posteo.de> <20150819004714.c06e4a70.akpm@linux-foundation.org> Date: Wed, 19 Aug 2015 13:47:11 +0300 Message-ID: Subject: Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t From: Alexey Dobriyan To: Andrew Morton Cc: Dongsu Park , Linux Kernel , linux-fsdevel , Peter Hurley , Josh Triplett , Al Viro , David Howells , Alban Crequy Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3180 Lines: 93 On Wed, Aug 19, 2015 at 10:47 AM, Andrew Morton wrote: > On Wed, 19 Aug 2015 09:24:31 +0200 Dongsu Park 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-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/