netdev - Re: [RFC PATCH v2 net-next 0/4] DSA master state tracking

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]

Date:   Fri, 10 Dec 2021 17:02:42 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Ansuel Smith <ansuelsmth@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [RFC PATCH v2 net-next 0/4] DSA master state tracking

On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:
> On Thu, Dec 09, 2021 at 07:39:23PM +0200, Vladimir Oltean wrote:
> > This patch set is provided solely for review purposes (therefore not to
> > be applied anywhere) and for Ansuel to test whether they resolve the
> > slowdown reported here:
> > https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
> > 
> > The patches posted here are mainly to offer a consistent
> > "master_state_change" chain of events to switches, without duplicates,
> > and always starting with operational=true and ending with
> > operational=false. This way, drivers should know when they can perform
> > Ethernet-based register access, and need not care about more than that.
> > 
> > Changes in v2:
> > - dropped some useless patches
> > - also check master operstate.
> > 
> > Vladimir Oltean (4):
> >   net: dsa: provide switch operations for tracking the master state
> >   net: dsa: stop updating master MTU from master.c
> >   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
> >   net: dsa: replay master state events in
> >     dsa_tree_{setup,teardown}_master
> > 
> >  include/net/dsa.h  | 11 +++++++
> >  net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
> >  net/dsa/dsa_priv.h | 13 ++++++++
> >  net/dsa/master.c   | 29 ++---------------
> >  net/dsa/slave.c    | 27 ++++++++++++++++
> >  net/dsa/switch.c   | 15 +++++++++
> >  6 files changed, 145 insertions(+), 30 deletions(-)
> > 
> > -- 
> > 2.25.1
> > 
> 
> Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
> I don't think we have other way to track this. Am I wrong?
> 
> All works correctly with this and promisc_on_master.
> If you have other test, feel free to send me other stuff to test.
> 
> (I'm starting to think the fail is caused by some delay that the switch
> require to actually start accepting packet or from the reinit? But I'm
> not sure... don't know if you notice something from the pcap)

I've opened the pcap just now. The Ethernet MDIO packets are
non-standard. When the DSA master receives them, it expects the first 6
octets to be the MAC DA, because that's the format of an Ethernet frame.
But the packets have this other format, according to your own writing:

/* Specific define for in-band MDIO read/write with Ethernet packet */
#define QCA_HDR_MDIO_SEQ_LEN           4 /* 4 byte for the seq */
#define QCA_HDR_MDIO_COMMAND_LEN       4 /* 4 byte for the command */
#define QCA_HDR_MDIO_DATA1_LEN         4 /* First 4 byte for the mdio data */
#define QCA_HDR_MDIO_HEADER_LEN        (QCA_HDR_MDIO_SEQ_LEN + \
                                       QCA_HDR_MDIO_COMMAND_LEN + \
                                       QCA_HDR_MDIO_DATA1_LEN)

#define QCA_HDR_MDIO_DATA2_LEN         12 /* Other 12 byte for the mdio data */
#define QCA_HDR_MDIO_PADDING_LEN       34 /* Padding to reach the min Ethernet packet */

The first 6 octets change like crazy in your pcap. Definitely can't add
that to the RX filter of the DSA master.

So yes, promisc_on_master is precisely what you need, it exists for
situations like this.

Considering this, I guess it works?