netdev - Re: [PATCH v6 net-next 08/15] net: allow ndo_get

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: <3e065920586e87e58a365eac94b69aabb3b244cb.camel@kernel.org>
Date:   Mon, 11 Jan 2021 14:24:52 -0800
From:   Saeed Mahameed <saeed@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Eric Dumazet <edumazet@...gle.com>,
        George McCollister <george.mccollister@...il.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Arnd Bergmann <arnd@...db.de>, Taehee Yoo <ap420073@...il.com>,
        Jiri Pirko <jiri@...nulli.us>, Florian Westphal <fw@...len.de>,
        Nikolay Aleksandrov <nikolay@...dia.com>,
        Pravin B Shelar <pshelar@....org>,
        Sridhar Samudrala <sridhar.samudrala@...el.com>
Subject: Re: [PATCH v6 net-next 08/15] net: allow ndo_get_stats64 to return
 an int error code

On Sat, 2021-01-09 at 19:26 +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> Some drivers need to do special tricks to comply with the new policy
> of
> ndo_get_stats64 being sleepable. For example, the bonding driver,
> which
> derives its stats from its lower interfaces, must recurse with
> dev_get_stats into its lowers with no locks held. But for that to
> work,
> it needs to dynamically allocate some memory for a refcounted copy of
> its array of slave interfaces (because recursing unlocked means that
> the
> original one is subject to disappearing). And since memory allocation
> can fail under pressure, we should not let it go unnoticed, but
> instead
> propagate the error code.
> 
> This patch converts all implementations of .ndo_get_stats64 to return
> int, and propagates that to the dev_get_stats calling site. Error
> checking will be done in further patches.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> Changes in v6:
> Remove the unused "int err" from __bond_release_one and add it in the
> patch it belongs to.
> 
> Changes in v5:
> None.
> 
> Changes in v4:
> Patch is new (Eric's suggestion).
> 

Just Give Eric the proper credit and add:

Suggested-by: Eric .. 


[...]

> @@ -354,4 +356,4 @@ int rmnet_vnd_update_dev_mtu(struct rmnet_port
> *port,
>  	}
>  
>  	return 0;
> -}
> \ No newline at end of file
> +}

Not related? 

[...]
> 
> -void dev_get_stats(struct net_device *dev, struct rtnl_link_stats64
> *storage)
> +int dev_get_stats(struct net_device *dev, struct rtnl_link_stats64
> *storage)
>  {
>  	const struct net_device_ops *ops = dev->netdev_ops;
> +	int err = 0;
>  
>  	if (ops->ndo_get_stats64) {
>  		memset(storage, 0, sizeof(*storage));
> -		ops->ndo_get_stats64(dev, storage);
> +		err = ops->ndo_get_stats64(dev, storage);
>  	} else if (ops->ndo_get_stats) {
>  		netdev_stats_to_stats64(storage, ops-
> >ndo_get_stats(dev));
>  	} else {
> @@ -10418,6 +10419,8 @@ void dev_get_stats(struct net_device *dev,
> struct rtnl_link_stats64 *storage)
>  	storage->rx_dropped += (unsigned long)atomic_long_read(&dev-
> >rx_dropped);
>  	storage->tx_dropped += (unsigned long)atomic_long_read(&dev-
> >tx_dropped);
>  	storage->rx_nohandler += (unsigned long)atomic_long_read(&dev-
> >rx_nohandler);

Must be consistent here, if there was an error you should abort without
touching the caller provided storage, even if you can for some stats.

> +
> +	return err;
>  }
>  EXPORT_SYMBOL(dev_get_stats);
>