Date: Thu, 30 Jul 2020 22:38:04 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Brooks Davis <brooks@freebsd.org> Cc: Ian Lepore <ian@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-stable@freebsd.org" <svn-src-stable@freebsd.org>, "svn-src-stable-12@freebsd.org" <svn-src-stable-12@freebsd.org> Subject: Re: svn commit: r363625 - stable/12/usr.sbin/mountd Message-ID: <QB1PR01MB33642CC86512308A0BAA82ABDD710@QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <20200730171016.GC94620@spindle.one-eyed-alien.net> References: <202007272318.06RNIFjV005206@repo.freebsd.org> <QB1PR01MB3364334927A6502C76338B42DD710@QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM> <4d5b871fad9412661c3914a64c8ca0b7a01d1dc6.camel@freebsd.org> <QB1PR01MB3364F79AB0FD28107B3AD341DD710@QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM> <QB1PR01MB33646EB6938CB0A8A1C271D5DD710@QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM>, <20200730171016.GC94620@spindle.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Brooks Davis wrote:=0A= >On Thu, Jul 30, 2020 at 03:48:34PM +0000, Rick Macklem wrote:=0A= >> Rick Macklem wrote:=0A= >> >Ian Lepore wrote:=0A= >> >>On Thu, 2020-07-30 at 01:52 +0000, Rick Macklem wrote:=0A= >> >>> Brooks Davis wrote:=0A= >> >>> > Author: brooks=0A= >> >>> > Date: Mon Jul 27 23:18:14 2020=0A= >> >>> > New Revision: 363625=0A= >> >>> > URL: https://svnweb.freebsd.org/changeset/base/363625=0A= >> >>> >=0A= >> >>> > Log:=0A= >> >>> > MFC r363439:=0A= >> >>> >=0A= >> >>> > Correct a type-mismatch between xdr_long and the variable "bad".= =0A= >> >>> >=0A= >> >>> > [...]=0A= >> >>> --> I can't see how the xdr.c code would work for a machine that is= =0A= >> >>> BIG_ENDIAN and where "long" is 64bits, but we don't have any of=0A= >> >>> those.=0A= >> >>>=0A= >> >>=0A= >> >>mips64 and powerpc64 are both big endian with 64-bit long.=0A= >> >Oops, I didn't know that. In the past, I've run PowerPC and MIPS, but t= hought=0A= >> >they both were little endian. (I recall the arches can be run either wa= y.)=0A= >> >=0A= >> >Anyhow, take a look at head/lib/libc/xdr/xdr.c and it looks to me like = it=0A= >> >has been broken "forever" (ever since we stopped using a K&R compiler= =0A= >> >that would have always made "long" 32bits).=0A= >> OK, I took another look at xdr.c and it isn't broken as I thought.=0A= >>=0A= >> xdr_long() takes a "long *" argument ("long" in Sun XDR is 32bits),=0A= >> but then it only passes it as an argument to XDR_PUTLONG(), which is act= ually=0A= >> a call to xdrmem_putlong_aligned() or xdrmem_putlong_unaligned().=0A= >> For xdrmem_putlong_aligned(), the line is:=0A= >> *(u_int32_t *)xdrs->x_private =3D htonl((u_int32_t)*lp);=0A= >> --> where lp is a "long *"=0A= >>=0A= >> I'll admit I'm not 100% sure if "(u_int32_t)*lp" gets the correct 32bits= of a 64bit=0A= >> long pointer for all arches? (I'm not very good at knowing what type cas= ts do.)=0A= >> If this is the equivalent of "u_int32_t t; t =3D *lp; htonl(t); then I t= hink the code is ok?=0A= >> (At least it makes it clear that it is using 32bits of the value pointed= to by the=0A= >> argument.)=0A= >>=0A= >> For xdrmem_putlong_unaligned(), it does the same thing via:=0A= >> u_int32_t l;=0A= >> ?.=0A= >> l =3D htonl((u_int32_t)*lp);=0A= >>=0A= >> --> At least the man page for xdr_long() should be clarified to note it= =0A= >> puts a 32bit quantity on the wire.=0A= I think I will try and come up with a man page patch, noting that xdr_long(= )=0A= always puts 32bits on the wire, even if long is 64bits for the arch.=0A= =0A= >>=0A= >> >If anyone has either of these and can set up an NFS server on one of=0A= >> >them and then try and do an NFSv3 mount that is not allowed, it would= =0A= >> >be interesting to see the packet trace and if the MNT RPC fails, becaus= e=0A= >> >it looks like it will put the high order 32bits on the wire and they'll= =0A= >> >always be 0?=0A= >> It would still be interesting to test this on a 64bit big endian, but so= long as=0A= >> the above cast works, it does look like it works for all arches.=0A= >>=0A= >> >Just to clarify. The behaviour wasn't broken by this commit. I just=0A= >> >don't see how the commit fixes anything?=0A= >> My mistake. Sorry for the noise.=0A= >>=0A= >> I now think the commit is correct since it uses "*lp" to get the value b= efore=0A= >> casting it down to 32bits. Passing in an "int *" was incorrect.=0A= >>=0A= >> The code does seem to handle "long *" for 64bit arches, although it=0A= >> only puts 32bits "on-the-wire".=0A= >>=0A= >> rick, who was confused because he knew there was only supposed to be=0A= >> 32bits go on the wire.=0A= >=0A= >Thank you for all the analysis. I'd initially changed all the uses=0A= >of bad to use xdr_int(), but switched to this "fix" because it's what=0A= >NetBSD and OpenBSD have been using for over a decade (and there was=0A= >less churn). I'm happy to flip it the other way if that seems more=0A= >correct/less confusing.=0A= I think your current patch is fine. The confusion is w.r.t. what xdr_long()= does=0A= for a 64bit long and I think a man page update may be the way to go.=0A= --> If you look in xdr.c, xdr_int() assigns the value to a long and then en= ds=0A= up truncating it back down, similar to xdr_long().=0A= --> Some of the stuff in xdr.c is pretty scary for 64bit longs, but i= t all=0A= seems to work, once you look at it for a while.;-)=0A= =0A= >The previous code does in fact cause a 64-bit load of a pointer to an=0A= >int on 64-bit platforms. I hit this in CheriBSD because that pointer=0A= >had 4-byte bounds.=0A= Yes. The first time I looked at the code (it was late evening), I misread= =0A= ((u_int32_t)*lp) as *((u_int32_t *)lp) and that was why I thought your= patch=0A= was broken.=0A= =0A= Thanks for doing this and sorry about the noise, rick=0A= ps: Personally, I've never understood why ANSI C allowed "long" to be 64bit= s=0A= on some arches. I still bump into hassles because the old K&R code was= =0A= written assuming long to be 32bits.=0A= =0A= -- Brooks=0A=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?QB1PR01MB33642CC86512308A0BAA82ABDD710>