netdev - Re: [PATCH net-next v2] bonding: add a vlan+mac tx hashing option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210114214203.GI1171031@redhat.com>
Date:   Thu, 14 Jan 2021 16:42:03 -0500
From:   Jarod Wilson <jarod@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     linux-kernel@...r.kernel.org, Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        "David S. Miller" <davem@...emloft.net>,
        Thomas Davis <tadavis@....gov>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] bonding: add a vlan+mac tx hashing option

On Thu, Jan 14, 2021 at 01:23:14PM -0800, Jakub Kicinski wrote:
> On Thu, 14 Jan 2021 16:11:41 -0500 Jarod Wilson wrote:
> > In truth, this code started out as a copy of bond_eth_hash(), which also
> > only uses the last byte, though of both source and destination macs. In
> > the typical use case for the requesting user, the bond is formed from two
> > onboard NICs, which typically have adjacent mac addresses, i.e.,
> > AA:BB:CC:DD:EE:01 and AA:BB:CC:DD:EE:02, so only the last byte is really
> > relevant to hash differently, but in thinking about it, a replacement NIC
> > because an onboard one died could have the same last byte, and maybe we
> > ought to just go full source mac right off the go here.
> > 
> > Something like this instead maybe:
> > 
> > static u32 bond_vlan_srcmac_hash(struct sk_buff *skb)
> > {
> >         struct ethhdr *mac_hdr = (struct ethhdr *)skb_mac_header(skb);
> >         u32 srcmac = 0;
> >         u16 vlan;
> >         int i;
> > 
> >         for (i = 0; i < ETH_ALEN; i++)
> >                 srcmac = (srcmac << 8) | mac_hdr->h_source[i];
> > 
> >         if (!skb_vlan_tag_present(skb))
> >                 return srcmac;
> > 
> >         vlan = skb_vlan_tag_get(skb);
> > 
> >         return vlan ^ srcmac;
> > }
> > 
> > Then the documentation is spot-on, and we're future-proof, though
> > marginally less performant in calculating the hash, which may have been a
> > consideration when the original function was written, but is probably
> > basically irrelevant w/modern systems...
> 
> No preference, especially if bond_eth_hash() already uses the last byte.
> Just make sure the choice is explained in the commit message.

I've sold myself on using the full MAC, because if there's no vlan tag
present, mac is the only thing used for the hash, increasing the chances
of getting the same hash for two different interfaces, which won't happen
if we've got the full MAC. Of course, I'm not sure why someone would be
using this xmit hash outside of the very particular use-case that includes
VLANs, but people do strange things...

-- 
Jarod Wilson
jarod@...hat.com