Does sys/dev/fxp/if_fxp.c using cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16) make any sense?

[Warner Losh]

On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers <
freebsd-hackers at 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.

Warner