[Cake] host hashes and NAT neglected in src/dst-host mode

[Cake] host hashes and NAT neglected in src/dst-host mode

[gamanakis]

I was having a look at cake's hash code and it seems srchost_hash and
dsthost_hash, as well as the flowkeys in NAT are not calculated in plain
"srchost" or "dsthost" modes. CAKE_FLOW_FLOWS and CAKE_FLOW_HOSTS are both 0
(conditional in line 633) when src/dst-host is set as flow mode. So the code
goes to "skip_hash" (line 683), and as a result the flowkeys are not updated
in respect to NAT (line 640), but also srchost_hash and dsthost_hash (line
653) are not calculated. 

Is this intentional? I would expect the NAT flowkeys to be updated in
src/dst-host modes, and also the host hashes to be calculated for fair host
isolation.

cake_hash() in sch_cake.c from linux-4.20:

 632         /* If both overrides are set we can skip packet dissection
entirely */
 633         if ((flow_override || !(flow_mode & CAKE_FLOW_FLOWS)) &&
 634             (host_override || !(flow_mode & CAKE_FLOW_HOSTS)))
 635                 goto skip_hash;
 636
 637         skb_flow_dissect_flow_keys(skb, &keys,
 638
FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
 639
 640         if (flow_mode & CAKE_FLOW_NAT_FLAG)
 641                 cake_update_flowkeys(&keys, skb);
 642
 643         /* flow_hash_from_keys() sorts the addresses by value, so we
have
 644          * to preserve their order in a separate data structure to
treat
 645          * src and dst host addresses as independently selectable.
 646          */
 647         host_keys = keys;
 648         host_keys.ports.ports     = 0;
 649         host_keys.basic.ip_proto  = 0;
 650         host_keys.keyid.keyid     = 0;
 651         host_keys.tags.flow_label = 0;
 652
 653         switch (host_keys.control.addr_type) {
 654         case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
 655                 host_keys.addrs.v4addrs.src = 0;
 656                 dsthost_hash = flow_hash_from_keys(&host_keys);
 657                 host_keys.addrs.v4addrs.src = keys.addrs.v4addrs.src;
 658                 host_keys.addrs.v4addrs.dst = 0;
 659                 srchost_hash = flow_hash_from_keys(&host_keys);
 660                 break;
 661
 662         case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
 663                 memset(&host_keys.addrs.v6addrs.src, 0,
 664                        sizeof(host_keys.addrs.v6addrs.src));
 665                 dsthost_hash = flow_hash_from_keys(&host_keys);
 666                 host_keys.addrs.v6addrs.src = keys.addrs.v6addrs.src;
 667                 memset(&host_keys.addrs.v6addrs.dst, 0,
 668                        sizeof(host_keys.addrs.v6addrs.dst));
 669                 srchost_hash = flow_hash_from_keys(&host_keys);
 670                 break;
 671
 672         default:
 673                 dsthost_hash = 0;
 674                 srchost_hash = 0;
 675         }
 676
 677         /* This *must* be after the above switch, since as a
 678          * side-effect it sorts the src and dst addresses.
 679          */
 680         if (flow_mode & CAKE_FLOW_FLOWS)
 681                 flow_hash = flow_hash_from_keys(&keys);
 682
 683 skip_hash:
 684         if (flow_override)
 685                 flow_hash = flow_override - 1;
 686         if (host_override) {
 687                 dsthost_hash = host_override - 1;
 688                 srchost_hash = host_override - 1;
 689         }
 690
 691         if (!(flow_mode & CAKE_FLOW_FLOWS)) {
 692                 if (flow_mode & CAKE_FLOW_SRC_IP)
 693                         flow_hash ^= srchost_hash;
 694
 695                 if (flow_mode & CAKE_FLOW_DST_IP)
 696                         flow_hash ^= dsthost_hash;
 697         }
 698
 699         reduced_hash = flow_hash % CAKE_QUEUES;

Re: [Cake] host hashes and NAT neglected in src/dst-host mode

[Jonathan Morton]

> On 7 Jan, 2019, at 6:00 am, <gamanakis@gmail.com> <gamanakis@gmail.com> wrote:
> 
> 633         if ((flow_override || !(flow_mode & CAKE_FLOW_FLOWS)) &&
> 634             (host_override || !(flow_mode & CAKE_FLOW_HOSTS)))
> 635                 goto skip_hash;

These lines require careful reading.

First, the "override" flags indicate whether an external filter has changed the flow or host hashes, meaning we should not then update them ourselves.

Secondly, the logic is "if we *don't* need the flow hash *and* we *don't* need the host hash, then skip the complicated hash code".

In the dual and triple modes, both the flow and host hashes are required, and bit-level examination of the codes used to identify them should reflect that.  In "flows" and "host" modes, only one or the other are needed, but they will still disable the above check (unless an external filter was used).

In short, only "flowblind" mode or the use of external filters are capable of skipping that block.

 - Jonathan Morton

Re: [Cake] host hashes and NAT neglected in src/dst-host mode

[Georgios Amanakis]

Yes, of course! I was counting the flow-modes wrong.
Sorry for the confusion.

On Mon, Jan 7, 2019 at 3:20 AM Jonathan Morton <chromatix99@gmail.com> wrote:
>
> > On 7 Jan, 2019, at 6:00 am, <gamanakis@gmail.com> <gamanakis@gmail.com> wrote:
> >
> > 633         if ((flow_override || !(flow_mode & CAKE_FLOW_FLOWS)) &&
> > 634             (host_override || !(flow_mode & CAKE_FLOW_HOSTS)))
> > 635                 goto skip_hash;
>
> These lines require careful reading.
>
> First, the "override" flags indicate whether an external filter has changed the flow or host hashes, meaning we should not then update them ourselves.
>
> Secondly, the logic is "if we *don't* need the flow hash *and* we *don't* need the host hash, then skip the complicated hash code".
>
> In the dual and triple modes, both the flow and host hashes are required, and bit-level examination of the codes used to identify them should reflect that.  In "flows" and "host" modes, only one or the other are needed, but they will still disable the above check (unless an external filter was used).
>
> In short, only "flowblind" mode or the use of external filters are capable of skipping that block.
>
>  - Jonathan Morton
>