From owner-freebsd-hackers@freebsd.org Fri Sep 21 21:22:44 2018 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9947210A492E for ; Fri, 21 Sep 2018 21:22:44 +0000 (UTC) (envelope-from marklmi@yahoo.com) Received: from sonic316-9.consmr.mail.gq1.yahoo.com (sonic316-9.consmr.mail.gq1.yahoo.com [98.137.69.33]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 2373970FC5 for ; Fri, 21 Sep 2018 21:22:44 +0000 (UTC) (envelope-from marklmi@yahoo.com) X-YMail-OSG: OIZ1xO4VM1kpBp9F8VU1zsIL.qjWlHhyb5jpLlCB2Q1hWoL_32ZuW5X8FI03cW8 GVNILgrSHrrs5WQywxr70BFehyZYBgdW.5_FruAaIFGRtXY2cwGwHQLdllr5NIkbfcCb1XMFdXaM cIMIxXowtSfabLPfKFm_x_KrIpS2AJ6v42Skz8XAaqVY.IjVa31dXOlhJLUcOry7TgUEweiiBkMG P4TvJmteimk_v_hfFstA5XqnO7D3GEBvL9F92HRju5z0BQp8G0ynYw4DevObrZ_F9LCPM10RjiyS _n2GhNL2.VmraVONTneMU06jf3CVq_LApmxsw4ADH7HZltNXnVDSa2P7YmCZZBoBY9cQG40VoJsr ES0UWu78OSaF6QOV6.KccGGh0znRxJBr_1sAbeRBQtp.y.D.50FTS6BvbY.Chp6VeVogUu5rNf0_ xD3AE995iZ5W62j_si8kuSi_ND01BEplYYTJWOMWZooERd2X_EfaXNBhdqAxYcpZl4fNrvqjyZSA V_deAVP4mNkngOnP7CwOaN031WVzm13XCDFG057beh2XmeMwbSl6ZlATJirkuP6O5ntsVmaE.afk pX9pR2RWSBPRNhtSdz.oEPwYBWYQ27KyNPYwa7v2GWX7U4NEto08Et.vw6YasH2GEIEAdyJaJi_2 Shk3meaqH2guiVwHF_sbU5aK2OzxTxasgsPRpdYBGlyO5gXq74D9z7vvxqv4QTMeSzaHsRmxJ7Yo b0txkfQRrDCir9lplm6URIcdNqtbA8E0KWQfeRX42SY.WPKN_HJEZQ.ewe0ORyKyhATkRjhdTlaW Faf03KXKxcME5NIeIIhqQaoqi3.Wt1cNuNjkuqoUKzY_G_Ecfz6pU5nOP36Wjjh1P_3D69v3h5hm n5T6sElwTa1d6NvJ2_xxauEGjrPcM.rKILZkNNz9zwI5hE3HG9rdG6nzVimeUEh5LhM__kRb1h_H 5cSwT1SzMdqY1xdW_vlaU.CxOmXFwTh9Re1KelGF4qGLuj3As4t5h6eLQ9IRJOOl2ko1u2NyJmIm 8NvCabqC9 Received: from sonic.gate.mail.ne1.yahoo.com by sonic316.consmr.mail.gq1.yahoo.com with HTTP; Fri, 21 Sep 2018 21:22:35 +0000 Received: from ip70-189-131-151.lv.lv.cox.net (EHLO [192.168.0.105]) ([70.189.131.151]) by smtp420.mail.gq1.yahoo.com (Oath Hermes SMTP Server) with ESMTPA ID 9015c8dae306f8c6d88e7c38e885d1f5; Fri, 21 Sep 2018 21:22:34 +0000 (UTC) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: Re: Does sys/dev/fxp/if_fxp.c using cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16) make any sense? From: Mark Millard In-Reply-To: <20180921201457.GB47693@spindle.one-eyed-alien.net> Date: Fri, 21 Sep 2018 14:22:33 -0700 Cc: Warner Losh , "freebsd-hackers@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <8AB15D63-664F-4B88-B37F-0900717AA2B9@yahoo.com> References: <3EBF1660-6CD5-4C80-A36D-4DE945073006@yahoo.com> <3E0252F3-3E65-4A2B-B17A-3BBFBAFFD5F6@yahoo.com> <20180921201457.GB47693@spindle.one-eyed-alien.net> To: Brooks Davis X-Mailer: Apple Mail (2.3445.9.1) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Sep 2018 21:22:44 -0000 On 2018-Sep-21, at 1:14 PM, Brooks Davis wrote: > On Fri, Sep 21, 2018 at 01:21:59PM -0600, Warner Losh wrote: >> On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers < >> freebsd-hackers@freebsd.org> wrote: >>=20 >>> [I decided that freebsd-hackers might be better for this, >>> under a different wording.] >>>=20 >>> sys/dev/fxp/if_fxp.c uses the statement: >>>=20 >>> cbp->tbd[-1].tb_size =3D htole32(m->m_pkthdr.tso_segsz << 16); >>>=20 >>> But sys/dev/fxp/if_fxpreg.h has the types involved as: >>>=20 >>> struct fxp_cb_tx { >>> uint16_t cb_status; >>> uint16_t cb_command; >>> uint32_t link_addr; >>> uint32_t tbd_array_addr; >>> uint16_t byte_count; >>> uint8_t tx_threshold; >>> uint8_t tbd_number; >>>=20 >>> /* >>> * The following structure isn't actually part of the TxCB, >>> * unless the extended TxCB feature is being used. In this >>> * case, the first two elements of the structure below are >>> * fetched along with the TxCB. >>> */ >>> union { >>> struct fxp_ipcb ipcb; >>> struct fxp_tbd tbd[FXP_NTXSEG + 1]; >>> } tx_cb_u; >>> }; >>>=20 >>> So cbp->tbd is not pointing into the middle of an array. >>> Thus the cbp->tbd[-1].tb_size =3D . . . assignment trashes memory >>> from what I can tell. >>>=20 >>> /usr/src/sys/dev/fxp/if_fxp.c has the [-1] assignment in: >>>=20 >>> /* Configure TSO. */ >>> if (m->m_pkthdr.csum_flags & CSUM_TSO) { >>> cbp->tbd[-1].tb_size =3D htole32(m->m_pkthdr.tso_segsz = << 16); >>> cbp->tbd[1].tb_size |=3D htole32(tcp_payload << 16); >>> cbp->ipcb_ip_schedule |=3D FXP_IPCB_LARGESEND_ENABLE | >>> FXP_IPCB_IP_CHECKSUM_ENABLE | >>> FXP_IPCB_TCP_PACKET | >>> FXP_IPCB_TCPUDP_CHECKSUM_ENABLE; >>> } >>>=20 >>> This cbp->tbd[-1].tb_size use goes back to -r185330 in 2008-Nov-26. >>>=20 >>> This is also when the "+ 1" was added to the: >>>=20 >>> struct fxp_tbd tbd[FXP_NTXSEG + 1] >>>=20 >>> above. >>>=20 >>> clang 7 via xtoolchain-llvm70 reported: >>>=20 >>> --- if_fxp.o --- >>> /usr/src/sys/dev/fxp/if_fxp.c:1630:3: error: array index -1 is = before the >>> beginning of the array [-Werror,-Warray-bounds] >>> cbp->tbd[-1].tb_size =3D htole32(m->m_pkthdr.tso_segsz = << 16); >>> ^ ~~ >>> /usr/src/sys/dev/fxp/if_fxpreg.h:297:3: note: array 'tbd' declared = here >>> struct fxp_tbd tbd[FXP_NTXSEG + 1]; >>> ^ >>> 1 error generated. >>> *** [if_fxp.o] Error code 1 >>>=20 >>> It does look odd to me. >>>=20 >>=20 >> So I did some digging into this a few months ago. >>=20 >> It turns out the code is right, kinda. >>=20 >> So, what's it's doing with the [-1] is as follows: >>=20 >> the sizeof tbd is 8 bytes long. So, we're dereferencing 8 before the = array, >> which points to tbd_array_addr. However tb_size is the second element = of >> that, so that points us at count + tx_threshold + tbd_number. So, = this code >> is setting count =3D 0 and tx_threshold to the low order bits of the = segment >> size and tbd_number to the high order bits. We set the count =3D 0 = later >> anyway, so that part of it isn't so bad. >>=20 >> Since this is in hardware, it may be special meanings for these bits, = and >> this 'trick' is used to just do one write rather than two. I've not = looked >> for the hardware manual to see if this is kosher, but that's what = we're >> doing. It's not as crazy stupid as it sounds at first blush. >=20 > WG14 is discussing making this definitly UB in the next C standard = (many > members think it already is, but this would make it more explict). If > this is required we need to find a better way to express it. Looks to me like the members that think it involves undefined behavior already (as far as C is concerned) get that via (using ISO/IEC 98999:2011 text): E1[E2] is equivalent to (*((E1)+(E2))) via 6.5.2.1 Array subscripting. "6.5.6 Additive Operators" in its semantics section for when a pointer and an integer type are involved says, in part: QUOTE If both the pointer operand and the result point elements of the same array object, or one past the last element of the array object, the evaluation shall not produce overflow; otherwise the behavior is undefined. END QUOTE "6.5.3.1 Address and indirection operators" in its semantics section, says in part: QUOTE If the operand had type "pointer type type", the result has type "type". If an invalid value has been assigned to the pointer, the behavior of = the unary * operator is undefined. END QUOTE That leaves the question if an undefined pointer arithmetic behavior leads to a known-to-be "invalid value" status for the pointer in order to connect the two quotes. I can see room for clarification. But it seems fairly clear that "implementation defined" for the overall classification is not the intent. =3D=3D=3D Mark Millard marklmi at yahoo.com ( dsl-only.net went away in early 2018-Mar)