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

[ebiederm@xmission.com (Eric W. Biederman)]

Francesco Ruggeri <fruggeri@arista.com> writes:

> On Thu, Apr 21, 2016 at 10:44 AM, Eric W. Biederman
> <ebiederm@xmission.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