Does sys/dev/fxp/if_fxp.c using cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso

On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers <

Post by Mark Millard via freebsd-hackers
[I decided that freebsd-hackers might be better for this,
under a different wording.]
cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16);
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.
/* 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.
struct fxp_tbd tbd[FXP_NTXSEG + 1]
above.
--- 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

Mark Millard via freebsd-hackers

2018-09-21 19:55:11 UTC

Post by Mark Millard via freebsd-hackers
[I decided that freebsd-hackers might be better for this,
under a different wording.]
cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16);
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.
/* 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.
struct fxp_tbd tbd[FXP_NTXSEG + 1]
above.
--- 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.
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)

Brooks Davis

2018-09-21 20:14:57 UTC

Post by Warner Losh
On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers <

So I did some digging into this a few months ago.
It turns out the code is right, kinda.
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.

-- Brooks

Mark Millard via freebsd-hackers

2018-09-21 21:22:33 UTC

Post by Warner Losh
On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers <

So I did some digging into this a few months ago.
It turns out the code is right, kinda.
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.

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)

Warner Losh

2018-09-21 21:50:08 UTC

Post by Warner Losh
On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers <

16);

Post by Mark Millard via freebsd-hackers
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.
struct fxp_tbd tbd[FXP_NTXSEG + 1]
above.
--- if_fxp.o ---
/usr/src/sys/dev/fxp/if_fxp.c:1630:3: error: array index -1 is before

the

Post by Mark Millard via freebsd-hackers
beginning of the array [-Werror,-Warray-bounds]
cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz <<

16);

Post by Mark Millard via freebsd-hackers
^ ~~
/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.
the sizeof tbd is 8 bytes long. So, we're dereferencing 8 before the

array,

Post by Warner Losh
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

Post by Warner Losh
is setting count = 0 and tx_threshold to the low order bits of the

segment

Post by Warner Losh
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

Post by Warner Losh
this 'trick' is used to just do one write rather than two. I've not

looked