From owner-svn-src-all@freebsd.org Thu Jul 30 22:38:07 2020 Return-Path: Delivered-To: svn-src-all@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 5BFE1368D34; Thu, 30 Jul 2020 22:38:07 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from CAN01-TO1-obe.outbound.protection.outlook.com (mail-eopbgr670054.outbound.protection.outlook.com [40.107.67.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mail.protection.outlook.com", Issuer "GlobalSign Organization Validation CA - SHA256 - G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4BHlg26zTdz430B; Thu, 30 Jul 2020 22:38:06 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YYF91y3Y/CXMyePEE+kYYz96fnOZPFQamWtO/I4Q2eOHEDuNO/pXn9MEHZhRlTJYLXahF4z4FruE2RwK7hIVS9MEQoQsdP53K7yvVPWEk+n5demncgzwcqo6cJfhFI8gYIjceLCHuGJ1ki6QrN1F9U+uVZQz41Pdnr5jIug8xop+FtVyRLyxOIYh167vrcn6mqWZzH0rKGY83qqJ6/P4Em14mRjyG0szWnt24QJNpbkwBbyheGN7kYOwgl5R2ZqfS5L4hLEbx/nDHO5mulhO6NZnADO0Fs7yfXzVOn+EUpdbMznu3akaNoNvDg+NEH0DItult8mcPzdaK3CJnUclUg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=nNrRhr4G5sufYSwcoRyZTVnUlce0Bo/4cGlqg7R39Ug=; b=Pt9se5sl7fvkb8t6kZqqNe0erV3AfUO81w1VFYkTjkQkjyuIS7Hc1xk41NDQHFmnhbuR9eMMY2dOWbORdNECUa40AyyrF0URzQwj/bGVbKSlfDlTW0IzBTtzYXbvt2HkC6r9A0nVy7qo+pAy8lHgWQYp3gMkp5vF7hQ3oPhwjkTykRGr/T/VFh2LFWXAKDMFVcfRG8IBdXgFiSQ0gEBefpFHV/kq68Irj7fW86PHtrL1Hb7lLn9XV6jO3yWp8LrzfNc5eogLlY/elMVjpSj1HWTTQNf8N5hDxGaa8APAbg8BYq9J8q1a0/5LEv2exN+s85dL3FM1fr0gt4tKQQGsjw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=uoguelph.ca; dmarc=pass action=none header.from=uoguelph.ca; dkim=pass header.d=uoguelph.ca; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=uoguelph.ca; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=nNrRhr4G5sufYSwcoRyZTVnUlce0Bo/4cGlqg7R39Ug=; b=rUCyB+6pIV0cnlYRbKNBHmucNHjmHMLHCkj0N29Cg2fn3YeRbO0JKJlpszUj70KLlMj1pKCCHKqLB8lFo11nW85tiVqnG+eOWtJ6bSkCgphHH0Q2/6u01a/Hofc+6RlAUu+onq6VV+J2EQo2TbQmJJp5FWmg5zrm9WpRx9TEw8GrwhzHzwOaN9C+aCQ4Sj4ncRoukGO5WTqqYvl13Zy+QSsUtTCvpe7BsNORPPHPxkOdz4p2IxNiNitqme1vKw7GvJeNXX54vCsJPXvRUCtqK6CeRD0A4t+dF2Qm7cyHr1CUsJll56TjeUBo4eAoseRi0HNKH/NZ/cCM67GSj0MxsQ== Received: from QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:c00:38::14) by QB1PR01MB2515.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:c00:36::28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3216.26; Thu, 30 Jul 2020 22:38:04 +0000 Received: from QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM ([fe80::60f3:4ca2:8a4a:1e91]) by QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM ([fe80::60f3:4ca2:8a4a:1e91%7]) with mapi id 15.20.3216.033; Thu, 30 Jul 2020 22:38:04 +0000 From: Rick Macklem To: Brooks Davis CC: Ian Lepore , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-stable@freebsd.org" , "svn-src-stable-12@freebsd.org" Subject: Re: svn commit: r363625 - stable/12/usr.sbin/mountd Thread-Topic: svn commit: r363625 - stable/12/usr.sbin/mountd Thread-Index: AQHWZGw7l3AA2GANGkmYHY+WWlMRT6kfVzKsgAAiMQCAADpv4oAAiFtWgAAjkQCAAFdClw== Date: Thu, 30 Jul 2020 22:38:04 +0000 Message-ID: References: <202007272318.06RNIFjV005206@repo.freebsd.org> <4d5b871fad9412661c3914a64c8ca0b7a01d1dc6.camel@freebsd.org> , <20200730171016.GC94620@spindle.one-eyed-alien.net> In-Reply-To: <20200730171016.GC94620@spindle.one-eyed-alien.net> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 88f6fb09-c62a-46d2-43b6-08d834d9396a x-ms-traffictypediagnostic: QB1PR01MB2515: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: Vhy9BBG6oO5huWh3erBx3rs5VirzEYUkiZzxGYxT0BW0ebAhBxeMqEOleelXIjOtZ75QMeQCOzW4zHras6+XSD53xiAiWhl3fBYJyyEc4+1KGFawdQW8CMvSijOZntR+vmH7ZXMqp9GSLkuBrTCYKE4crsMl8wEDUW9GrDzKRAoDO7ohTuW3CFUNv8eSPbKALIfkCXtF2weZwE1SQLNb6s8ilxxczzqMQJ7nRGj5pKOuVKJ4hlyFlLWVzJaEugPDXtyJuih2l7sduoE2hon3aSlw1Kb0Pr02w9naN7sat9c5cZ3Nqft5UmUVDFAV8Ro/sroRd2VBKrY3Z8OlZCWpMLnWzX9yaYbAqs8aXGsKsgPxXWYe31Toz/XCBZ7Mte50Ih8MDXbzXUROvdNUrJwovQ== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM; PTR:; CAT:NONE; SFTY:; SFS:(346002)(366004)(376002)(396003)(39860400002)(136003)(7696005)(966005)(33656002)(76116006)(450100002)(478600001)(6916009)(86362001)(5660300002)(83380400001)(8936002)(9686003)(2906002)(186003)(54906003)(8676002)(55016002)(6506007)(786003)(64756008)(91956017)(71200400001)(66556008)(66946007)(66446008)(4326008)(66476007)(52536014)(316002); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: yJEJzL/no5UUDMJ2ak1VTp4eAgHXkeiy18Y6ueEAkhQTn6Waj8pi8SDiU7bims7mJ7ocMsbs4YglyMMikSCIar+2SQ1WScPM481De9ihaWv2H6lqlm1aBNubm03aivChOZVWDcnfZqrmQbXkQ+z3tqzdHJmZTggOA7t/nd7o5KDQ3T1jukPQF3zd89gtkC8rjJn5q6/4hV8agcQbUQpFOx+/CvNlIkcB8yL7pOy7ZLvqrNPNv6WngFLkaLBMI+aDv4e4vy6nMZHq/Y08jeyBsaxsaIoefX+a36TkusTMR68hYW+haLj4Mp24CdfKPiUcbWLwydtHS5M3Upv59SsEQ+p64qCic71uWcg3f9HsfpjEgZnweWwfx/uXWTLDANMEZvO2s1caCOZWmrO4b7UjB8zqHHnzUaY9hpP0FsXRvryLfArOo4uBunjRPG+Bvd8WPIpJlUrj924P+FaHdje0I/k903jelS4kEc64DG+b1Cfe/PU4tc/4C06hrax6pi75BsFVg8dJkLChkYnY5N0aLzhoYWJhuDxUSfVw1jYmtsQ= x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: uoguelph.ca X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: QB1PR01MB3364.CANPRD01.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-Network-Message-Id: 88f6fb09-c62a-46d2-43b6-08d834d9396a X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Jul 2020 22:38:04.7349 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: be62a12b-2cad-49a1-a5fa-85f4f3156a7d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ThcPlmXmcC7vHK0ixmaLvxDBjQzWJMSyPT23+Bn6PWfBxUuZSMqlISpfZuQUjLwmtjjCPXQDZmX2AVqWmhUwUQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: QB1PR01MB2515 X-Rspamd-Queue-Id: 4BHlg26zTdz430B X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:8075, ipnet:40.104.0.0/14, country:US] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jul 2020 22:38:07 -0000 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=