From owner-svn-src-stable@freebsd.org Thu Jul 30 22:55:31 2020 Return-Path: Delivered-To: svn-src-stable@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 96E95368EEA for ; Thu, 30 Jul 2020 22:55:31 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4BHm370Hxdz43dW for ; Thu, 30 Jul 2020 22:55:30 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: by mail-wm1-f68.google.com with SMTP id q76so6410076wme.4 for ; Thu, 30 Jul 2020 15:55:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=ju1VdFRxCoO59mLAC28bUSbPuU5zG1TcmqC2zSCcAns=; b=RAKckSSdk5kdutlVCMtqI+uWGF4Qhs7JYkmUdyp8ZUGSFdFXJSmYhBTVB4TCjkaZpc OuLoRwYMPElMbWk/GMflk6ExUnNrgn7RBfjrcuMMhIasTszSIyumYzoPQCNvZeoDqWmW m4XGF5kodxV4MUp8QMuGDbXTDn3ONNBw7P9+CPts+w8e10o1TSloZ//PGMRv69EK1rG/ qvI6/jriAjbi0WxFZqwoOxK9I4i7oGqVX9lafj3Lx0Uh1SwVRhcBhokh22xU40ndLyg4 VEjcPXT+0nacvC8moofWiCktPzpfWBWvt9vYEeUy6eB1GwB/yv09HcLwAW9Hq1Q6reBo aaJA== X-Gm-Message-State: AOAM531qm05wt9hpRgEJJQ2934p+A9eOnZn9OHLUU32K5Usz3qCIvql+ Q8t/I0QKGqfUAebMWjHXbg6PSw== X-Google-Smtp-Source: ABdhPJy0MCaEenksD67PeY1obTFbZr6O2AUrIO0qcwBeo+zvYDYaT2X6qGof4HxUxLxVoCMK78mBCA== X-Received: by 2002:a7b:c00c:: with SMTP id c12mr1306063wmb.54.1596149729706; Thu, 30 Jul 2020 15:55:29 -0700 (PDT) Received: from [192.168.149.251] (trinity-students-nat.trin.cam.ac.uk. [131.111.193.104]) by smtp.gmail.com with ESMTPSA id m4sm10171010wmi.48.2020.07.30.15.55.28 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 30 Jul 2020 15:55:29 -0700 (PDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: svn commit: r363625 - stable/12/usr.sbin/mountd From: Jessica Clarke In-Reply-To: Date: Thu, 30 Jul 2020 23:55:28 +0100 Cc: Brooks Davis , Ian Lepore , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-stable@freebsd.org" , "svn-src-stable-12@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <4110A1E0-BAFD-416C-A455-CDB8C2F8E208@freebsd.org> References: <202007272318.06RNIFjV005206@repo.freebsd.org> <4d5b871fad9412661c3914a64c8ca0b7a01d1dc6.camel@freebsd.org> <20200730171016.GC94620@spindle.one-eyed-alien.net> To: Rick Macklem X-Mailer: Apple Mail (2.3608.80.23.2.2) X-Rspamd-Queue-Id: 4BHm370Hxdz43dW X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of jrtc27@jrtc27.com designates 209.85.128.68 as permitted sender) smtp.mailfrom=jrtc27@jrtc27.com X-Spamd-Result: default: False [-2.09 / 15.00]; TO_DN_EQ_ADDR_SOME(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; MV_CASE(0.50)[]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17]; MIME_GOOD(-0.10)[text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-stable@freebsd.org]; ARC_NA(0.00)[]; DMARC_NA(0.00)[freebsd.org]; NEURAL_HAM_LONG(-1.06)[-1.059]; RCVD_COUNT_THREE(0.00)[3]; TO_MATCH_ENVRCPT_SOME(0.00)[]; NEURAL_HAM_SHORT(-0.53)[-0.532]; RCPT_COUNT_SEVEN(0.00)[7]; RCVD_IN_DNSWL_NONE(0.00)[209.85.128.68:from]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; FORGED_SENDER(0.30)[jrtc27@freebsd.org,jrtc27@jrtc27.com]; RWL_MAILSPIKE_POSSIBLE(0.00)[209.85.128.68:from]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; FROM_NEQ_ENVFROM(0.00)[jrtc27@freebsd.org,jrtc27@jrtc27.com]; RCVD_TLS_ALL(0.00)[] X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jul 2020 22:55:31 -0000 On 30 Jul 2020, at 23:38, Rick Macklem wrote: > Brooks Davis wrote: >> On Thu, Jul 30, 2020 at 03:48:34PM +0000, Rick Macklem wrote: >>> Rick Macklem wrote: >>>> Ian Lepore wrote: >>>>> On Thu, 2020-07-30 at 01:52 +0000, Rick Macklem wrote: >>>>>> Brooks Davis wrote: >>>>>>> Author: brooks >>>>>>> Date: Mon Jul 27 23:18:14 2020 >>>>>>> New Revision: 363625 >>>>>>> URL: https://svnweb.freebsd.org/changeset/base/363625 >>>>>>>=20 >>>>>>> Log: >>>>>>> MFC r363439: >>>>>>>=20 >>>>>>> Correct a type-mismatch between xdr_long and the variable "bad". >>>>>>>=20 >>>>>>> [...] >>>>>> --> I can't see how the xdr.c code would work for a machine that = is >>>>>> BIG_ENDIAN and where "long" is 64bits, but we don't have any of >>>>>> those. >>>>>>=20 >>>>>=20 >>>>> mips64 and powerpc64 are both big endian with 64-bit long. >>>> Oops, I didn't know that. In the past, I've run PowerPC and MIPS, = but thought >>>> they both were little endian. (I recall the arches can be run = either way.) >>>>=20 >>>> Anyhow, take a look at head/lib/libc/xdr/xdr.c and it looks to me = like it >>>> has been broken "forever" (ever since we stopped using a K&R = compiler >>>> that would have always made "long" 32bits). >>> OK, I took another look at xdr.c and it isn't broken as I thought. >>>=20 >>> xdr_long() takes a "long *" argument ("long" in Sun XDR is 32bits), >>> but then it only passes it as an argument to XDR_PUTLONG(), which is = actually >>> a call to xdrmem_putlong_aligned() or xdrmem_putlong_unaligned(). >>> For xdrmem_putlong_aligned(), the line is: >>> *(u_int32_t *)xdrs->x_private =3D htonl((u_int32_t)*lp); >>> --> where lp is a "long *" >>>=20 >>> I'll admit I'm not 100% sure if "(u_int32_t)*lp" gets the correct = 32bits of a 64bit >>> long pointer for all arches? (I'm not very good at knowing what type = casts do.) >>> If this is the equivalent of "u_int32_t t; t =3D *lp; htonl(t); then = I think the code is ok? >>> (At least it makes it clear that it is using 32bits of the value = pointed to by the >>> argument.) >>>=20 >>> For xdrmem_putlong_unaligned(), it does the same thing via: >>> u_int32_t l; >>> ?. >>> l =3D htonl((u_int32_t)*lp); >>>=20 >>> --> At least the man page for xdr_long() should be clarified to note = it >>> puts a 32bit quantity on the wire. > I think I will try and come up with a man page patch, noting that = xdr_long() > always puts 32bits on the wire, even if long is 64bits for the arch. >=20 >>>=20 >>>> If anyone has either of these and can set up an NFS server on one = of >>>> them and then try and do an NFSv3 mount that is not allowed, it = would >>>> be interesting to see the packet trace and if the MNT RPC fails, = because >>>> it looks like it will put the high order 32bits on the wire and = they'll >>>> always be 0? >>> It would still be interesting to test this on a 64bit big endian, = but so long as >>> the above cast works, it does look like it works for all arches. >>>=20 >>>> Just to clarify. The behaviour wasn't broken by this commit. I just >>>> don't see how the commit fixes anything? >>> My mistake. Sorry for the noise. >>>=20 >>> I now think the commit is correct since it uses "*lp" to get the = value before >>> casting it down to 32bits. Passing in an "int *" was incorrect. >>>=20 >>> The code does seem to handle "long *" for 64bit arches, although it >>> only puts 32bits "on-the-wire". >>>=20 >>> rick, who was confused because he knew there was only supposed to be >>> 32bits go on the wire. >>=20 >> Thank you for all the analysis. I'd initially changed all the uses >> of bad to use xdr_int(), but switched to this "fix" because it's what >> NetBSD and OpenBSD have been using for over a decade (and there was >> less churn). I'm happy to flip it the other way if that seems more >> correct/less confusing. > I think your current patch is fine. The confusion is w.r.t. what = xdr_long() does > for a 64bit long and I think a man page update may be the way to go. > --> If you look in xdr.c, xdr_int() assigns the value to a long and = then ends > up truncating it back down, similar to xdr_long(). > --> Some of the stuff in xdr.c is pretty scary for 64bit longs, = but it all > seems to work, once you look at it for a while.;-) >=20 >> The previous code does in fact cause a 64-bit load of a pointer to an >> int on 64-bit platforms. I hit this in CheriBSD because that pointer >> had 4-byte bounds. > Yes. The first time I looked at the code (it was late evening), I = misread > ((u_int32_t)*lp) as *((u_int32_t *)lp) and that was why I thought = your patch > was broken. >=20 > Thanks for doing this and sorry about the noise, rick > ps: Personally, I've never understood why ANSI C allowed "long" to be = 64bits > on some arches. I still bump into hassles because the old K&R code = was > written assuming long to be 32bits. Yeah, c.f. XChangeProperty(3) which has a similarly weird interface (but slightly more so) that seriously confused me a few years ago. If you pass 8 for the number of bits to return to you it wants a char, for 16 it wants a short and for 32 it wants a long. XGetWindowProperty(3) also talks about long_offset and long_length when it really means int32_offset/length, or just int_offset/length these days in POSIX. As for _why_ it's done, well, char is 8, short is 16, int is 32 so then why make long 32 too when it could be 64 and still efficient (in terms of instructions, not necessarily space; also means long long is free for int128 on 128-bit machines!). Plus maybe more often than not people wanted a machine word, not a 32-bit value, though by now hopefully people have learnt to use size_t/uintptr_t as appropriate (CHERI work suggests that's _mostly_ true). But then we have to have Windows which decided to keep a 32-bit long, so it's all a mess anyway. Jess