netdev - Re: [PATCH v2 net-next] net: dsa: microchip: Add MIB counter reading support

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] [day] [month] [year] [list]

Message-ID: <20171117041856.GE27450@lunn.ch>
Date:   Fri, 17 Nov 2017 05:18:56 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Tristram.Ha@...rochip.com
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Pavel Machek <pavel@....cz>, muvarov@...il.com,
        nathan.leigh.conrad@...il.com, vivien.didelot@...oirfairelinux.com,
        UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next] net: dsa: microchip: Add MIB counter reading
 support

On Thu, Nov 16, 2017 at 06:42:50PM -0800, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>
> 
> Add MIB counter reading support.

Hi Tristram

Some more details would be good here.

> +static void ksz9477_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
> +			      u64 *dropped, u64 *cnt)
> +{
> +	addr = ksz9477_mib_names[addr].index;
> +	ksz9477_r_mib_cnt(dev, port, addr, cnt);
> +}

dropped is unused here. Which seems to make dropped in general unused.

> +static void ksz_mib_read_work(struct work_struct *work)
> +{
> +	struct ksz_device *dev =
> +		container_of(work, struct ksz_device, mib_read);
> +	struct ksz_port *p;
> +	struct ksz_port_mib *mib;
> +	int i;
> +
> +	for (i = 0; i < dev->mib_port_cnt; i++) {
> +		p = &dev->ports[i];
> +		if (!p->on)
> +			continue;
> +		mib = &p->mib;
> +		mutex_lock(&mib->cnt_mutex);
> +
> +		/* read only dropped counters when link is not up */
> +		if (p->link_down)
> +			p->link_down = 0;

It is not obvious to me what this is doing.

> +		else if (!p->link_up)
> +			mib->cnt_ptr = dev->reg_mib_cnt;
> +		port_r_cnt(dev, i);
> +		mutex_unlock(&mib->cnt_mutex);
> +	}
> +}
> +
> +void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port_mib *mib;
> +
> +	mib = &dev->ports[port].mib;
> +
> +	/* freeze MIB counters if supported */
> +	if (dev->dev_ops->freeze_mib)
> +		dev->dev_ops->freeze_mib(dev, port, true);
> +	mutex_lock(&mib->cnt_mutex);
> +	port_r_cnt(dev, port);
> +	mutex_unlock(&mib->cnt_mutex);
> +	if (dev->dev_ops->freeze_mib)
> +		dev->dev_ops->freeze_mib(dev, port, false);
> +	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));

Should the memcpy be made while holding the mutex? As soon as you
release it, the timer code can start updating the values in
mib->counters. That then makes the freeze pointless.

>  void ksz_switch_remove(struct ksz_device *dev)
>  {
> +	/* timer started */
> +	if (dev->mib_read_timer.expires) {
> +		del_timer_sync(&dev->mib_read_timer);
> +		flush_work(&dev->mib_read);
> +	}

Is this race free?

   Andrew