[PATCH 1/5]: DCCP Fix use of invalid loss intervals

[PATCH 1/5]: DCCP Fix use of invalid loss intervals

[Ian McDonald]

I've been trying to fix up performance for CCID3 and found this little
bug that means the comparison was invalid and we were using invalid loss
intervals when we shouldn't of.

Signed-off-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
---
diff --git a/net/dccp/ccids/lib/loss_interval.c b/net/dccp/ccids/lib/loss_interval.c
index 0a0baef..372d7e7 100644
--- a/net/dccp/ccids/lib/loss_interval.c
+++ b/net/dccp/ccids/lib/loss_interval.c
@@ -91,7 +91,7 @@ u32 dccp_li_hist_calc_i_mean(struct list_head *list)
 	u32 w_tot  = 0;
 
 	list_for_each_entry_safe(li_entry, li_next, list, dccplih_node) {
-		if (li_entry->dccplih_interval != ~0) {
+		if (li_entry->dccplih_interval != ~0U) {
 			i_tot0 += li_entry->dccplih_interval * dccp_li_hist_w[i];
 			w_tot  += dccp_li_hist_w[i];
 			if (i != 0)

Re: [PATCH 1/5]: DCCP Fix use of invalid loss intervals

[Gerrit Renker]

Quoting Ian McDonald:
|  Signed-off-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>

Re: [PATCH 1/5]: DCCP Fix use of invalid loss intervals

[Eddie Kohler]

Ian (catching up slowly slowly), here is a nit as nitty as they come.

This diff seems strange to me, since ~ actually does the same thing on 
integers and unsigned integers.  (This code:

     printf("%u %u\n", ~0, ~0U);

will print the same thing twice.)

Perhaps dccplih_interval is a 64-bit number?  In which case you want to 
say something like ~0ULL?

Eddie


Ian McDonald wrote:
> I've been trying to fix up performance for CCID3 and found this little
> bug that means the comparison was invalid and we were using invalid loss
> intervals when we shouldn't of.
> 
> Signed-off-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
> ---
> diff --git a/net/dccp/ccids/lib/loss_interval.c b/net/dccp/ccids/lib/loss_interval.c
> index 0a0baef..372d7e7 100644
> --- a/net/dccp/ccids/lib/loss_interval.c
> +++ b/net/dccp/ccids/lib/loss_interval.c
> @@ -91,7 +91,7 @@ u32 dccp_li_hist_calc_i_mean(struct list_head *list)
>  	u32 w_tot  = 0;
>  
>  	list_for_each_entry_safe(li_entry, li_next, list, dccplih_node) {
> -		if (li_entry->dccplih_interval != ~0) {
> +		if (li_entry->dccplih_interval != ~0U) {
>  			i_tot0 += li_entry->dccplih_interval * dccp_li_hist_w[i];
>  			w_tot  += dccp_li_hist_w[i];
>  			if (i != 0)
> -
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5]: DCCP Fix use of invalid loss intervals

[Ian McDonald]

On 1/5/07, Eddie Kohler <kohler@cs.ucla.edu> wrote:
> Ian (catching up slowly slowly), here is a nit as nitty as they come.
>
> This diff seems strange to me, since ~ actually does the same thing on
> integers and unsigned integers.  (This code:
>
>      printf("%u %u\n", ~0, ~0U);
>
> will print the same thing twice.)
>
> Perhaps dccplih_interval is a 64-bit number?  In which case you want to
> say something like ~0ULL?
>
> Eddie

Printing gives them the same result as you are using a %u mask. If you
do it with a %d mask you will get a different result.

And that is the issue dccp_lih_interval is unsigned 32 bit and ~0 is a
signed number and is large negative and they therefore can't be equal.

Ian
-- 
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group

Re: [PATCH 1/5]: DCCP Fix use of invalid loss intervals

[Eddie Kohler]

But the two bit patterns are the same, and the comparison will promote 
them both to unsigned.

Try running the following program:

int main (int c, char **v)
{
     if (~0 != ~0U)
	printf("not equal as unknown");
     if ((int) ~0 != (int) ~0U)
	printf("not equal as ints");
     if ((unsigned) ~0 != (unsigned) ~0U)
	printf("not equal as unsigneds");
     if ((int) ~0 != (unsigned) ~0U)
	printf("not equal as int/unsigned");
     if ((unsigned) ~0 != (int) ~0U)
	printf("not equal as unsigned/int");
}

It prints nothing.

If one of the two numbers is 64-bit, your analysis works.  Maybe I'm 
missing something but I don't think so...
Eddie



Ian McDonald wrote:
> On 1/5/07, Eddie Kohler <kohler@cs.ucla.edu> wrote:
>> Ian (catching up slowly slowly), here is a nit as nitty as they come.
>>
>> This diff seems strange to me, since ~ actually does the same thing on
>> integers and unsigned integers.  (This code:
>>
>>      printf("%u %u\n", ~0, ~0U);
>>
>> will print the same thing twice.)
>>
>> Perhaps dccplih_interval is a 64-bit number?  In which case you want to
>> say something like ~0ULL?
>>
>> Eddie
> 
> Printing gives them the same result as you are using a %u mask. If you
> do it with a %d mask you will get a different result.
> 
> And that is the issue dccp_lih_interval is unsigned 32 bit and ~0 is a
> signed number and is large negative and they therefore can't be equal.
> 
> Ian

Re: [PATCH 1/5]: DCCP Fix use of invalid loss intervals

[Ian McDonald]

On 1/5/07, Eddie Kohler <kohler@cs.ucla.edu> wrote:
> But the two bit patterns are the same, and the comparison will promote
> them both to unsigned.
>
When I test from userspace I reach the same conclusion.

However without that patch in the kernel I get problems.

We have been having some weird signed/unsigned stuff going on - I'm
wondering if its something in the kernel build environment as it is
quite bizarre at times....

Ian
-- 
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group

Re: [PATCH 1/5]: DCCP Fix use of invalid loss intervals

[Gerrit Renker]

Hi Eddie,

thanks a lot for looking into this and I think we should take this on board and find 
a different solution to achieve the same purpose, but without comparison against ~0 or ~0U.

|  But the two bit patterns are the same, and the comparison will promote 
|  them both to unsigned.
Using both signed and unsigned can lead to very subtle bugs. As a recent example, we have had
real problems because time differences (signed type) was implicitly converted to unsigned.

The use of ~0U appears in the following functions:
	* to mark a loss interval as empty in dccp_li_hist_interval_new
	* to find non-empty loss intervals in dccp_li_hist_calc_i_mean
	* to mark an empty first loss interval in ccid3_hc_rx_calc_first_li
          (also connected with catching error conditions)

Sometimes the conversion happens through the return type, sometimes directly: it is not obvious
to see what happens here.

So I agree with Eddie and think we should find a different way of identifying empty loss intervals;
or check each part of the code to make sure it will be safe. 

I can also see the point Ian makes, in that this will require some work - hence added as ToDo item on 
http://linux-net.osdl.org/index.php/TODO#DCCP

Gerrit


|  Try running the following program:
|  
|  int main (int c, char **v)
|  {
|       if (~0 != ~0U)
|  	printf("not equal as unknown");
|       if ((int) ~0 != (int) ~0U)
|  	printf("not equal as ints");
|       if ((unsigned) ~0 != (unsigned) ~0U)
|  	printf("not equal as unsigneds");
|       if ((int) ~0 != (unsigned) ~0U)
|  	printf("not equal as int/unsigned");
|       if ((unsigned) ~0 != (int) ~0U)
|  	printf("not equal as unsigned/int");
|  }
|  
|  It prints nothing.
|  
|  If one of the two numbers is 64-bit, your analysis works.  Maybe I'm 
|  missing something but I don't think so...
|  Eddie
|  
|  
|  
|  Ian McDonald wrote:
|  > On 1/5/07, Eddie Kohler <kohler@cs.ucla.edu> wrote:
|  >> Ian (catching up slowly slowly), here is a nit as nitty as they come.
|  >>
|  >> This diff seems strange to me, since ~ actually does the same thing on
|  >> integers and unsigned integers.  (This code:
|  >>
|  >>      printf("%u %u\n", ~0, ~0U);
|  >>
|  >> will print the same thing twice.)
|  >>
|  >> Perhaps dccplih_interval is a 64-bit number?  In which case you want to
|  >> say something like ~0ULL?
|  >>
|  >> Eddie
|  > 
|  > Printing gives them the same result as you are using a %u mask. If you
|  > do it with a %d mask you will get a different result.
|  > 
|  > And that is the issue dccp_lih_interval is unsigned 32 bit and ~0 is a
|  > signed number and is large negative and they therefore can't be equal.
|  > 
|  > Ian
|  -
|  To unsubscribe from this list: send the line "unsubscribe dccp" in
|  the body of a message to majordomo@vger.kernel.org
|  More majordomo info at  http://vger.kernel.org/majordomo-info.html
|  
|