netdev - Re: [PATCH 4/5] Adds options DROPPED PACKETS and LOSS INTERVALS to receiver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 19 Sep 2009 14:16:08 +0100 (BST)
From:	gerrit@....abdn.ac.uk
To:	"Ivo Calado" <ivocalado@...edded.ufcg.edu.br>
Cc:	"Gerrit Renker" <gerrit@....abdn.ac.uk>, dccp@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH 4/5] Adds options DROPPED PACKETS and LOSS INTERVALS to 
      receiver

>> | Adds options DROPPED PACKETS and LOSS INTERVALS to receiver.
I must admit that I did not look at this deeply enough to be able to
say whether it would work or not. The comments that were sent were after
the first reading.

Whether to add the Loss Intervals / Dropped Packet options is related to
the question in patch 2/5. This needs to be clarified first: you do add
the Loss Intervals option, but if you do it, the division of the loss
intervals is not necessary - unless I am missing something here, this
computation is done by the sender.

If I understand RFC 4342/4828/5622 correctly, the sender would need to
keep track of the RTTs for each sent loss interval. Since the loss
interval boundaries are set by the receiver, the sender would need to
store the window counter value (or the RTT). RFC 4828 is a bit misleading
since it quotes RFC 3448/5348 (where the receiver computes the loss
event rate), whereas CCID-4 is based on RFC 4342 (where the sender
normally computes the loss event rate).

>> The condition above should be '&&', not '||'. Suggested alternative:
>>
>> +       if (tfrc_lh_slab == NULL)
>> +               goto lh_failed;
>> +
>> +       tfrc_ld_slab = kmem_cache_create("tfrc_sp_li_data",
>> +                                        sizeof(struct
>> tfrc_loss_data_entry), 0,
>> +                                        SLAB_HWCACHE_ALIGN, NULL);
>> +       if (tfrc_ld_slab != NULL)
>> +               return 0;
>> +
>> +       kmem_cache_destroy(tfrc_lh_slab);
>> +       tfrc_lh_slab = NULL;
>> +lh_failed:
>> +       return -ENOBUFS;
>>  }
>>
>
> Thanks for revising this. Adding one label for each failure case will
> not scale well. In another patch it will be needed to create another
> structure, and so, requiring another label.
Using such labels follows a coding convention in the networking code.
As an example, consider ip4_init_mib_net() in net/ipv4/af_inet.c.

The pattern is that if step n fails, it does a rollback, undoing all
preceding initialisations in the reverse order. I think this is also
in agreement with Documentation/CodingStyle, chap. 7.

> And how would be to determine if one packet's ecn is set to ECT 0 or ECT
> 1?
It should be possible to use '==' directly, i.e.
switch (DCCP_SKB_CB(skb)->dccpd_ecn) {
case  INET_ECN_NOT_ECT:  // ECN not enabled
case  INET_ECN_ECT_1:    // ECT(1), see below
case  INET_ECN_ECT_0:    // ECT(0)
case  INET_ECN_CE:       // congestion
}

However, the kernel currently only supports ECT(0). Resolving this is
ongoing work in another thread. For the moment, it simplifies the ECN
nonce verification; as per figure 1 in RFC 3540, the sum will always
be 0 if only ECT(0) is used.

This would allow to write a function stub for ECN nonce verification,
which for the moment only does something like

bool dccp_verify_ecn_nonce(const u8 sum)
{
      return sum == 0;
}

The same "fix" has currently been put into the Ack Vector nonce sum,
this is in
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;\
a=commitdiff;h=50e6081f6ff37102ac5f92df85f017e2c15f338a

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html