On 2/20/25 5:50 PM, Lucas Vargas Dias wrote:
> Em qui., 20 de fev. de 2025 às 13:13, Dumitru Ceara <[email protected]>
> escreveu:
>
>> On 2/20/25 3:41 PM, Lucas Vargas Dias wrote:
>>> Hi Dumitru,
>>>
>>>
>>>
>>>
>>> Em qui., 20 de fev. de 2025 às 11:19, Dumitru Ceara <[email protected]>
>>> escreveu:
>>>
>>>> On 2/7/25 2:43 PM, Lucas Vargas Dias via dev wrote:
>>>>> Fix the prefix filter function as the return condition when IPv6
>>>>> prefixes have same length. If denylist prefix and prefix verified
>>>>> have the same length, it must be have compared only.
>>>>> Without this fix if denylist filter has
>>>>> 2003:db08::/64 and it exists a route to 2003:db88, this route
>>>>> is blocked because the calc for IPv6 doesn't check if they have
>>>>> the same prefix lenght.
>>>>> AND operator between prefix and denylist prefix will be
>>>>> 2003:db08::/68 in this example (2003:db88::/64 & 2003:db08::64)
>>>>>
>>>>> Signed-off-by: Lucas Vargas Dias <[email protected]>
>>>>> ---
>>>>
>>>> Hi Lucas,
>>>>
>>>> Thanks for the patch!
>>>>
>>>>> ic/ovn-ic.c | 12 +++++++++++-
>>>>> tests/ovn-ic.at | 14 ++++++++++----
>>>>> 2 files changed, 21 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
>>>>> index 8320cbea5..7339811a4 100644
>>>>> --- a/ic/ovn-ic.c
>>>>> +++ b/ic/ovn-ic.c
>>>>> @@ -1073,12 +1073,22 @@ prefix_is_deny_listed(const struct smap
>>>> *nb_options,
>>>>> }
>>>>> } else {
>>>>> struct in6_addr mask = ipv6_create_mask(plen);
>>>>> + struct in6_addr m_bl_prefix = ipv6_addr_bitand(&bl_prefix,
>>>> &mask);
>>>>> +
>>>>> + if (plen == bl_plen) {
>>>>> + struct in6_addr prefix_v6 = ipv6_addr_bitand(prefix,
>>>> &mask);
>>>>> + if (!ipv6_addr_equals(&prefix_v6, &m_bl_prefix)) {
>>>>> + continue;
>>>>> + }
>>>>> + matched = true;
>>>>> + break;
>>>>> + }
>>>>
>>>> Why isn't this a problem for IPv4 too?
>>>>
>>>> Because for ipv4 it will be compared in if IN6_IS_ADDR_V4MAPPED and
>>> it compares (prefix and mask) with (bl_prefix and mask). It considers the
>>> mask
>>> before the comparison.
>>>
>>>
>>
>> OK, thanks for the reply. But then why don't we do the same thing for
>> IPv6?
>>
>> The following passes your test:
>>
>> struct in6_addr bl_mask = ipv6_create_mask(bl_plen);
>> struct in6_addr m_prefix = ipv6_addr_bitand(prefix, &bl_mask);
>> struct in6_addr m_bl_prefix = ipv6_addr_bitand(&bl_prefix, &bl_mask);
>>
>> if (!ipv6_addr_equals(&m_prefix, &m_bl_prefix)) {
>> continue;
>> }
>>
>> Would this be correct? Or am I missing something?
>>
>> Thanks,
>> Dumitru
>>
>> Actually, we can do the same thing. I think your suggestion is better than
> my changes.
Thanks for the confirmation. Would you be able to post a v2 then?
Thanks,
Dumitru
> Regards,
> Lucas
>
>>>> /* First calculate the difference between bl_prefix and
>>>> prefix, so
>>>>> * use the bl mask to ensure prefixes are correctly
>>>> validated.
>>>>> * e.g.: 2005:1734:5678::/50 is a subnet of 2005:1234::/21
>>>> */
>>>>> struct in6_addr m_prefixes = ipv6_addr_bitand(prefix,
>>>> &bl_prefix);
>>>>> struct in6_addr m_prefix = ipv6_addr_bitand(&m_prefixes,
>>>> &mask);
>>>>> - struct in6_addr m_bl_prefix = ipv6_addr_bitand(&bl_prefix,
>>>> &mask);
>>>>> +
>>>>> if (!ipv6_addr_equals(&m_prefix, &m_bl_prefix)) {
>>>>> continue;
>>>>> }
>>>>> diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
>>>>> index 9fc386131..0ce08260c 100644
>>>>> --- a/tests/ovn-ic.at
>>>>> +++ b/tests/ovn-ic.at
>>>>> @@ -1352,7 +1352,7 @@ for i in 1 2; do
>>>>> check ovn-nbctl set nb_global . options:ic-route-adv=true
>>>>> # Enable denylist single filter for IPv6
>>>>> check ovn-nbctl set nb_global . options:ic-route-denylist=" \
>>>>> - 2003:db8:1::/64,2004:aaaa::/32,2005:1234::/21"
>>>>> + 2003:db08:1::/64,2004:aaaa::/32,2005:1234::/21"
>>>>>
>>>>> check ovn-ic-nbctl --wait=sb sync
>>>>> # Create LRP and connect to TS
>>>>> @@ -1369,7 +1369,10 @@ for i in 1 2; do
>>>>>
>>>>> # Create denylisted LRPs and connect to TS
>>>>> check ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext$i \
>>>>> - 11:11:11:11:11:1$i 2003:db8:1::$i/64
>>>>> + 11:11:11:11:11:1$i 2003:db88:1::$i/64
>>>>> +
>>>>> + check ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext1$i \
>>>>> + 11:11:11:11:12:1$i 2003:db08:1::$i/64
>>>>>
>>>>> check ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext2$i \
>>>>> 22:22:22:22:22:2$i 2004:aaaa:bbb::$i/48
>>>>> @@ -1388,6 +1391,7 @@ check ovn-ic-nbctl --wait=sb sync
>>>>> AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr1 |
>>>>> awk '/learned/{print $1, $2}' ], [0], [dnl
>>>>> 2002:db8:1::/64 2001:db8:1::2
>>>>> +2003:db88:1::/64 2001:db8:1::2
>>>>> 2005:1834:5678::/50 2001:db8:1::2
>>>>> ])
>>>>>
>>>>> @@ -1403,7 +1407,8 @@ check ovn-ic-nbctl --wait=sb sync
>>>>> AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr1 |
>>>>> awk '/learned/{print $1, $2}' | sort ], [0], [dnl
>>>>> 2002:db8:1::/64 2001:db8:1::2
>>>>> -2003:db8:1::/64 2001:db8:1::2
>>>>> +2003:db08:1::/64 2001:db8:1::2
>>>>> +2003:db88:1::/64 2001:db8:1::2
>>>>> 2004:aaaa:bbb::/48 2001:db8:1::2
>>>>> 2005:1734:5678::/50 2001:db8:1::2
>>>>> 2005:1834:5678::/50 2001:db8:1::2
>>>>> @@ -1413,7 +1418,7 @@ for i in 1 2; do
>>>>> ovn_as az$i
>>>>>
>>>>> check ovn-nbctl set nb_global . \
>>>>> -
>> options:ic-route-denylist="2003:db8:1::/64,2004:db8:1::/64"
>>>>> +
>> options:ic-route-denylist="2003:db88:1::/64,2004:db8:1::/64"
>>>>>
>>>>> # Create an 'extra' denylisted LRP and connect to TS
>>>>> check ovn-nbctl lrp-add lr$i lrp-lr$i-p-ext5$i \
>>>>> @@ -1424,6 +1429,7 @@ check ovn-ic-nbctl --wait=sb sync
>>>>> AT_CHECK([ovn_as az1 ovn-nbctl lr-route-list lr1 |
>>>>> awk '/learned/{print $1, $2}' | sort ], [0], [dnl
>>>>> 2002:db8:1::/64 2001:db8:1::2
>>>>> +2003:db08:1::/64 2001:db8:1::2
>>>>> 2004:aaaa:bbb::/48 2001:db8:1::2
>>>>> 2005:1734:5678::/50 2001:db8:1::2
>>>>> 2005:1834:5678::/50 2001:db8:1::2
>>>>
>>>> Regards,
>>>> Dumitru
>>>>
>>>>
>>>> Regards,
>>> Lucas
>>>
>>
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
