| 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
| ||
|
Date: Mon, 14 Sep 2009 21:40:40 -0300
From: Ivo Calado <ivocalado@...edded.ufcg.edu.br>
To: 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
The comments follow below
On Sun, Sep 13, 2009 at 15:41, Gerrit Renker <gerrit@....abdn.ac.uk> wrote:
> | Adds options DROPPED PACKETS and LOSS INTERVALS to receiver. In this patch is added the
> | mechanism of gathering information about loss intervals and storing it, for later
> | construction of these two options.
> This also needs some more work, please see inline.
>
<snip>
>
> + tfrc_sp_update_li_data(ld, h, skb, new_loss, new_event);
> +
> I don't understand why 'tfrc_sp_update_li_data' is called twice, one call seems
> to be redundant. What it seems to be wanting to do is
When we are designing the loss count algorithm it seemed to be
necessary, but now that i need to revise this (patch nº 2), I'll
observe this.
> bool new_loss = false;
>
> //...
<snip>
> at the begin of the function, all subsequent 'dccp_data_packet(skb)' are unnecessary.
> Almost every 'if' statement ends in 'return', this seems ad-hoc and could be reduced
> by adding if-else-if-else-if..., which would probably also reduce the code duplication.
>
I'll try to reduce code duplication, thanks.
>
> @@ -244,8 +463,11 @@
> tfrc_lh_slab = kmem_cache_create("tfrc_sp_li_hist",
> sizeof(struct tfrc_loss_interval), 0,
> SLAB_HWCACHE_ALIGN, NULL);
> + tfrc_ld_slab = kmem_cache_create("tfrc_sp_li_data",
> + sizeof(struct tfrc_loss_data_entry), 0,
> + SLAB_HWCACHE_ALIGN, NULL);
>
> - if ((tfrc_lh_slab != NULL))
> + if ((tfrc_lh_slab != NULL) || (tfrc_ld_slab != NULL))
> return 0;
>
> if (tfrc_lh_slab != NULL) {
> @@ -253,6 +475,11 @@
> tfrc_lh_slab = NULL;
> }
>
> + if (tfrc_ld_slab != NULL) {
> + kmem_cache_destroy(tfrc_ld_slab);
> + tfrc_ld_slab = NULL;
> + }
> +
> return -ENOBUFS;
> }
> 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.
>
>
>
> --- dccp_tree_work5.orig/net/dccp/ccids/lib/loss_interval_sp.h
> +++ dccp_tree_work5/net/dccp/ccids/lib/loss_interval_sp.h
> @@ -70,13 +70,52 @@
> struct tfrc_rx_hist;
> #endif
>
> +struct tfrc_loss_data_entry {
> + struct tfrc_loss_data_entry *next;
> + u32 lossless_length:24;
> + u8 ecn_nonce_sum:1;
> + u32 loss_length:24;
> + u32 data_length:24;
> + u32 drop_count:24;
> +};
> According to RFC 4342, 8.6.1, Loss Length is a 23-bit number, i.e.
> u32 loss_length:23;
>
Thanks!
>
>
>
> +#define TFRC_LOSS_INTERVALS_OPT_MAX_LENGTH 28
> +#define TFRC_DROP_OPT_MAX_LENGTH 84
> +#define TFRC_LI_OPT_SZ \
> + (2 + TFRC_LOSS_INTERVALS_OPT_MAX_LENGTH*9)
> +#define TFRC_DROPPED_OPT_SZ \
> + (1 + TFRC_DROP_OPT_MAX_LENGTH*3)
> It would be good to have a reminder where the numbers come from, i.e.
> * the "28" comes from RFC 4342, 8.6
> * the "84" comes from RFC 5622, 8.7
> * the "9" again is from RFC 4342, 8.6
> * the 1 + TFRC_DROP_OPT_MAX_LENGTH*3 = DCCP_SINGLE_OPT_MAXLEN (linux/dccp.h)
>
Sorry, this documentation was written, but i left it in another future patch.
>
> +struct tfrc_loss_data {
> + struct tfrc_loss_data_entry *head;
> + u16 counter;
> + u8 loss_intervals_opts[TFRC_LI_OPT_SZ];
> + u8 drop_opts[TFRC_DROPPED_OPT_SZ];
> + u8 last_loss_count;
> + u8 sto_ecn;
> + u8 sto_is_data;
> +};
>
>
>
> +static inline void tfrc_ld_init(struct tfrc_loss_data *ld)
> +{
> + memset(ld, 0, sizeof(struct tfrc_loss_data));
> +}
> A tip from CodingStyle - using "sizeof(*ld)" will continue to work if there
> are changes in the interface.
>
Thanks.
>
> --- dccp_tree_work5.orig/net/dccp/dccp.h
> +++ dccp_tree_work5/net/dccp/dccp.h
> @@ -403,6 +403,16 @@
> return (DCCP_SKB_CB(skb)->dccpd_ecn & INET_ECN_MASK) == INET_ECN_CE;
> }
>
> +static inline bool dccp_skb_is_ecn_ect0(const struct sk_buff *skb)
> +{
> + return (DCCP_SKB_CB(skb)->dccpd_ecn & INET_ECN_MASK) == INET_ECN_ECT_0;
> +}
> +
> +static inline bool dccp_skb_is_ecn_ect1(const struct sk_buff *skb)
> +{
> + return (DCCP_SKB_CB(skb)->dccpd_ecn & INET_ECN_MASK) == INET_ECN_ECT_0;
> +}
> The routines are not needed, because the dccpd_ecn field is (deliberately) only
> 2 bits wide. In the second case it should have been INET_ECN_ECT_1.
>
And how would be to determine if one packet's ecn is set to ECT 0 or ECT 1?
--
Ivo Augusto Andrade Rocha Calado
MSc. Candidate
Embedded Systems and Pervasive Computing Lab - http://embedded.ufcg.edu.br
Systems and Computing Department - http://www.dsc.ufcg.edu.br
Electrical Engineering and Informatics Center - http://www.ceei.ufcg.edu.br
Federal University of Campina Grande - http://www.ufcg.edu.br
PGP: 0x03422935
Quidquid latine dictum sit, altum viditur.
--
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
Powered by blists - more mailing lists