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, 3:22 PM Mark Millard <marklmi at yahoo.com> wrote:

> On 2018-Sep-21, at 1:14 PM, Brooks Davis <brooks at freebsd.org> 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 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.
> >
> > 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.
>

I think the intent of the code is my explanation. At the time the code was
written, it was what the common compiler interpretation would have been. If
that is changed under the rubric of pedantry, then we'll need to change the
code. But we'll need to get the data sheet. NetBSD doesn't implement this
feature. Linux's driver is a bit opaque at first glance, but it might be a
source of details as well. I only spent a minute or two looking.

Warner

>