Date: Fri, 21 Sep 2018 20:14:57 +0000 From: Brooks Davis <brooks@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: Mark Millard <marklmi@yahoo.com>, "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org> 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? Message-ID: <20180921201457.GB47693@spindle.one-eyed-alien.net> In-Reply-To: <CANCZdfpw33hvK8c1Lkv4=-tcDRfGmcO05VuvOoY%2BwsVM_yu%2B6g@mail.gmail.com> References: <3EBF1660-6CD5-4C80-A36D-4DE945073006@yahoo.com> <3E0252F3-3E65-4A2B-B17A-3BBFBAFFD5F6@yahoo.com> <CANCZdfpw33hvK8c1Lkv4=-tcDRfGmcO05VuvOoY%2BwsVM_yu%2B6g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--DBIVS5p969aUjpLe
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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.]
> >
> > sys/dev/fxp/if_fxp.c uses the statement:
> >
> > cbp->tbd[-1].tb_size =3D htole32(m->m_pkthdr.tso_segsz << 16);
> >
> > But sys/dev/fxp/if_fxpreg.h has the types involved as:
> >
> > 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;
> >
> > /*
> > * 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;
> > };
> >
> > 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.
> >
> > /usr/src/sys/dev/fxp/if_fxp.c has the [-1] assignment in:
> >
> > /* 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;
> > }
> >
> > This cbp->tbd[-1].tb_size use goes back to -r185330 in 2008-Nov-26.
> >
> > This is also when the "+ 1" was added to the:
> >
> > struct fxp_tbd tbd[FXP_NTXSEG + 1]
> >
> > above.
> >
> > clang 7 via xtoolchain-llvm70 reported:
> >
> > --- if_fxp.o ---
> > /usr/src/sys/dev/fxp/if_fxp.c:1630:3: error: array index -1 is before t=
he
> > 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
> >
> > It does look odd to me.
> >
>=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 arra=
y,
> 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 co=
de
> is setting count =3D 0 and tx_threshold to the low order bits of the segm=
ent
> 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.
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.
-- Brooks
--DBIVS5p969aUjpLe
Content-Type: application/pgp-signature; name="signature.asc"
-----BEGIN PGP SIGNATURE-----
iQEcBAEBAgAGBQJbpVFAAAoJEKzQXbSebgfATwcH/iWq3SQ0Fx/PT5Loj5YWUM6A
Hy/i4X2fuEoDvdIekU1DbxBefwILtvL3NAxj4n64/hV8aNQeroQq8hwAh23wqG8P
+q2izscYeGf0BNGWIjEVXB55D1XG7o71K3ZCguA/J6UN5gNy+soqXwwgJz5YDwBn
8kOCUKWS/uk+tjbK/FoDDzcwojFcXEyw5x78mVlAWzJNDsZvJjda7m4bCY/Vu2OK
QWjUvaKGLfQTP4hgQ2Vgg9UUnKObfj1OiNHHbkb0nHN8jjTfgf5l24olwCa/P+Js
XcuRLzjzs40Lki4WbPOnRymxVlsCvUrOnxINrcDVGHZd+qhWQ0tYdswQolsCRXw=
=yySh
-----END PGP SIGNATURE-----
--DBIVS5p969aUjpLe--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180921201457.GB47693>