netdev - Re: [PATCH net-next 3/3] net: phylink: remove "using_mac_select

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: <20241011103912.wmzozfnj6psgqtax@skbuf>
Date: Fri, 11 Oct 2024 13:39:12 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 3/3] net: phylink: remove "using_mac_select_pcs"

On Thu, Oct 10, 2024 at 02:00:32PM +0100, Russell King (Oracle) wrote:
> On Thu, Oct 10, 2024 at 12:21:43PM +0100, Russell King (Oracle) wrote:
> > Hmm. Looking at this again, we're getting into quite a mess because of
> > one of your previous review comments from a number of years back.
> > 
> > You stated that you didn't see the need to support a transition from
> > having-a-PCS to having-no-PCS. I don't have a link to that discussion.
> > However, it is why we've ended up with phylink_major_config() having
> > the extra complexity here, effectively preventing mac_select_pcs()
> > from being able to remove a PCS that was previously added:
> > 
> > 		pcs_changed = pcs && pl->pcs != pcs;
> > 
> > because if mac_select_pcs() returns NULL, it was decided that any
> > in-use PCS would not be removed. It seems (at least to me) to be a
> > silly decision now.
> > 
> > However, if mac_select_pcs() in phylink_major_config() returns NULL,
> > we don't do any validation of the PCS.
> > 
> > So this, today, before these patches, is already an inconsistent mess.
> > 
> > To fix this, I think:
> > 
> > 	struct phylink_pcs *pcs = NULL;
> > ...
> >         if (pl->mac_ops->mac_select_pcs) {
> >                 pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
> >                 if (IS_ERR(pcs))
> >                         return PTR_ERR(pcs);
> > 	}
> > 
> > 	if (!pcs)
> > 		pcs = pl->pcs;
> > 
> > is needed to give consistent behaviour.
> > 
> > Alternatively, we could allow mac_select_pcs() to return NULL, which
> > would then allow the PCS to be removed.
> > 
> > Let me know if you've changed your mind on what behaviour we should
> > have, because this affects what I do to sort this out.
> 
> Here's a link to the original discussion from November 2021:
> 
> https://lore.kernel.org/all/E1mpSba-00BXp6-9e@rmk-PC.armlinux.org.uk/
> 
> Google uselessly refused to find it, so I searched my own mailboxes
> to find the message ID.

Important note: I cannot find any discussion on any mailing list which
fills the gap between me asking what is the real world applicability of
mac_select_pcs() returning NULL after it has returned non-NULL, and the
current phylink behavior, as described above by you. That behavior was
first posted here:
https://lore.kernel.org/netdev/Ybiue1TPCwsdHmV4@shell.armlinux.org.uk/
in patches 1/7 and 2/7. I did not state that phylink should keep the old
PCS around, and I do not take responsibility for that.

Keeping in mind that I don't know whether anything has changed since
2021 which would make this condition any less theoretical than it was
back then, I guess if I were maintaining the code involved, I'd choose
between 2 options (whichever is easiest):

- Imagine a purely theoretical scenario where phylink transitions
  between a state->interface requiring a phylink_pcs, and one not
  requiring a phylink_pcs. I'm not even saying a serial PCS hardware
  block isn't present, just that it isn't modeled as a phylink_pcs
  (for reasons which may be valid or not). Probably the most logical
  thing to do in this scenario is allow the old phylink_pcs to be
  removed, and its ops never to be used for the new state->interface.

- Validate, possibly at phylink_validate_phy() time, that for all
  phy->possible_interfaces, mac_select_pcs() either returns NULL for
  all of them, or non-NULL for all of them. The idea would be to leave
  room for the use case to define itself (and the restriction to be
  lifted whenever necessary), instead of giving a predefined behavior
  for the transition when in reality we have no idea of the use case
  behind it. I don't know whether checking phy->possible_interfaces
  would be sufficient in ensuring that such a transition cannot occur.

I find no contradiction between my replies (mostly questions, actually)
in Nov 2021 and my current agreement that phylink's behavior of keeping
the old PCS and using it for the new state->interface doesn't make much
sense.