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-hack...@freebsd.org> wrote:
>> [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 = 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 = . . . 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 = htole32(m->m_pkthdr.tso_segsz << 16);
>> cbp->tbd[1].tb_size |= htole32(tcp_payload << 16);
>> cbp->ipcb_ip_schedule |= 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 the
>> beginning of the array [-Werror,-Warray-bounds]
>> cbp->tbd[-1].tb_size = 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.
>>
> So I did some digging into this a few months ago.
>
> It turns out the code is right, kinda.
>
> So, what's it's doing with the [-1] is as follows:
>
> 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 = 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 = 0 later
> anyway, so that part of it isn't so bad.
>
> 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.
===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)
_______________________________________________
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"
Re: Does sys/dev/fxp/if_fxp.c using cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16) make any sense?
Mark Millard via freebsd-toolchain Fri, 21 Sep 2018 13:16:26 -0700
