netdev - Re: [PATCH net 3/3] net-sysfs: move the xps cpus/rxqs retrieval in a common function

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: <161000966161.3275.12891261917424414122@kwain.local>
Date:   Thu, 07 Jan 2021 09:54:21 +0100
From:   Antoine Tenart <atenart@...nel.org>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net 3/3] net-sysfs: move the xps cpus/rxqs retrieval in a common function

Quoting Alexander Duyck (2021-01-06 20:54:11)
> On Wed, Jan 6, 2021 at 10:04 AM Antoine Tenart <atenart@...nel.org> wrote:
> > +/* 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)
> 
> Why pass dev and index instead of just the queue which already
> contains both?

Right, I can do that.

> I think it would make more sense to just stick to passing the queue
> through along with a pointer to the xps_dev_maps value that we need to
> read.

That would require to hold rcu_read_lock in the caller and I'd like to
keep it in that function.

> >         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;
> >         }
> >
> 
> So if we store the num_tc and nr_ids in the dev_maps structure then we
> could simplify this a bit by pulling the num_tc info out of the
> dev_map and only asking the Tx queue for the tc in that case and
> validating it against (tc <0 || num_tc <= tc) and returning an error
> if either are true.
> 
> This would also allow us to address the fact that the rxqs feature
> doesn't support the subordinate devices as you could pull out the bit
> above related to the sb_dev and instead call that prior to calling
> xps_queue_show so that you are operating on the correct device map.
> 
> > -       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 think Jakub had mentioned earlier the idea of possibly moving some
> fields into the xps_cpus_map and xps_rxqs_map in order to reduce the
> complexity of this so that certain values would be protected by the
> RCU lock.
> 
> This might be a good time to look at encoding things like the number
> of IDs and the number of TCs there in order to avoid a bunch of this
> duplication. Then you could just pass a pointer to the map you want to
> display and the code should be able to just dump the values.:

100% agree to all the above. That would also prevent from making out of
bound accesses when dev->num_tc is increased after dev_maps is
allocated. I do have a series ready to be send storing num_tc into the
maps, and reworking code to use it instead of dev->num_tc. The series
also adds checks to ensure the map is valid when we access it (such as
making sure dev->num_tc == map->num_tc). I however did not move nr_ids
into the map yet, but I'll look into it.

The idea is to send it as a follow up series, as this one is only moving
code around to improve maintenance and readability. Even if all the
patches were in the same series that would be a prerequisite.

Thanks!
Antoine