Attention is currently required from: flichtenheld, plaisthos. MaxF has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/30?usp=email )
Change subject: Implement initial packet reflection protection using bloom filter ...................................................................... Patch Set 9: Code-Review-1 (10 comments) Patchset: PS9: I havne't reviewed the whole thing yet, but I found some stuff to complain about File src/openvpn/Makefile.am: http://gerrit.openvpn.net/c/openvpn/+/30/comment/5feab99c_f33c2318 : PS8, Line 46: b tabs vs spaces File src/openvpn/bloom.h: http://gerrit.openvpn.net/c/openvpn/+/30/comment/88db0086_921e0752 : PS8, Line 8: 2022 Shouldn't this be 2024? http://gerrit.openvpn.net/c/openvpn/+/30/comment/c0e46973_ad5f7ee1 : PS8, Line 32: BLOOM_FILTER_BIT_COUNT BITS File src/openvpn/bloom.c: http://gerrit.openvpn.net/c/openvpn/+/30/comment/ca1b18dc_b9ffbcbe : PS8, Line 101: /** : * Calculates the number of bytes we need for storing a bloom filter of size : * size. We add + 1 to avoid rounding problems and too small allocation */ : static inline : size_t : bloom_get_filter_byte_count(size_t size) : { : static_assert(sizeof(bloom_counter_t) * 8 % BLOOM_FILTER_BITS_COUNT == 0, : "bloom_counter_t must be a multiple of BLOOM_FILTER_BIT_COUNT"); : : return size * sizeof(bloom_counter_t)/BLOOM_FILTER_BITS_COUNT + 1; : } I'm really confused by what size means here. This is *not* the size field in the bloom filter struct, right? It's the total number of bits in the bloom filter, right? http://gerrit.openvpn.net/c/openvpn/+/30/comment/88af9dc3_bf01868e : PS8, Line 115: static inline : size_t : bloom_get_filter_bit_offset(size_t bucket) : { : return (bucket * BLOOM_FILTER_BITS_COUNT) % sizeof(bloom_counter_t); : } If this is a bit offset, don't we need 8 * sizeof(bloom_counter_t) here? http://gerrit.openvpn.net/c/openvpn/+/30/comment/f0ce5e7f_d277c415 : PS8, Line 122: static inline : size_t : bloom_get_filter_array_index(size_t bucket) : { : return (bucket * BLOOM_FILTER_BITS_COUNT) / sizeof(bloom_counter_t); : } This seems wrong to me. Every bucket is 2 bits wide. The bloom_counter_t is 32 bits wide, or 4 bytes, so sizeof(bloom_counter_t) == 4. Bucket 3 is at a 6 bit offset, so it's in the first bloom_counter_t (array index 0). But this function gives 3 * 2 / 4 == 1. http://gerrit.openvpn.net/c/openvpn/+/30/comment/688a1a4a_2c63f6ac : PS8, Line 172: whitespace File src/openvpn/reflect_filter.c: http://gerrit.openvpn.net/c/openvpn/+/30/comment/e570d1bc_a93710c4 : PS8, Line 43: static bool : reflect_filter_rate_l Remove empty line http://gerrit.openvpn.net/c/openvpn/+/30/comment/71c672e1_c93872de : PS8, Line 116: /* we keep the count in the key instead of in the bloom filter table as : * can then keep the counter in the bloom filter itself small (2 bits) ...as we can then... -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/30?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I0a9274cab7fefce3b13c05052fb9a072e0bfa6b9 Gerrit-Change-Number: 30 Gerrit-PatchSet: 9 Gerrit-Owner: plaisthos <arne-open...@rfc2549.org> Gerrit-Reviewer: MaxF <m...@max-fillinger.net> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Attention: flichtenheld <fr...@lichtenheld.com> Gerrit-Comment-Date: Sat, 21 Sep 2024 14:26:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
