netdev - Re: [PATCH net-next] macvlan: fix failure during registration v2

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] [day] [month] [year] [list]

Message-ID: <8737qd3ukp.fsf@x220.int.ebiederm.org>
Date:	Fri, 22 Apr 2016 21:52:38 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Francesco Ruggeri <fruggeri@...sta.com>
Cc:	netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Mahesh Bandewar <maheshb@...gle.com>
Subject: Re: [PATCH net-next] macvlan: fix failure during registration v2

Francesco Ruggeri <fruggeri@...sta.com> writes:

> On Thu, Apr 21, 2016 at 10:44 AM, Eric W. Biederman
> <ebiederm@...ssion.com> wrote:
> <
>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>> index 95394ed..e770221 100644
>>> --- a/drivers/net/macvtap.c
>>> +++ b/drivers/net/macvtap.c
>>> @@ -1303,6 +1303,8 @@ static int macvtap_device_event(struct notifier_block *unused,
>>>               }
>>>               break;
>>>       case NETDEV_UNREGISTER:
>>> +             if (vlan->minor == 0)
>>> +                     break;
>>
>> I don't understand this bit.  A minor of 0 is never assigned.  That is
>> clear from the code.  On what code path can you get here without
>> assigning a minor?
>>
>
> You can have vlan->minor == 0 if macvtap_device_event(NETDEV_REGISTER)
> failed.
>
> macvtap_device_event is invoked in the context of macvtap_newlink and
> it can fail if for example a macvtap interface using the same ifindex
> already exists in a different namespace. That is how we originally ran
> into the port->count issue.
> In that case the sequence is
>
> macvtap_newlink
>   macvlan_common_newlink
>     register_netdevice
>       call_netdevice_notifiers(NETDEV_REGISTER, dev)
>         macvtap_device_event(NETDEV_REGISTER)
>           <fail here, vlan->minor = 0>
>       rollback_registered(dev);
>         rollback_registered_many
>           call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
>             macvtap_device_event(NETDEV_UNREGISTER)
>               <nothing to do here>
>
> Should this bit go into a separate patch?
> Would a comment like this help:
>
> /* We can have vlan->minor == 0 if NETDEV_REGISTER above failed */
>
> Marc Angel posted https://marc.info/?l=linux-netdev&m=146116146925511&w=2
> about conflicts if one tries to create macvtap interfaces with the same
> ifindex in different namespaces.

Just for clarity I would recommend splitting the two changes.

One change that fixes macvlan_init to do the right thing.

Another change that includes your backtrace above, adds the comment
and fixes macvlan to ignore that case.  Arguably we should be fixing
register_netdevice to call call_netdeivce_notifiers(NETDEV_UNREIGSTER)
if call_netdevice_notifiers(NETDEV_REGISTER) failed.  Having a separate
patch will at least allow us to look at this second issue all by itself.

Thank you for your patience in all of this.

Eric