<meta http-equiv="Content-Type" content="text/html; charset=utf-8"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:large"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">ср, 9 мар. 2022 г. в 19:40, Daniel P. Berrangé <<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Mar 02, 2022 at 11:34:56AM +0300, Nikolay Shirokovskiy wrote:<br>
> In order to delete or rename tree of chains in netfilter we use generic<br>
> code that inspect current state of netfilter and traverse the tree<br>
> accordingly. I found the way traverse is done a bit hard to think about.<br>
> <br>
> As an example I'll take ebtable rules generated by<br>
> ebiptablesTearOldRules for input chains only. Common arguments like '-t<br>
> nat' and '--concurrent' are also filtered. To illustrate the point<br>
> libvirt-I-vnet1 has one subchain I-vnet1-mac.<br>
> <br>
> Let's look at ebtablesRemoveSubChainsFW[1] work. It make a recursive<br>
> deletion of chains. I marked below the ebtables calls that it generates<br>
> in the whole list of commands that generates ebiptablesTearOldRules. One<br>
> can see that the rules perplex with the ebtablesRenameTmpSubAndRootChainsFW[2]<br>
> rules and this looks a bit hard to manage. I'd prefer that the order of<br>
> ebtables commands will be aligned with code.<br>
> <br>
> ebtables -D PREROUTING -i vnet1 -j libvirt-I-vnet1<br>
> ebtables -L libvirt-I-vnet1 # listing to detect subchains[1]<br>
> ebtables -F libvirt-I-vnet1<br>
> ebtables -X libvirt-I-vnet1<br>
> ebtables -L libvirt-J-vnet1 # [2]<br>
> ebtables -E libvirt-J-vnet1 libvirt-I-vnet1 # [2]<br>
> ebtables -L I-vnet1-mac # listing to detect subchains[1]<br>
> ebtables -F I-vnet1-mac # flushing to unlink subchains[1]<br>
> ebtables -X I-vnet1-mac # deleting chain itself[1]<br>
> ebtables -L J-vnet1-mac # [2]<br>
> ebtables -F I-vnet1-mac # [2]<br>
> ebtables -X I-vnet1-mac # [2]<br>
> ebtables -E J-vnet1-mac I-vnet1-mac # [2]<br>
> <br>
> Notice also that we need to flush libvirt-I-vnet1 to unlink subchains in<br>
> order to be able to delete them. And again code look like reversed:<br>
> <br>
> ebtablesRemoveSubChainsFW(fw, ifname);<br>
> ebtablesRemoveRootChainFW(fw, true, ifname);<br>
> ebtablesRemoveRootChainFW(fw, false, ifname);<br>
> <br>
> This is because ebtablesRemoveSubChainsFW only adds listing call and<br>
> makes actual subchains cleanup in the the process of virFirewallApply.<br>
> <br>
> So to make it more manageble/straightforward I propose here to list and<br>
> add resulted commands inplace. Note that in the process we need:<br>
> <br>
> 1. Move ebtablesRemoveTmpRootChainFW rules to the _ebtablesRemoveSubChainsFW<br>
> to keep correct order - first flush parent and then delete subchains.<br>
> <br>
> 2. Avoid using virFirewallStartRollback as otherwise there will be<br>
> unnesseary listing call done in ebtablesRemoveTmpSubChainsFW.<br>
> <br>
> The fact we can't use virFirewallStartRollback in this approach can be<br>
> seen as a donwside. However it is not a real rollback - you need to code<br>
> all the steps to rollback on you own so it can be easily replaced by<br>
> same virFirewallApply. Not only in this place but in all the other places.<br>
> <br>
> Now compare the commands order in ebiptablesTearOldRules after patch:<br>
> <br>
> ebtables -L libvirt-I-vnet1 # listing to detect subchains [1]<br>
> ebtables -L I-vnet1-mac # listing to detect subchains [1]<br>
> ebtables -D PREROUTING -i vnet1 -j libvirt-I-vnet1<br>
> ebtables -F libvirt-I-vnet1 # flushing and deleting of chain [1]<br>
> ebtables -X libvirt-I-vnet1<br>
> ebtables -F I-vnet1-mac # flushing and deleting of subchain [1]<br>
> ebtables -X I-vnet1-mac<br>
> ebtables -L libvirt-J-vnet1<br>
> ebtables -E libvirt-J-vnet1 libvirt-I-vnet1<br>
> ebtables -L J-vnet1-mac<br>
> ebtables -F I-vnet1-mac<br>
> ebtables -X I-vnet1-mac<br>
> ebtables -E J-vnet1-mac I-vnet1-mac<br>
> <br>
> Notice that listing commands from ebtablesRemoveSubChainsFW pop up to<br>
> the top of the list. This may seen again as perplexing but it is of<br>
> a different kind. It only moves nonmodifying listing commands to the top<br>
> which does not perplexing understanding command flow much. Also if the<br>
> calls will be optimized we have only one listing command on the<br>
> top.<br>
> ---<br>
> src/nwfilter/nwfilter_ebiptables_driver.c | 154 +++++++++++-----------<br>
> 1 file changed, 75 insertions(+), 79 deletions(-)<br>
> <br>
> diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c<br>
> index 54065a0f75..6952ebc059 100644<br>
> --- a/src/nwfilter/nwfilter_ebiptables_driver.c<br>
> +++ b/src/nwfilter/nwfilter_ebiptables_driver.c<br>
> @@ -148,6 +148,42 @@ static char chainprefixes_host_temp[3] = {<br>
> 0<br>
> };<br>
> <br>
> +<br>
> +static int<br>
> +ebiptablesQueryCallback(virFirewall *fw G_GNUC_UNUSED,<br>
> + virFirewallLayer layer G_GNUC_UNUSED,<br>
> + const char *const *lines,<br>
> + void *opaque)<br>
> +{<br>
> + GStrv *capture = opaque;<br>
> +<br>
> + *capture = g_strdupv((gchar **)lines);<br>
> + return 0;<br>
> +}<br>
> +<br>
> +<br>
> +static GStrv<br>
> +ebiptablesQuery(virFirewallLayer layer,<br>
> + ...)<br>
> +{<br>
> + g_autoptr(virFirewall) fw = virFirewallNew();<br>
> + g_auto(GStrv) capture = NULL;<br>
> + va_list args;<br>
> +<br>
> + virFirewallStartTransaction(fw, 0);<br>
> +<br>
> + va_start(args, layer);<br>
> + virFirewallAddRuleFullV(fw, layer, true,<br>
> + ebiptablesQueryCallback, &capture, args);<br>
> + va_end(args);<br>
> +<br>
> + if (virFirewallApply(fw) < 0)<br>
> + return NULL;<br>
> +<br>
> + return g_steal_pointer(&capture);<br>
> +}<br>
<br>
This method illustrates why we didn't take this approach in the<br>
first place. The intention of the current design is to have<br>
clean separation between deciding what iptables commands to<br>
invoke, and when we actually invoke them. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
This breaks that separation, because now in the middle of<br>
populating the virFirewall rules we want to run, we have<br>
to actually invoke the firewall tools multiple times to<br>
query current state. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> @@ -2649,10 +2644,9 @@ static int<br>
> ebtablesRemoveSubChainsQuery(virFirewall *fw,<br>
> virFirewallLayer layer,<br>
> const char *const *lines,<br>
> - void *opaque)<br>
> + const char *chainprefixes)<br>
> {<br>
> size_t i, j;<br>
> - const char *chainprefixes = opaque;<br>
> <br>
> for (i = 0; lines[i] != NULL; i++) {<br>
> char *tmp = strstr(lines[i], "-j ");<br>
> @@ -2665,17 +2659,21 @@ ebtablesRemoveSubChainsQuery(virFirewall *fw,<br>
> for (j = 0; chainprefixes[j]; j++) {<br>
> if (tmp[0] == chainprefixes[j] &&<br>
> tmp[1] == '-') {<br>
> + g_auto(GStrv) capture = NULL;<br>
> +<br>
> VIR_DEBUG("Processing chain '%s'", tmp);<br>
> - virFirewallAddRuleFull(fw, layer,<br>
> - false, ebtablesRemoveSubChainsQuery,<br>
> - (void *)chainprefixes,<br>
> - "-t", "nat", "-L", tmp, NULL);<br>
> + capture = ebiptablesQuery(layer,<br>
> + "-t", "nat", "-L", tmp, NULL);<br>
> virFirewallAddRuleFull(fw, layer,<br>
> true, NULL, NULL,<br>
> "-t", "nat", "-F", tmp, NULL);<br>
> virFirewallAddRuleFull(fw, layer,<br>
> true, NULL, NULL,<br>
> "-t", "nat", "-X", tmp, NULL);<br>
> + if (capture)<br>
> + ebtablesRemoveSubChainsQuery(fw, layer,<br>
> + (const char *const *)capture,<br>
> + chainprefixes);<br>
> }<br>
<br>
....here we're still populating the 'fw' object, but actually<br>
also running firewall commands at the same time inside the<br>
ebtablesRemoveSubChainsQuery method. <br>
<br>
The current implementation on the backend of virFirewallApply<br>
is not technically transactional, but the intent behind the<br>
design is that we would be able to serialize execution of the<br>
virFirewallApply method. This wouldn't be possible with the<br>
way this patch changes things, as now we're dealing with<br>
multiple virFirewall object instances.<br></blockquote><div><br></div><div class="gmail_default" style="font-size:large">Well I'd say if we'd have a transaction like with a nfttables we anyway first</div><div class="gmail_default" style="font-size:large">have to inspect tables then make a patch and then apply it. You have to inspect tables</div><div class="gmail_default" style="font-size:large">anyway outside the transaction. Which is basically what this patch does.</div><div class="gmail_default" style="font-size:large"><br></div><div class="gmail_default" style="font-size:large">Anyway I think it is for good to address the issue with perplexing rules from different</div><div class="gmail_default" style="font-size:large">larger logical operations which is illustrated above (removing old chains and renaming tmp chains</div><div class="gmail_default" style="font-size:large">to non-tmp ones.</div><div class="gmail_default" style="font-size:large"><br></div><div class="gmail_default" style="font-size:large">As an alternative I considered that rules that are created inside the transaction could be added </div><div class="gmail_default" style="font-size:large">to some marked place in the rules list instead of always at the end of the list. But IMHO this</div><div class="gmail_default" style="font-size:large">is more complicated. I'd prefer a simple approach which is to inspect tables/create patch/apply patch.</div><div class="gmail_default" style="font-size:large"><br></div><div class="gmail_default" style="font-size:large">Nikolay</div></div></div>