| 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
| ||
|
Message-ID: <13087238-6a57-439e-b7cb-b465b9e27cd6@kernel.org>
Date: Wed, 22 Nov 2023 00:19:38 +1000
From: Greg Ungerer <gerg@...nel.org>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: phylink: require supported_interfaces to be
filled
Hi Russell,
On 20/5/23 20:41, Russell King (Oracle) wrote:
> We have been requiring the supported_interfaces bitmap to be filled in
> by MAC drivers that have a mac_select_pcs() method. Now that all MAC
> drivers fill in the supported_interfaces bitmap, it is time to enforce
> this. We have already required supported_interfaces to be set in order
> for optical SFPs to be configured in commit f81fa96d8a6c ("net: phylink:
> use phy_interface_t bitmaps for optical modules").
>
> Refuse phylink creation if supported_interfaces is empty, and remove
> code to deal with cases where this mask is empty.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> ---
> I believe what I've said above is indeed the case, but there is always
> the chance that something has been missed and this will cause breakage.
> I would post as RFC and ask for testing, but in my experience that is
> a complete waste of time as it doesn't result in any testing feedback.
> So, it's probably better to get it merged into net-next and then wait
> for any reports of breakage.
This commit breaks a platform I have with a Marvell 88e6350 switch.
During boot up it now fails with:
...
mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
mv88e6085 d0072004.mdio-mii:11: phylink: error: empty supported_interfaces
error creating PHYLINK: -22
mv88e6085: probe of d0072004.mdio-mii:11 failed with error -22
...
The 6350 looks to be similar to the 6352 in many respects, though it lacks
a SERDES interface, but it otherwise mostly seems compatible. Using the 6352
phylink_get_caps function instead of the 6185 one fixes this:
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5418,7 +5418,7 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
.set_max_frame_size = mv88e6185_g1_set_max_frame_size,
.stu_getnext = mv88e6352_g1_stu_getnext,
.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
- .phylink_get_caps = mv88e6185_phylink_get_caps,
+ .phylink_get_caps = mv88e6352_phylink_get_caps,
};
static const struct mv88e6xxx_ops mv88e6351_ops = {
The story doesn't quite end here though. With this fix in place support
for the 6350 is then again broken by commit b92143d4420f ("net: dsa:
mv88e6xxx: add infrastructure for phylink_pcs"). This results in a dump
on boot up:
...
mv88e6085 d0072004.mdio-mii:11: switch 0x3710 detected: Marvell 88E6350, revision 2
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000000 when read
[00000000] *pgd=00000000
Internal error: Oops: 5 [#1] ARM
Modules linked in:
CPU: 0 PID: 8 Comm: kworker/u2:0 Not tainted 6.7.0-rc2-dirty #26
Hardware name: Marvell Armada 370/XP (Device Tree)
Workqueue: events_unbound deferred_probe_work_func
PC is at mv88e6xxx_port_setup+0x1c/0x44
LR is at dsa_port_devlink_setup+0x74/0x154
pc : [<c057ea24>] lr : [<c0819598>] psr: a0000013
sp : c184fce0 ip : c542b8f4 fp : 00000000
r10: 00000001 r9 : c542a540 r8 : c542bc00
r7 : c542b838 r6 : c5244580 r5 : 00000005 r4 : c5244580
r3 : 00000000 r2 : c542b840 r1 : 00000005 r0 : c1a02040
...
I can see that the mv88e6350_ops struct doesn't have an initializer for
pcs_ops. Based on the similarity with the 6352 again I tried using the
mv88e6352_pcs_ops for that:
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -5069,7 +5069,8 @@ static const struct mv88e6xxx_ops mv88e6350_ops = {
.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
.stu_getnext = mv88e6352_g1_stu_getnext,
.stu_loadpurge = mv88e6352_g1_stu_loadpurge,
- .phylink_get_caps = mv88e6185_phylink_get_caps,
+ .phylink_get_caps = mv88e6352_phylink_get_caps,
+ .pcs_ops = &mv88e6352_pcs_ops,
};
This gets the 6350 switch back to working again (on current linux 6.7-rc2).
I am not entirely sure if this needs a dedicated phylink_get_caps
and pcs_ops for the 6350, or if it is safe to use the 6352 ones?
Looking at the mv88e6351_ops I am guessing it is going to suffer the
same problems too.
Regards
Greg
> drivers/net/phy/phylink.c | 26 +++++++++++---------------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a4dd5197355a..093b7b6e0263 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -712,14 +712,11 @@ static int phylink_validate(struct phylink *pl, unsigned long *supported,
> {
> const unsigned long *interfaces = pl->config->supported_interfaces;
>
> - if (!phy_interface_empty(interfaces)) {
> - if (state->interface == PHY_INTERFACE_MODE_NA)
> - return phylink_validate_mask(pl, supported, state,
> - interfaces);
> + if (state->interface == PHY_INTERFACE_MODE_NA)
> + return phylink_validate_mask(pl, supported, state, interfaces);
>
> - if (!test_bit(state->interface, interfaces))
> - return -EINVAL;
> - }
> + if (!test_bit(state->interface, interfaces))
> + return -EINVAL;
>
> return phylink_validate_mac_and_pcs(pl, supported, state);
> }
> @@ -1513,19 +1510,18 @@ struct phylink *phylink_create(struct phylink_config *config,
> struct phylink *pl;
> int ret;
>
> - if (mac_ops->mac_select_pcs &&
> - mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
> - ERR_PTR(-EOPNOTSUPP))
> - using_mac_select_pcs = true;
> -
> /* Validate the supplied configuration */
> - if (using_mac_select_pcs &&
> - phy_interface_empty(config->supported_interfaces)) {
> + if (phy_interface_empty(config->supported_interfaces)) {
> dev_err(config->dev,
> - "phylink: error: empty supported_interfaces but mac_select_pcs() method present\n");
> + "phylink: error: empty supported_interfaces\n");
> return ERR_PTR(-EINVAL);
> }
>
> + if (mac_ops->mac_select_pcs &&
> + mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
> + ERR_PTR(-EOPNOTSUPP))
> + using_mac_select_pcs = true;
> +
> pl = kzalloc(sizeof(*pl), GFP_KERNEL);
> if (!pl)
> return ERR_PTR(-ENOMEM);
Powered by blists - more mailing lists