Date: Fri, 21 Sep 2018 12:55:11 -0700 From: Mark Millard <marklmi@yahoo.com> To: Warner Losh <imp@bsdimp.com> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org>, FreeBSD Toolchain <freebsd-toolchain@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: <EEF930C4-3946-48FC-B152-B87447B2353A@yahoo.com> 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
On 2018-Sep-21, at 12:21 PM, Warner Losh <imp at bsdimp.com> wrote:
>> On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers =
<freebsd-hackers@freebsd.org> wrote:
>> [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
> 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.
Thanks for the explanation. Too bad the code does not document the hack.
Looks like xtoolchain-llvm70 use will require avoiding reporting this as
an error that stops buildkernel, a change for the build environment. Of
course, that may well wait for head to be 13 instead of 12.
=3D=3D=3D
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?EEF930C4-3946-48FC-B152-B87447B2353A>