netdev - Re: [PATCH net-next v3 04/10] net: dsa: Allow drivers to filter packets they can decode source port from

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 for Android: free password hash cracker in your pocket

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

Date:   Sun, 5 May 2019 21:29:13 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v3 04/10] net: dsa: Allow drivers to filter
 packets they can decode source port from

On Sun, 5 May 2019 at 20:02, Florian Fainelli <f.fainelli@...il.com> wrote:
>
>
>
> On 5/5/2019 3:19 AM, Vladimir Oltean wrote:
> > Frames get processed by DSA and redirected to switch port net devices
> > based on the ETH_P_XDSA multiplexed packet_type handler found by the
> > network stack when calling eth_type_trans().
> >
> > The running assumption is that once the DSA .rcv function is called, DSA
> > is always able to decode the switch tag in order to change the skb->dev
> > from its master.
> >
> > However there are tagging protocols (such as the new DSA_TAG_PROTO_SJA1105,
> > user of DSA_TAG_PROTO_8021Q) where this assumption is not completely
> > true, since switch tagging piggybacks on the absence of a vlan_filtering
> > bridge. Moreover, management traffic (BPDU, PTP) for this switch doesn't
> > rely on switch tagging, but on a different mechanism. So it would make
> > sense to at least be able to terminate that.
> >
> > Having DSA receive traffic it can't decode would put it in an impossible
> > situation: the eth_type_trans() function would invoke the DSA .rcv(),
> > which could not change skb->dev, then eth_type_trans() would be invoked
> > again, which again would call the DSA .rcv, and the packet would never
> > be able to exit the DSA filter and would spiral in a loop until the
> > whole system dies.
> >
> > This happens because eth_type_trans() doesn't actually look at the skb
> > (so as to identify a potential tag) when it deems it as being
> > ETH_P_XDSA. It just checks whether skb->dev has a DSA private pointer
> > installed (therefore it's a DSA master) and that there exists a .rcv
> > callback (everybody except DSA_TAG_PROTO_NONE has that). This is
> > understandable as there are many switch tags out there, and exhaustively
> > checking for all of them is far from ideal.
> >
> > The solution lies in introducing a filtering function for each tagging
> > protocol. In the absence of a filtering function, all traffic is passed
> > to the .rcv DSA callback. The tagging protocol should see the filtering
> > function as a pre-validation that it can decode the incoming skb. The
> > traffic that doesn't match the filter will bypass the DSA .rcv callback
> > and be left on the master netdevice, which wasn't previously possible.
> >
> > Signed-off-by: Vladimir Oltean <olteanv@...il.com>
> > --
> just one nit below:
>
> Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
>
> [snip]
>
> > -     if (unlikely(netdev_uses_dsa(dev)))
> > +     if (unlikely(netdev_uses_dsa(dev)) && dsa_can_decode(skb, dev))
> >               return htons(ETH_P_XDSA);
>
> Just in case you need to resubmit, should not the dsa_can_decode() also
> be part of the unlikely() statement here?
>
> --
> Florian

Hi Florian,

Does this matter, since the compiler performs short-circuit evaluation
and the first condition is already unlikely?
Also, if the netdev does use DSA, conceptually it is not unlikely for
it to be able to decode packets, hence it is strange to optimize the
branch predictor for that.

-Vladimir