svn commit: r301932 - head/sys/dev/cxgbe/tom

[Bruce Evans]

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