Date: Fri, 24 Jun 2016 21:26:39 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r301932 - head/sys/dev/cxgbe/tom Message-ID: <20160624210041.J1013@besplex.bde.org> In-Reply-To: <10550858.Wqzuf5sXBE@ralph.baldwin.cx> References: <201606152108.u5FL8pcO012174@repo.freebsd.org> <10550858.Wqzuf5sXBE@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160624210041.J1013>