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]

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.

===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)