netdev - Re: [PATCH net] net-sysfs: update the queue counts in the unregistration path

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: <20211201070513.6830f1d4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 1 Dec 2021 07:05:13 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Antoine Tenart <atenart@...nel.org>
Cc:     davem@...emloft.net, alexander.duyck@...il.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net] net-sysfs: update the queue counts in the
 unregistration path

On Wed, 01 Dec 2021 10:32:01 +0100 Antoine Tenart wrote:
> > Would you mind pointing where in the code that happens? I can't seem 
> > to find anything looking at real_num_.x_queues outside dev.c and
> > net-sysfs.c :S  
> 
> I read the above commit message again; it's not well explained... Sorry
> for that.
> 
> The above trace was triggered using veths and this patch would solve
> this as veths do use real_num_x_queues to fill 'struct ethtool_channels'
> in its get_channels ops[1] which is then used to avoid making channel
> counts updates if it is 0[2].

But when we are at line 175 in [2] we already updated the values from
the user space request at lines 144-151. This check validates the new
config so a transition from 0 -> n should not be prevented here AFAICT.

> In addition, keeping track of the queue counts in the unregistration
> path do help other drivers as it will allow adding a warning in
> netdev_queue_update_kobjects when adding queues after unregister
> (without tracking the queue counts in the unregistration path we can't
> detect illegal queue additions). We could also not only warn for illegal
> uses of netdev_queue_update_kobjects but also return an error.
> 
> Another change that was discussed is to forbid ethtool ops after
> unregister. This is good, but is outside of the queue code so it might
> not solve all issues.
> 
> (I do have those two patches, warn + ethtool, in my local tree and plan
> on targeting net-next).
> 
> As you can see I'm a bit puzzled at how to fix this in the best way
> possible[3]. I think the combination of the three patches should be good
> enough, with only one sent to net as it does fix veths which IMHO is
> easier to trigger. WDYT?
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/net/veth.c#L222
> [2] https://elixir.bootlin.com/linux/latest/source/net/ethtool/channels.c#L175
> [3] Because the queue code does rely on external states.

Any way of fixing this is fine. If you ask me personally I'd probably
go with the ethtool fix to net and the zeroing and warn to net-next.
Unless I'm misreading and this fix does work, in which case your plan
is good, too.