| 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
| ||
|
Date: Thu, 28 Jan 2021 10:30:40 -0800
From: Alexander Duyck <alexander.duyck@...il.com>
To: Antoine Tenart <atenart@...nel.org>
Cc: David Miller <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 03/11] net-sysfs: move the xps cpus/rxqs
retrieval in a common function
On Thu, Jan 28, 2021 at 6:44 AM Antoine Tenart <atenart@...nel.org> wrote:
>
> Most of the xps_cpus_show and xps_rxqs_show functions share the same
> logic. Having it in two different functions does not help maintenance
> and we can already see small implementation differences. This should not
> be the case and this patch moves their common logic into a new function,
> xps_queue_show, to improve maintenance.
>
> While the rtnl lock could be held in the new xps_queue_show, it is still
> held in xps_cpus_show and xps_rxqs_show as this is an important
> information when looking at those two functions. This does not add
> complexity.
>
> Signed-off-by: Antoine Tenart <atenart@...nel.org>
> ---
> net/core/net-sysfs.c | 168 ++++++++++++++++++++-----------------------
> 1 file changed, 79 insertions(+), 89 deletions(-)
>
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 5a39e9b38e5f..6e6bc05181f6 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -1314,77 +1314,98 @@ static const struct attribute_group dql_group = {
> #endif /* CONFIG_BQL */
>
> #ifdef CONFIG_XPS
> -static ssize_t xps_cpus_show(struct netdev_queue *queue,
> - char *buf)
> +/* Should be called with the rtnl lock held. */
> +static int xps_queue_show(struct net_device *dev, unsigned long **mask,
> + unsigned int index, bool is_rxqs_map)
> {
> - int cpu, len, ret, num_tc = 1, tc = 0;
> - struct net_device *dev = queue->dev;
> + const unsigned long *possible_mask = NULL;
> + int j, num_tc = 0, tc = 0, ret = 0;
> struct xps_dev_maps *dev_maps;
> - unsigned long *mask;
> - unsigned int index;
> -
> - if (!netif_is_multiqueue(dev))
> - return -ENOENT;
> -
> - index = get_netdev_queue_index(queue);
> -
> - if (!rtnl_trylock())
> - return restart_syscall();
> + unsigned int nr_ids;
>
> if (dev->num_tc) {
> /* Do not allow XPS on subordinate device directly */
> num_tc = dev->num_tc;
> - if (num_tc < 0) {
> - ret = -EINVAL;
> - goto err_rtnl_unlock;
> - }
> + if (num_tc < 0)
> + return -EINVAL;
>
> /* If queue belongs to subordinate dev use its map */
> dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev;
>
> tc = netdev_txq_to_tc(dev, index);
> - if (tc < 0) {
> - ret = -EINVAL;
> - goto err_rtnl_unlock;
> - }
> + if (tc < 0)
> + return -EINVAL;
> }
>
> - mask = bitmap_zalloc(nr_cpu_ids, GFP_KERNEL);
> - if (!mask) {
> - ret = -ENOMEM;
> - goto err_rtnl_unlock;
> + rcu_read_lock();
> +
> + if (is_rxqs_map) {
> + dev_maps = rcu_dereference(dev->xps_rxqs_map);
> + nr_ids = dev->num_rx_queues;
> + } else {
> + dev_maps = rcu_dereference(dev->xps_cpus_map);
> + nr_ids = nr_cpu_ids;
> + if (num_possible_cpus() > 1)
> + possible_mask = cpumask_bits(cpu_possible_mask);
> }
I was good with what we had up until this point. THe issue is we are
allocating the nr_ids for the bitmap in one location, and then
populating it here.
In order to keep this consisten we would need to either hold the
rtnl_lock in the case of the number of Rx queues, or we would need to
call cpus_read_lock in order to prevent the number of CPUs from
changing. It may be better to look at encoding the number of IDs into
the map first, and then using that value. Otherwise we need to be
holding the appropriate lock or passing the number of IDs ourselves as
an argument.
Also we can just drop the possible_mask. No point in carrying it and
it will simplify the loop below as we shouldn't have added the CPU if
it wasn't possible to access it.
> + if (!dev_maps)
> + goto rcu_unlock;
>
> - rcu_read_lock();
> - dev_maps = rcu_dereference(dev->xps_cpus_map);
> - if (dev_maps) {
> - for_each_possible_cpu(cpu) {
> - int i, tci = cpu * num_tc + tc;
> - struct xps_map *map;
> -
> - map = rcu_dereference(dev_maps->attr_map[tci]);
> - if (!map)
> - continue;
> -
> - for (i = map->len; i--;) {
> - if (map->queues[i] == index) {
> - set_bit(cpu, mask);
> - break;
> - }
> + for (j = -1; j = netif_attrmask_next(j, possible_mask, nr_ids),
> + j < nr_ids;) {
I would drop the mask check and just work from 0 to nr_ids - 1.
> + int i, tci = j * num_tc + tc;
> + struct xps_map *map;
> +
> + map = rcu_dereference(dev_maps->attr_map[tci]);
> + if (!map)
> + continue;
> +
> + for (i = map->len; i--;) {
> + if (map->queues[i] == index) {
> + set_bit(j, *mask);
> + break;
> }
> }
> }
> +
> +rcu_unlock:
> rcu_read_unlock();
>
> + return ret;
> +}
> +
> +static ssize_t xps_cpus_show(struct netdev_queue *queue, char *buf)
> +{
> + struct net_device *dev = queue->dev;
> + unsigned long *mask;
> + unsigned int index;
> + int len, ret;
> +
> + if (!netif_is_multiqueue(dev))
> + return -ENOENT;
> +
> + index = get_netdev_queue_index(queue);
> +
> + mask = bitmap_zalloc(nr_cpu_ids, GFP_KERNEL);
The nr_cpu_ids could change from this read to the one in the
xps_queue_show function. We should either wrap this in a READ_ONCE and
use a local variable to track the nr_ids or we need to wrap this in
cpus_read_lock.
> + if (!mask)
> + return -ENOMEM;
> +
> + if (!rtnl_trylock()) {
> + bitmap_free(mask);
> + return restart_syscall();
> + }
> +
> + ret = xps_queue_show(dev, &mask, index, false);
> rtnl_unlock();
>
> + if (ret) {
> + bitmap_free(mask);
> + return ret;
> + }
> +
> len = bitmap_print_to_pagebuf(false, buf, mask, nr_cpu_ids);
> bitmap_free(mask);
> return len < PAGE_SIZE ? len : -EINVAL;
> -
> -err_rtnl_unlock:
> - rtnl_unlock();
> - return ret;
> }
>
> static ssize_t xps_cpus_store(struct netdev_queue *queue,
> @@ -1430,65 +1451,34 @@ static struct netdev_queue_attribute xps_cpus_attribute __ro_after_init
>
> static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
> {
> - int j, len, ret, num_tc = 1, tc = 0;
> struct net_device *dev = queue->dev;
> - struct xps_dev_maps *dev_maps;
> unsigned long *mask;
> unsigned int index;
> + int len, ret;
>
> index = get_netdev_queue_index(queue);
>
> - if (!rtnl_trylock())
> - return restart_syscall();
> -
> - if (dev->num_tc) {
> - num_tc = dev->num_tc;
> - tc = netdev_txq_to_tc(dev, index);
> - if (tc < 0) {
> - ret = -EINVAL;
> - goto err_rtnl_unlock;
> - }
> - }
> mask = bitmap_zalloc(dev->num_rx_queues, GFP_KERNEL);
The dev->num_rx_queues could change without the rtnl lock being held.
As such we should either use a READ_ONCE or we need to encapsulate the
entire thing in an rtnl_lock.
> - if (!mask) {
> - ret = -ENOMEM;
> - goto err_rtnl_unlock;
> - }
> -
> - rcu_read_lock();
> - dev_maps = rcu_dereference(dev->xps_rxqs_map);
> - if (!dev_maps)
> - goto out_no_maps;
> -
> - for (j = -1; j = netif_attrmask_next(j, NULL, dev->num_rx_queues),
> - j < dev->num_rx_queues;) {
> - int i, tci = j * num_tc + tc;
> - struct xps_map *map;
> -
> - map = rcu_dereference(dev_maps->attr_map[tci]);
> - if (!map)
> - continue;
> + if (!mask)
> + return -ENOMEM;
>
> - for (i = map->len; i--;) {
> - if (map->queues[i] == index) {
> - set_bit(j, mask);
> - break;
> - }
> - }
> + if (!rtnl_trylock()) {
> + bitmap_free(mask);
> + return restart_syscall();
> }
> -out_no_maps:
> - rcu_read_unlock();
>
> + ret = xps_queue_show(dev, &mask, index, true);
> rtnl_unlock();
>
> + if (ret) {
> + bitmap_free(mask);
> + return ret;
> + }
> +
> len = bitmap_print_to_pagebuf(false, buf, mask, dev->num_rx_queues);
> bitmap_free(mask);
>
> return len < PAGE_SIZE ? len : -EINVAL;
> -
> -err_rtnl_unlock:
> - rtnl_unlock();
> - return ret;
> }
>
> static ssize_t xps_rxqs_store(struct netdev_queue *queue, const char *buf,
> --
> 2.29.2
>
Powered by blists - more mailing lists