netdev - Re: [PATCH v4] net: macb: Fix several edge cases in validate

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:   Mon, 25 Oct 2021 22:19:01 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Sean Anderson <sean.anderson@...o.com>
Cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Antoine Tenart <atenart@...nel.org>
Subject: Re: [PATCH v4] net: macb: Fix several edge cases in validate

On Mon, Oct 25, 2021 at 01:24:05PM -0400, Sean Anderson wrote:
> There were several cases where validate() would return bogus supported
> modes with unusual combinations of interfaces and capabilities. For
> example, if state->interface was 10GBASER and the macb had HIGH_SPEED
> and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
> another case, SGMII could be enabled even if the mac was not a GEM
> (despite this being checked for later on in mac_config()). These
> inconsistencies make it difficult to refactor this function cleanly.
> 
> This attempts to address these by reusing the same conditions used to
> decide whether to return early when setting mode bits. The logic is
> pretty messy, but this preserves the existing logic where possible.
> 
> Signed-off-by: Sean Anderson <sean.anderson@...o.com>
> ---
> 
> Changes in v4:
> - Drop cleanup patch
> 
> Changes in v3:
> - Order bugfix patch first
> 
> Changes in v2:
> - New
> 
>  drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
>  1 file changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 309371abfe23..40bd5a069368 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config,
>  			  unsigned long *supported,
>  			  struct phylink_link_state *state)
>  {
> +	bool have_1g = true, have_10g = true;
>  	struct net_device *ndev = to_net_dev(config->dev);
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };

I think DaveM would ask for this to be reverse-christmas-tree, so the
new bool should be here.

>  	struct macb *bp = netdev_priv(ndev);
>  
> -	/* We only support MII, RMII, GMII, RGMII & SGMII. */
> +	/* There are three major types of interfaces we support:
> +	 * - (R)MII supporting 10/100 Mbit/s
> +	 * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
> +	 * - 10GBASER supporting 10 Gbit/s only
> +	 */
>  	if (state->interface != PHY_INTERFACE_MODE_NA &&
>  	    state->interface != PHY_INTERFACE_MODE_MII &&
>  	    state->interface != PHY_INTERFACE_MODE_RMII &&
> @@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config,
>  		return;
>  	}
>  
> -	if (!macb_is_gem(bp) &&
> -	    (state->interface == PHY_INTERFACE_MODE_GMII ||
> -	     phy_interface_mode_is_rgmii(state->interface))) {
> -		linkmode_zero(supported);
> -		return;
> +	/* For 1G and up we must have both have a GEM and GIGABIT_MODE */
> +	if (!macb_is_gem(bp) ||
> +	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
> +		if (state->interface == PHY_INTERFACE_MODE_GMII ||
> +		    phy_interface_mode_is_rgmii(state->interface) ||
> +		    state->interface == PHY_INTERFACE_MODE_SGMII ||
> +		    state->interface == PHY_INTERFACE_MODE_10GBASER) {
> +			linkmode_zero(supported);
> +			return;
> +		} else if (state->interface == PHY_INTERFACE_MODE_NA) {
> +			have_1g = false;
> +			have_10g = false;
> +		}
>  	}

Would it make more sense to do:

	bool have_1g = false, have_10g = false;

	if (macb_is_gem(bp) &&
	    (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
		if (bp->caps & MACB_CAPS_PCS)
			have_1g = true;
		if (bp->caps & MACB_CAPS_HIGH_SPEED)
			have_10g = true;
	}

	switch (state->interface) {
	case PHY_INTERFACE_MODE_NA:
	case PHY_INTERFACE_MODE_MII:
	case PHY_INTERFACE_MODE_RMII:
		break;

	case PHY_INTERFACE_MODE_GMII:
	case PHY_INTERFACE_MODE_RGMII:
	case PHY_INTERFACE_MODE_RGMII_ID:
	case PHY_INTERFACE_MODE_RGMII_RXID:
	case PHY_INTERFACE_MODE_RGMII_TXID:
	case PHY_INTERFACE_MODE_SGMII:
		if (!have_1g) {
			linkmode_zero(supported);
			return;
		}
		break;

	case PHY_INTERFACE_MODE_10GBASER:
		if (!have_10g) {
			linkmode_zero(supported);
			return;
		}
		break;

	default:
		linkmode_zero(supported);
		return;
	}

This uses positive logic to derive have_1g and have_10g, and then uses
the switch statement to validate against those. Would the above result
in more understandable code?

Thanks.

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