Jon,
on api trace: does the below work ? (even though the current scenario
is trivially reproducible, the api traces are very useful for tougher
cases, and save a lot of typing while storytelling).
DBGvpp# api trace on
..... do the things ....
DBGvpp# api trace save macip-trace
API trace saved to /tmp/macip-trace
DBGvpp# api trace custom-dump /tmp/macip-trace
SCRIPT: macip_acl_add_replace -1 count 1 count 1 \
ipv4 permit \
src mac 00:00:00:00:00:00 mask 00:00:00:00:00:00 \
src ip 0.0.0.0/0, \
SCRIPT: macip_acl_interface_add_del sw_if_index 0 acl_index 0 add
SCRIPT: macip_acl_add_replace 0 count 1 count 1 \
ipv4 permit \
src mac 00:00:00:00:00:00 mask 00:00:00:00:00:00 \
src ip 0.0.0.0/0, \
Now, to the issue itself: it's exactly as I described, but with a twist:
vnet_set_input_acl_intfc(), which is used under the hood to assign the
inacl on the interfaces, is quite picky - if there is an existing
inacl applied, it just quietly does nothing. (@DaveB - this kinda
feels strange, I am not sure what the logic is behind doing that.)
Anyway, rather than debating on why it behaves this way, and,
especially since we actually are deleting the tables in question, it's
better to unapply the inacls first, and then reapply them after the
tables have been recreated.
The result is in https://gerrit.fd.io/r/#/c/9772/ - you can verify
that it addresses the issue.
Now, going on to "how exactly did this slip through" - seems the macip
tests are quite a bit too lenient than they should be. I'll need to
address that as well, though probably I will split the dot1q/dot1ad
test cases out, and in the process refactor things a bit... so in the
interests of your time, maybe 9772 can go with just an actual code
fix.
I've dumped the entire debugging process into a log, which turned out
be fairly long, so to avoid sending the walls of text to the list,
I've dumped it elsewhere:
http://stdio.be/blog/2017-12-09-Debugging-VPP-MACIP-ACLs/
Thanks again for catching this.
--a
On 12/8/17, Jon Loeliger <j...@netgate.com> wrote:
> On Fri, Dec 8, 2017 at 12:55 PM, Jon Loeliger <j...@netgate.com> wrote:
>>
>>
>>
>>> We need to iterate the am->macip_acl_by_sw_if_index and for interfaces
>>> which have the acl in question applied, reapply these new tables to
>>> these
>>> interfaces so the already active macip acl remained active.
>>>
>>
>> Ah, I see....
>>
>
> I'm not convinced this is the issue. I added that loop, and it doesn't
> change things. Specifically, a->ip4_table_index is 0, not 3 after the
> creation of the classify tables.
>
> So, here is sort of a trace through some key places of the
> classify table construction. It follows the API sequence I
> outlined in an earlier message (create MACIP with one permit rule,
> then bind to interface, then modify that MACIP's rule to deny
> and update it using macip_acl_add_replace).
>
> I'll annotate this some.
>
> Here is the initial creation of the classify tables:
>
> macip_acl_add_list: looks like a create
> macip_create_classify_tables: top a->count 1
> macip_create_classify_tables: last_table before 4294967295
>
> Now we create the rules in reverse order:
>
> macip_create_classify_tables: ARP looop top
> macip_create_classify_tables: ARP loop last_table 0
> macip_create_classify_tables: IP[46] loop top
> macip_create_classify_tables: IP[46] loop last_table 1
> macip_create_classify_tables: IP[46] loop last_table 2
> macip_create_classify_tables: IP[46] loop last_table 3
> macip_create_classify_tables: ip4_t_i 3
> macip_create_classify_tables: ip6_t_i 3
> macip_create_classify_tables: l2_t_i 3
>
> That leaves last_table at 3, the most recently created rule,
> with a chain from 0 through 3:
>
> vpp# show classify tables
> TableIdx Sessions NextTbl NextNode
> 0 1 1 -1
> Heap: 3 objects, 204 of 2k used, 76 free, 0 reclaimed, 1k overhead, 8188k
> capacity
> nbuckets 32, skip 0 match 2 flag 0 offset 0
> mask 000000000000000000000000ffff000000000000000000000000000000000000
> linear-search buckets 0
> 1 1 2 -1
> Heap: 3 objects, 204 of 2k used, 76 free, 0 reclaimed, 1k overhead, 8188k
> capacity
> nbuckets 32, skip 0 match 2 flag 0 offset 0
> mask 000000000000000000000000ffff0000ffff0000000000000000000000000000
> linear-search buckets 0
> 2 1 3 -1
> Heap: 3 objects, 236 of 2k used, 76 free, 0 reclaimed, 1k overhead, 8188k
> capacity
> nbuckets 32, skip 0 match 3 flag 0 offset 0
> mask 00000000:
> 000000000000000000000000ffff0000ffff0000ffff00000000000000000000
> 00000020: 00000000000000000000000000000000
> linear-search buckets 0
> 3 1 -1 0
> Heap: 3 objects, 204 of 2k used, 76 free, 0 reclaimed, 1k overhead, 8188k
> capacity
> nbuckets 32, skip 0 match 2 flag 0 offset 0
> mask 000000000000000000000000ffff000000000000000000000000000000000000
>
> Now I bind that MACIP to the interface, and get:
>
> vpp# show acl-plugin macip interface
> sw_if_index 0: -1
> sw_if_index 1: 0
> vpp# show acl-plugin macip acl
> MACIP acl_index: 0, count: 1 (true len 1) tag {bob} is free pool slot: 0
> ip4_table_index 3, ip6_table_index 3, l2_table_index 3
> rule 0: ipv4 action 1 ip 0.0.0.0/0 mac 00:00:00:00:00:00 mask
> 00:00:00:00:00:00
>
> Now I update the MACIP rule, changing "permit" to "deny".
> And I call macip_acl_add_replace():
>
> macip_acl_add_list: replacing ACLs now, acl_list_index is 0
> macip_acl_interface_add_acl: macip_acl_index 0
> macip_acl_add_list: looks like a replace
> macip_create_classify_tables: top a->count 1
> macip_create_classify_tables: last_table before 4294967295
>
> That's just like before. Good.
>
> And now we count down the existing matching rules:
>
> macip_create_classify_tables: ARP looop top
> macip_create_classify_tables: ARP loop last_table 3
> macip_create_classify_tables: IP[46] loop top
> macip_create_classify_tables: IP[46] loop last_table 2
> macip_create_classify_tables: IP[46] loop last_table 1
> macip_create_classify_tables: IP[46] loop last_table 0
>
> And leave last_table at 0.
> Which we then store in the {ip4,ip6,l2}_table_index variables:
>
> macip_create_classify_tables: ip4_t_i 0
> macip_create_classify_tables: ip6_t_i 0
> macip_create_classify_tables: l2_t_i 0
>
> This is my loop added to the bottom of macip_acl_add_list():
>
> for (i = 0; i < vec_len (am->macip_acl_by_sw_if_index); i++)
> {
> vlib_cli_output (vm, "%s: check swif %d\n", __func__, i);
>
> if (am->macip_acl_by_sw_if_index[i] == *acl_list_index)
> {
> vlib_cli_output (vm, "%s: re-add acl_list_index %d\n", __func__,
> *acl\
> _list_index);
> macip_acl_interface_add_acl (am, i, *acl_list_index);
> }
> }
>
>
> macip_acl_add_list: replacing ACLs now, acl_list_index is 0
> macip_acl_add_list: check swif 0
> macip_acl_add_list: check swif 1
> macip_acl_add_list: re-add acl_list_index 0
> macip_acl_interface_add_acl: macip_acl_index 0
>
> So the MACIP ACL is applied to the SW IF, I believe, but
> the MACIP ACL itself appears to be pointing to the wrong
> end of the classify table chain in either the "create" case,
> or in the "replace" case. But I think they should be the same.
>
> HTH,
> jdl
>
_______________________________________________
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev
