On Wed, 15 Jun 2016, John Baldwin wrote:
On Wednesday, June 15, 2016 09:08:51 PM John Baldwin wrote:
Author: jhb
Date: Wed Jun 15 21:08:51 2016
New Revision: 301932
URL: https://svnweb.freebsd.org/changeset/base/301932
Log:
Use sbused() instead of sbspace() to avoid signed issues.
This description is sort of backwards. sbused() is unsigned (u_int),
so using it gives (un)sign extension bugs. sbspace() has complications
to return signed (long) to avoid (un)sign extension bugs. It has to
be careful to avoid (un)sign extension bugs internally (intermediate
versions of it were broken by removing this care when converting it
from a macro to an inline function). The changed code also mostly
uses signed types, but ...
Inserting a full mbuf with an external cluster into the socket buffer
resulted in sbspace() returning -MLEN. However, since sb_hiwat is
unsigned, the -MLEN value was converted to unsigned in comparisons. As a
result, the socket buffer was never autosized. Note that sb_lowat is signed
to permit direct comparisons with sbspace(), but sb_hiwat is unsigned.
Follow suit with what tcp_output() does and compare the value of sbused()
with sb_hiwat instead.
... direct comparisons with sb_hiwat gives the (un)sign extension bugs since
its foor-shooting type is exposed.
Amusingly (or not), sb_lowat used to be signed as well. Mike Karels
changed it to signed in this commit to BSD:
https://svnweb.freebsd.org/csrg/sys/sys/socketvar.h?revision=43896
The log reads:
add SB_ASYNC in sockbuf, add SB_NOTIFY, SB_NOINTR;
make lowat signed for comparison with sbspace (should probably give up
and make all fields signed
They shoyld all have been signed to begin with.
X Modified: head/sys/dev/cxgbe/tom/t4_cpl_io.c
X ==============================================================================
X --- head/sys/dev/cxgbe/tom/t4_cpl_io.c Wed Jun 15 21:01:53 2016
(r301931)
X +++ head/sys/dev/cxgbe/tom/t4_cpl_io.c Wed Jun 15 21:08:51 2016
(r301932)
X @@ -577,7 +577,7 @@ t4_push_frames(struct adapter *sc, struc
X struct tcpcb *tp = intotcpcb(inp);
X struct socket *so = inp->inp_socket;
X struct sockbuf *sb = &so->so_snd;
X - int tx_credits, shove, compl, space, sowwakeup;
X + int tx_credits, shove, compl, sowwakeup;
X struct ofld_tx_sdesc *txsd = &toep->txsd[toep->txsd_pidx];
X
X INP_WLOCK_ASSERT(inp);
X @@ -652,9 +652,7 @@ t4_push_frames(struct adapter *sc, struc
X }
X }
X
X - space = sbspace(sb);
Here 'space' was signed (int) to hold sbspace(), but this was still a type
mismatch since sbspace() returns signed long.
X -
X - if (space <= sb->sb_hiwat * 3 / 8 &&
Then this broke because the unsigned sb_hiwat is too hard to use.
X + if (sbused(sb) > sb->sb_hiwat * 5 / 8 &&
Is this conversion really correct? sbspace(sb) is only
sb->sb_hiwat - sbused(sb) in the usual case. For a direct translation,
convert sb_hiwat * 3 / 8 to int (it is sure to fit).
Bruce
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
