netdev - Re: [PATCH] net: phylink: Update SFP selected interface on advertising changes

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, 27 Aug 2021 12:11:23 +1000
From:   Nathan Rossi <nathan@...hanrossi.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, Nathan Rossi <nathan.rossi@...i.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH] net: phylink: Update SFP selected interface on
 advertising changes

On Thu, 26 Aug 2021 at 19:25, Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> On Thu, Aug 26, 2021 at 07:12:30AM +0000, Nathan Rossi wrote:
> > From: Nathan Rossi <nathan.rossi@...i.com>
> >
> > Currently changes to the advertising state via ethtool do not cause any
> > reselection of the configured interface mode after the SFP is already
> > inserted and initially configured.
> >
> > While it is not typical to change the advertised link modes for an
> > interface using an SFP in certain use cases it is desirable. In the case
> > of a SFP port that is capable of handling both SFP and SFP+ modules it
> > will automatically select between 1G and 10G modes depending on the
> > supported mode of the SFP. However if the SFP module is capable of
> > working in multiple modes (e.g. a SFP+ DAC that can operate at 1G or
> > 10G), one end of the cable may be attached to a SFP 1000base-x port thus
> > the SFP+ end must be manually configured to the 1000base-x mode in order
> > for the link to be established.
> >
> > This change causes the ethtool setting of advertised mode changes to
> > reselect the interface mode so that the link can be established.
>
> This may be a better solution than the phylink_helper_basex_speed()
> approach used to select between 2500 and 1000 base-X, but the config
> needs to be validated after selecting a different interface mode, as
> per what phylink_sfp_config() does.

I will add this in a v2. But will wait to get your feedback on the below before
sending that out.

>
> I also suspect that this will allow you to select e.g. 1000base-X but
> there will be no way back to 10G modes as they will be masked out of
> the advertising mask from that point on, as the 1000base-X interface
> mode does not allow them.

Because only the advertising bitmap is changed it is possible to revert any
changes because the supported bitmap is not affected. Although I may be
misunderstanding the exact details of the issue you are describing?

Note, the phylink_sfp_config phylink_validate call after sfp_select_interface
does not modify the supported bitmap so adding that validate call in this change
will not affect the ability to reselect changed advertised bits.

I did some additional testing and noticed that the advertising mask is not
updated in phylink_sfp_config if supported does not change (e.g. SFP reinsert),
which leads to the advertising state mismatching (e.g. advertising at 1G, but
actually operating at 10G). This just needs to check if the advertising also
mismatches in phylink_sfp_config.

Thanks,
Nathan

>
> So, I think the suggested code is problematical as it stands.
>
> phylink_sfp_config() uses a multi-step approach to selecting the
> interface mode for a reason - first step is to discover what the MAC
> is capable of in _any_ interface mode using _NA to reduce the supported
> and advertised mask down, and then to select the interface from that.
> I'm not entirely convinced that is a good idea here yet though.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!