netdev - Re: [patch net-next v2 15/16] netdevsim: move netdev creation/destruction to dev probe

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: <20190423100528.48908627@cakuba.netronome.com>
Date:   Tue, 23 Apr 2019 10:05:28 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, mlxsw@...lanox.com,
        dsahern@...il.com
Subject: Re: [patch net-next v2 15/16] netdevsim: move netdev
 creation/destruction to dev probe

On Tue, 23 Apr 2019 09:20:14 +0200, Jiri Pirko wrote:
> Mon, Apr 22, 2019 at 09:31:33PM CEST, jakub.kicinski@...ronome.com wrote:
> >On Sat, 20 Apr 2019 12:29:21 +0200, Jiri Pirko wrote:  
> >> diff --git a/tools/testing/selftests/bpf/test_offload.py b/tools/testing/selftests/bpf/test_offload.py
> >> index 5f2e4f9e70e4..a0566dcf064a 100755
> >> --- a/tools/testing/selftests/bpf/test_offload.py
> >> +++ b/tools/testing/selftests/bpf/test_offload.py
> >> @@ -1,6 +1,7 @@
> >>  #!/usr/bin/python3
> >>  
> >>  # Copyright (C) 2017 Netronome Systems, Inc.
> >> +# Copyright (c) 2019 Mellanox Technologies. All rights reserved  
> >
> >What's your guiding principle with adding those copyright lines
> >everywhere?  
> 
> Semi-random :) I can remove it if you want.

Technically I have no opinion, personally I find it petty and
unnecessary in the git era..  Probably not worth your time now 
to go and remove :)

> >> +class NetdevSim:
> >> +    """
> >> +    Class for netdevsim netdevice and its attributes.
> >> +    """
> >> +
> >> +    def __init__(self, nsimdev, port_index):
> >> +        self.nsimdev = nsimdev
> >> +        self.port_index = port_index
> >> +        self.ns = ""
> >> +        self.dfs_dir = "%s/ports/%u/" % (nsimdev.dfs_dir, port_index)
> >> +        self.dfs_refresh()
> >> +
> >> +        ifname = "eni%unp%u" % (nsimdev.addr, port_index + 1)
> >> +        timeout = 0.5
> >> +        timeout_start = time.time()
> >> +
> >> +        while True:
> >> +            try:
> >> +                _, [self.dev] = ip("link show dev %s" % ifname)
> >> +            except Exception as e:
> >> +                if time.time() < timeout_start + timeout:
> >> +                    continue
> >> +                raise e
> >> +            break  
> >
> >udevadm settle?  The reliance on latest systemd is a real bad idea,  
> 
> Not sure that "udevadm settle" would help here. The diver bus probe handle
> may be executed from a workqueue, so here, there might be the udev event
> queue empty yet still the netdev is not present.

Mm.. true.  I must say I had a ton of issues with this in Netronome
test systems, those "wait for renames" loop always broke no matter
what..  OS upgrade, adding debug stuff into the kernel..  I get
flashbacks.  I've seen udev easily take 20s to rename on a heavy kernel.
Since we replaced the loops with udevadm settle - it's been a bliss.  
YMMV but in my experience regardless of workqueues and all udev is way
more reliable.

> >could you fall back to netdevsimX names if eni$SOME$CUFT is not found?  
> 
> That would not help. If you have udev which does not support this, you
> are going to endup with "eth0, eth1, .."

That is not great :(  The kbuild bot runs BPF offload tests, they run
with some CentOS user space.  If we don't want to name the device in
the kernel, perhaps it'd be possible to scan sysfs?  The ifc names
should be under $device/net/, right?