netdev - Re: [patch net-next rfc 00/15] netdevsim: impement proper device model

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]

Date:   Thu, 18 Apr 2019 09:22:56 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, mlxsw@...lanox.com
Subject: Re: [patch net-next rfc 00/15] netdevsim: impement proper device
 model

Tue, Apr 16, 2019 at 08:04:59PM CEST, jakub.kicinski@...ronome.com wrote:
>On Tue, 16 Apr 2019 10:59:37 +0200, Jiri Pirko wrote:
>> >> 4) netdevsim instances are created by "ip link add" which is great for
>> >>    soft devices with no hw backend. The rtnl core allocates netdev and
>> >>    calls into driver holding rtnl mutex. For hw-backed devices, this
>> >>    flow is wrong as it breaks order in which things are done.
>> >> 
>> >> This patchset adjust netdevsim to fix all above.
>> >> 
>> >> In order to support proper devlink and devlink port instances and to be
>> >> able to emulate real devices, there is need to implement bus probe and
>> >> instantiate everything from there. User can specify device id and port
>> >> count to be instantianted. For example:
>> >> 
>> >> echo "10 4" > /sys/bus/netdevsim/new_device  
>> >
>> >I really don't like the design where ID has to be allocated by user
>> >space.  It's a step back.
>> >
>> >I also dislike declaring ports from the start.  In real drivers ports
>> >are never "atomically" registered, they are crated and destroyed one   
>> 
>> Care to define "atomically" here? It is done in a very similar way
>> to how it is done in mlxsw for example. Same flows.
>> 
>> 
>> >by one, and a lot of races/UAFs/bugs lie in those small periods of
>> >time where one netdev got unregistered, but other are still around...  
>> 
>> Same here. Not sure where do you see the differences.
>
>The difference is that today I can do this:
>
>create a netdevsim1 with shared dev 1
>create some state associated with shared dev 1
>create a netdevsim2 with shared dev 1
>check if all the shared dev 1 state created for netdevsim1 is visible
>	via netdevsim2

Hmm, you are testing netdevsim implementation then, not the kernel
interfaces. What is the point of testing netdevsim?


>destroy netdevsim1
>check the shared dev 1 state again
>
>If I say "give me 2 ports" from the start, that makes the testing
>(which is the whole point of this code) harder.
>
>> Also, I plan to implement port splitting in follow-up patchset. All
>> flows are there as well.
>
>Sure, let's just be clear that we won't be merging an ABI that has just
>a netdevsim implementation, right?  I have some reservations about the

So what do you suggest? Allow to somehow add and remove ports during
test? You can already do that with VFs. Do you want to do that with
netdevsim "physical" ports? If yes, how? I can imagine to extend devlink
port api with something like:

$ sudo devlink dev
netdevsim/netdevsim0
$ sudo devlink port
netdevsim/netdevsim0/0: type eth netdev eth0 flavour physical

$ sudo devlink dev port add netdevsim/netdevsim0 index 22
$ sudo devlink port
netdevsim/netdevsim0/0: type eth netdev eni0p1 flavour physical
netdevsim/netdevsim0/22: type eth netdev eni0p23 flavour physical

$ sudo devlink port del netdevsim/netdevsim0/0
$ sudo devlink port
netdevsim/netdevsim0/22: type eth netdev eni0p23 flavour physical

But I see only usecase for this extension for netdevsim, not for real
devices..


>"port splitting" or device slicing, which should be discussed over real
>code, not netdevsim.