netdev - Re: [PATCH net-next] macvtap: add namespace support to the sysfs device class

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: <8737q933kv.fsf@x220.int.ebiederm.org>
Date:	Mon, 25 Apr 2016 14:12:32 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Marc Angel <marc@...sta.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] macvtap: add namespace support to the sysfs device class

Marc Angel <marc@...sta.com> writes:

> When creating macvtaps that are expected to have the same ifindex
> in different network namespaces, only the first one will succeed.
> The others will fail with a sysfs_warn_dup warning due to them trying
> to create the following sysfs link (with 'NN' the ifindex of macvtapX):
>
> /sys/class/macvtap/tapNN -> /sys/devices/virtual/net/macvtapX/tapNN
>
> This is reproducible by running the following commands:
>
> ip netns add ns1
> ip netns add ns2
> ip link add veth0 type veth peer name veth1
> ip link set veth0 netns ns1
> ip link set veth1 netns ns2
> ip netns exec ns1 ip l add link veth0 macvtap0 type macvtap
> ip netns exec ns2 ip l add link veth1 macvtap1 type macvtap
>
> The last command will fail with "RTNETLINK answers: File exists" (along
> with the kernel warning) but retrying it will work because the ifindex
> was incremented.

Yes.  That is totally broken, and you would not even care excpect that
the ifindex maintained separately per network namespace.  Useful for
migration but it totally breaks things in this case.

> The 'net' device class is isolated between network namespaces so each
> one has its own hierarchy of net devices.
> This isn't the case for the 'macvtap' device class.
> The problem occurs half-way through the netdev registration, when
> `macvtap_device_event` is called-back to create the 'tapNN' macvtap
> class device under the 'macvtapX' net class device.
>
> This patch adds namespace support the the 'macvtap' device class so
> that /sys/class/macvtap is no longer shared between net namespaces.
>
> However, doing this has the side effect of changing
> /sys/devices/virtual/net/macvtapX/tapNN  into
> /sys/devices/virtual/net/macvtapX/macvtap/tapNN

I forget the details of how this interface works, but
/sys/devices/virtual/net is definitely allows different overlapping
content per network namespace, so we should not need to add an extra
directory to make this work.

> This is due to Commit 24b1442 ("Driver-core: Always create class
> directories for classses that support namespaces.")
>
> Signed-off-by: Marc Angel <marc@...sta.com>
> ---
> I'm not sure that the problems described in that commit message
> apply to macvtaps so maybe it is possible to keep the 'tapNN'
> device directly under 'macvtapX' and not disrupt userland.
>
> Should it even be possible to add a device of a class that doesn't
> support namespaces under one that does?
> This could lead to dead symlinks in the new device class directory or
> duplicate warnings because a device of the same name already exists in
> another namespace.

This definitely looks like something that bears digging into, and fixing
properly.

Eric