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: <Zwllt43iS5EDvjHN@shell.armlinux.org.uk>
Date: Fri, 11 Oct 2024 18:51:51 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
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 Fri, Oct 11, 2024 at 03:54:21PM +0300, Vladimir Oltean wrote:
> On Fri, Oct 11, 2024 at 11:58:07AM +0100, Russell King (Oracle) wrote:
> > On Fri, Oct 11, 2024 at 01:39:12PM +0300, Vladimir Oltean wrote:
> > > 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.
> > 
> > I wanted to add support for phylink_set_pcs() to remove the current
> > PCS and submitted a patch for it. You didn't see a use case and objected
> > to the patch, which wasn't merged.
> 
> It was an RFC, it wasn't a candidate for merging anyway.

What does that have to do with it????????????

An idea is put forward (the idea of allowing PCS to be removed.) It's
put forward as a RFC. It gets shot down. Author then goes away believing
that there is no desire to allow PCS to be removed. That idea gets
carried forward into future patches.

_That_ is what exactly happened. I'm not attributing blame for it,
merely explaining how we got to where we are with this, and how we've
ended up in the mess we have with PCS able to be used outside of its
validated set.

You want me to provide more explanation on the patch, but I've
identified a fundamental error here caused as an effect of a previous
review comment.

I'm now wondering what to do about it and how to solve this in a way
that won't cause us to go around another long confrontational discussion
but it seems that's not possible.

So, do I ignore your review comments and just do what I think is the
right thing, or do I attempt to discuss it with you? I think, given
_this_ debacle, I ignore you. I would much rather involve you but it
seems that's a mistake.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!