[PATCH net 0/2] net: macb: fix the link speed the taprio setup reads

[PATCH net 0/2] net: macb: fix the link speed the taprio setup reads

[Aleksei Sviridkin]

Two small fixes in macb_taprio_setup_replace(), both in how it obtains
the link speed it scales the schedule with.

The first: it hands phylink_ethtool_ksettings_get() a stack variable
it never zeroed, while phylink fills only what the link mode provides
and even reads one field back from the caller. The second: the speed
check is written as "<= 0" on a u32, so SPEED_UNKNOWN passes it and
turns into a 1 ns hardware limit that every entry then exceeds.

Compile-tested against net; the driver has no test surface, and no
macb board here.

Aleksei Sviridkin (2):
  net: macb: zero the link settings taprio reads back
  net: macb: reject an unknown link speed in the taprio setup

 drivers/net/ethernet/cadence/macb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.53.0

[PATCH net 1/2] net: macb: zero the link settings taprio reads back

[Aleksei Sviridkin]

phylink_ethtool_ksettings_get() fills only what the current link mode
provides. With no PHY attached it writes port and supported; on a fixed
or in-band link it also writes speed and duplex, but only if
base.rate_matching already reads RATE_MATCH_NONE, a field it never
writes itself and so takes from the caller. The ethtool core zeroes the
structure on every path into the op, so its callers never notice; the
taprio path passes a stack variable and reads a speed nobody wrote.

Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 76ee4f506033..a43855db1e45 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4297,7 +4297,7 @@ static int macb_taprio_setup_replace(struct net_device *netdev,
 	struct macb_queue_enst_config *enst_queue;
 	struct tc_taprio_sched_entry *entry;
 	struct macb *bp = netdev_priv(netdev);
-	struct ethtool_link_ksettings kset;
+	struct ethtool_link_ksettings kset = {};
 	struct macb_queue *queue;
 	u32 queue_mask;
 	u8 queue_id;
-- 
2.53.0

Re: [PATCH net 1/2] net: macb: zero the link settings taprio reads back

[Théo Lebrun]

Hello Aleksei,

On Wed Sep 2, 2026 at 10:05 AM CEST, Aleksei Sviridkin wrote:
> phylink_ethtool_ksettings_get() fills only what the current link mode
> provides. With no PHY attached it writes port and supported; on a fixed
> or in-band link it also writes speed and duplex, but only if
> base.rate_matching already reads RATE_MATCH_NONE, a field it never
> writes itself and so takes from the caller. The ethtool core zeroes the
> structure on every path into the op, so its callers never notice; the
> taprio path passes a stack variable and reads a speed nobody wrote.
>
> Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support")
> Assisted-by: LLM
> Signed-off-by: Aleksei Sviridkin <f@lex.la>

Agreed on the fix.

Could we improve the commit message? Point how straightforwardly the
case where uninitialised memory might be used, which is the main thing
we care about. See how the following is easier to parse out (for humans):

   phylink_ethtool_ksettings_get(bp->phylink, &kset) gets called with
   uninitialised kset but kset is not only an out-param. In some cases
   kset->base.rate_matching might be read.

   ... explain the cases in question ...

> ---
>  drivers/net/ethernet/cadence/macb_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 76ee4f506033..a43855db1e45 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4297,7 +4297,7 @@ static int macb_taprio_setup_replace(struct net_device *netdev,
>  	struct macb_queue_enst_config *enst_queue;
>  	struct tc_taprio_sched_entry *entry;
>  	struct macb *bp = netdev_priv(netdev);
> -	struct ethtool_link_ksettings kset;
> +	struct ethtool_link_ksettings kset = {};
>  	struct macb_queue *queue;
>  	u32 queue_mask;
>  	u8 queue_id;

Let's try to respect reverse xmas tree as we grow a variable.
I know it wasn't perfect before.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH net 2/2] net: macb: reject an unknown link speed in the taprio setup

[Aleksei Sviridkin]

speed is a u32, so SPEED_UNKNOWN arrives as 0xffffffff and passes the
"speed <= 0" check. A taprio schedule installed while the link is down
then has its hardware interval limit derived from that value, a limit
that rounds to 1 ns, and the first entry fails with a misleading
"exceeds hardware limit". Name the case instead of relying on the
sign.

Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support")
Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a43855db1e45..81530b9257b2 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4324,7 +4324,7 @@ static int macb_taprio_setup_replace(struct net_device *netdev,
 	}
 
 	speed = kset.base.speed;
-	if (unlikely(speed <= 0)) {
+	if (unlikely(speed == SPEED_UNKNOWN || !speed)) {
 		netdev_err(netdev, "Invalid speed: %d\n", speed);
 		return -EINVAL;
 	}
-- 
2.53.0

Re: [PATCH net 2/2] net: macb: reject an unknown link speed in the taprio setup

[Théo Lebrun]

Hello Aleksei,

On Wed Sep 2, 2026 at 10:05 AM CEST, Aleksei Sviridkin wrote:
> speed is a u32, so SPEED_UNKNOWN arrives as 0xffffffff and passes the
> "speed <= 0" check. A taprio schedule installed while the link is down
> then has its hardware interval limit derived from that value, a limit
> that rounds to 1 ns, and the first entry fails with a misleading
> "exceeds hardware limit". Name the case instead of relying on the
> sign.

I don't understand the

   Name the case instead of relying on the sign.

sentence, it seems to contradict what really happens which is

   SPEED_UNKNOWN [...] passes the "speed <= 0" check

> Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support")
> Assisted-by: LLM
> Signed-off-by: Aleksei Sviridkin <f@lex.la>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a43855db1e45..81530b9257b2 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4324,7 +4324,7 @@ static int macb_taprio_setup_replace(struct net_device *netdev,
>  	}
>  
>  	speed = kset.base.speed;
> -	if (unlikely(speed <= 0)) {
> +	if (unlikely(speed == SPEED_UNKNOWN || !speed)) {
>  		netdev_err(netdev, "Invalid speed: %d\n", speed);
>  		return -EINVAL;
>  	}

The zero case shouldn't happen, it's SPEED_UNKNOWN that can occur often
(on link down). It's basically the same as checking netif_running().

Doesn't the netdev_err() call deserve a fix in the SPEED_UNKNOWN case?
We might be printing "Invalid speed: 4294967295". I don't think we want
two format strings (0 or SPEED_UNKNOWN).
We could do "Invalid speed %d, link-down?\n".

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net 2/2] net: macb: reject an unknown link speed in the taprio setup

[Andrew Lunn]

On Wed, Sep 02, 2026 at 08:05:28AM +0000, Aleksei Sviridkin wrote:
> speed is a u32, so SPEED_UNKNOWN arrives as 0xffffffff and passes the
> "speed <= 0" check. A taprio schedule installed while the link is down
> then has its hardware interval limit derived from that value, a limit
> that rounds to 1 ns, and the first entry fails with a misleading
> "exceeds hardware limit". Name the case instead of relying on the
> sign.
> 
> Fixes: 89934dbf169e ("net: macb: Add TAPRIO traffic scheduling support")
> Assisted-by: LLM
> Signed-off-by: Aleksei Sviridkin <f@lex.la>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a43855db1e45..81530b9257b2 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4324,7 +4324,7 @@ static int macb_taprio_setup_replace(struct net_device *netdev,
>  	}
>  
>  	speed = kset.base.speed;
> -	if (unlikely(speed <= 0)) {
> +	if (unlikely(speed == SPEED_UNKNOWN || !speed)) {

Can ethtool_validate_speed() be used?

    Andrew

---
pw-bot: cr

Re: [PATCH net 2/2] net: macb: reject an unknown link speed in the taprio setup

[Aleksei Sviridkin]

> Can ethtool_validate_speed() be used?

No: it accepts SPEED_UNKNOWN by definition, and zero as well, so it
lets through exactly the values this check rejects.