netdev - Re: [PATCH v3 net-next 3/4] net: dsa: microchip: get port link status

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: <1ae2439f-f285-228f-b223-c07e97ee6ce2@gmail.com>
Date:   Thu, 21 Feb 2019 14:55:43 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Tristram.Ha@...rochip.com,
        Sergio Paracuellos <sergio.paracuellos@...il.com>,
        Andrew Lunn <andrew@...n.ch>, Pavel Machek <pavel@....cz>
Cc:     UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 3/4] net: dsa: microchip: get port link status

On 2/21/19 2:03 PM, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>
> 
> Get port link status to know whether to read MIB counters when the link
> is going down.  Add port_cleanup function to read MIB counters the last
> time as when the port is disabled the PHY is also powered down.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@...rochip.com>
> ---

Some comments here and there again, not against a merge, but would be
nice to address in the future.

[snip]

> +void ksz_port_cleanup(struct ksz_device *dev, int port)
> +{
> +	/* Read all MIB counters when the link is going down. */
> +	if (dev->live_ports & (1 << port)) {
> +		struct ksz_port *p = &dev->ports[port];
> +
> +		p->read = true;
> +		schedule_work(&dev->mib_read);
> +	}
> +
> +	/* Common code for port cleanup. */
> +	dev->on_ports &= ~(1 << port);

It seems to be that dev->on_ports is a shorthand for a port is enabled
whether its link is up or down, which is equivalent to using:
!dsa_port_is_unused(ds, port)) unless I am mistaken about the intention
of the code.

In general you seem to be very prone to adding a lot of these
bookkeeping variables which just creates more opportunities for bugs to
creep in.

> +	dev->live_ports &= ~(1 << port);
> +}
> +EXPORT_SYMBOL_GPL(ksz_port_cleanup);
> +
>  void ksz_update_port_member(struct ksz_device *dev, int port)
>  {
>  	struct ksz_port *p;
> @@ -156,6 +172,26 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
>  }
>  EXPORT_SYMBOL_GPL(ksz_phy_write16);
>  
> +void ksz_adjust_link(struct dsa_switch *ds, int port,
> +		     struct phy_device *phydev)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	if (!phydev->link) {
> +		/* Read all MIB counters when the link is going down. */
> +		if (dev->live_ports & (1 << port)) {
> +			p->read = true;
> +			schedule_work(&dev->mib_read);
> +		}
> +		dev->live_ports &= ~(1 << port);
> +	} else {
> +		/* Remember which port is connected and active. */
> +		dev->live_ports |= (1 << port) & dev->on_ports;
> +	}

How about something simpler than this:

	if (phydev->link != p->old_link) {
		p->read = !!phydev->link;
		schedule_work(&dev->mib_read);
	}

This is also super racy because you schedule the workqueue without any
locking against the state machine which runs with the PHY device's mutex
held, so there is the possibility of the following happening:

Thread 0			Thread 1

ksz_adjust_link()
	phydev->link = 0
	schedule_work()
				ksz_adjust_link()
				p->read = true
	mib_read_workqueue()

and so we don't read counters anymore. It is probably fine since this is
clearly entirely opportunistic and in order to minimize register bandwidth.
-- 
Florian