[PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[anuradhak]

From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>

Applications can detect errors in the network that would require
disabling the device independent of the admin state. In the presence of
these errors traffic could be black holed or looped resulting in a
network meltdown. Clearing the IFF_UP flag for error disabling the
device can be problematic because -

1. The administrator cannot distinguish between a user space daemon’s
error-disable and a regular device disable.
2. Applications can monitor the error state and enable the device once
the error is removed. If IFF_UP is used for this purpose the application
may end up enabling a device that the administrator has intentionally
disabled for other reasons. This could result in network changes not
expected by the admin.

To avoid these problems this patch adds a distinct IFF_PROTO_DOWN flag
for error disabling a device.

This patch introduces a netdevice proto_down field to allow multiple
applications to disable a device independent of each other. This field
is a bitmap with two defined protocols currently, MLAG and STP. Bits
can be added in the future to define other protocols that may need to
disable the device. If any of the bits in the proto_down field are set
an oper DOWN is done on the device by setting IFF_PROTO_DOWN.

IFF_PROTO_DOWN is a netdevice flag that is used to control the oper
state and also used for notifying drivers that a protocol has disabled
the device. Switch drivers could use the IFF_PROTO_DOWN flag to further
handle the error condition; for e.g. they could carrier down the device
allowing directly connected switches to quickly learn about the error
state and stop forwarding traffic to this device.

STP applications can use the proto_down control to implement BPDU guard
functionality which requires shutting down access ports on detecting
rogue switches that could take over as the root bridge.

MLAG applications can use the proto_down control to hold devices down
on the secondary switch on detecting a split-brain situation between the
MLAG-peers.

Anuradha Karuppiah (3):
  net core: Add support for netdevice proto_down.
  virtio net: Handle proto_down state by setting the carrier off.
  ip link: Config and display device proto_down bits.

Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
Signed-off-by: Wilson Kok <wkok@cumulusnetworks.com>

 drivers/net/virtio_net.c     |   78 +++++++++++++++++++++++++++++++++++++++---
 include/linux/netdevice.h    |    3 ++
 include/uapi/linux/if.h      |   29 +++++++++++++++-
 include/uapi/linux/if_link.h |    9 +++++
 net/8021q/vlan_dev.c         |    3 +-
 net/core/dev.c               |   36 +++++++++++++++++++
 net/core/link_watch.c        |    2 +-
 net/core/net-sysfs.c         |    2 ++
 net/core/rtnetlink.c         |   21 ++++++++++++
 9 files changed, 176 insertions(+), 7 deletions(-)

-- 
1.7.10.4

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[Alexei Starovoitov]

On Fri, Mar 20, 2015 at 8:11 AM,  <anuradhak@cumulusnetworks.com> wrote:
> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>
> Applications can detect errors in the network that would require
> disabling the device independent of the admin state. In the presence of
> these errors traffic could be black holed or looped resulting in a
> network meltdown. Clearing the IFF_UP flag for error disabling the
> device can be problematic because -
>
> 1. The administrator cannot distinguish between a user space daemon’s
> error-disable and a regular device disable.
> 2. Applications can monitor the error state and enable the device once
> the error is removed. If IFF_UP is used for this purpose the application
> may end up enabling a device that the administrator has intentionally
> disabled for other reasons. This could result in network changes not
> expected by the admin.
>

Both reasons look like workaround for user space issues.
Just keep this fake-down state in userspace.
What's the point pushing it to kernel?
looking at 3rd patch:
+ * @IF_LINK_PROTO_DOWN_MLAG: proto_down by a multi-chassis LAG application.
+ * @IF_LINK_PROTO_DOWN_STP: proto_down by an STP application.

so there will be new flag for every application that cannot deal with
normal down?

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[Anuradha Karuppiah]

On Fri, Mar 20, 2015 at 9:13 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Fri, Mar 20, 2015 at 8:11 AM,  <anuradhak@cumulusnetworks.com> wrote:
>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>>
>> Applications can detect errors in the network that would require
>> disabling the device independent of the admin state. In the presence of
>> these errors traffic could be black holed or looped resulting in a
>> network meltdown. Clearing the IFF_UP flag for error disabling the
>> device can be problematic because -
>>
>> 1. The administrator cannot distinguish between a user space daemon’s
>> error-disable and a regular device disable.
>> 2. Applications can monitor the error state and enable the device once
>> the error is removed. If IFF_UP is used for this purpose the application
>> may end up enabling a device that the administrator has intentionally
>> disabled for other reasons. This could result in network changes not
>> expected by the admin.
>>
>
> Both reasons look like workaround for user space issues.
> Just keep this fake-down state in userspace.
> What's the point pushing it to kernel?

Applications can deal with IFF_UP being cleared and they can certainly
clear IFF_UP as well on detecting errors. However an application
cannot know the reason for the !IFF_UP notification. So if an
application detected a device error being cleared it would have to
unconditionally enable the device as a part of recovery handling
thereby ignoring the administrator’s request to keep the device
disabled. Separating error-disable (IFF_PROTO_DOWN) from admin-disable
(!IFF_UP) lets the administrator have a say in keeping a device
disabled.

> looking at 3rd patch:
> + * @IF_LINK_PROTO_DOWN_MLAG: proto_down by a multi-chassis LAG application.
> + * @IF_LINK_PROTO_DOWN_STP: proto_down by an STP application.
>
> so there will be new flag for every application that cannot deal with
> normal down?

These applications can clear the error state independent of each
other. Say for e.g.  both STP-BPDU guard and MLAG error-disabled a
device. When the MLAG split-brain error is resolved the MLAG
application could clear IFF_PROTO_DOWN but the BPDU guard error would
still exist. This will create problem windows that could aggressively
affect the network.

New bits only need to be added if there are new errors that need to be
cleared independent of other applications that can error-disable a
device.

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[David Miller]

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Fri, 20 Mar 2015 09:13:56 -0700

> Both reasons look like workaround for user space issues.
> Just keep this fake-down state in userspace.

+1

> so there will be new flag for every application that cannot deal with
> normal down?

+1 +1 +1

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[David Miller]

From: anuradhak@cumulusnetworks.com
Date: Fri, 20 Mar 2015 08:11:55 -0700

> Applications can detect errors in the network that would require
> disabling the device independent of the admin state.

I hate changes like this.

The only reason it exists, is because you don't want to put together
the infrastructure and framework necessary for applications managing
this state in userspace to _work_ _together_.  So you make it a kernel
problem.

UP/DOWN is a boolean state, that's not changing.

So you need to design your stuff such that one entity takes all of the
collective decisions together and calculates the final up/down state,
and then asks the kernel to do that.

Nothing is more bullshit than having someone ask the kernel to up the
interface and the thing doesn't come up because of this auxiliary
crap.  That's broken and nobody is going to expect nor be happy with
that behavior.

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[Anuradha Karuppiah]

On Fri, Mar 20, 2015 at 1:28 PM, David Miller <davem@davemloft.net> wrote:
> From: anuradhak@cumulusnetworks.com
> Date: Fri, 20 Mar 2015 08:11:55 -0700
>
>> Applications can detect errors in the network that would require
>> disabling the device independent of the admin state.
>
> I hate changes like this.
>
> The only reason it exists, is because you don't want to put together
> the infrastructure and framework necessary for applications managing
> this state in userspace to _work_ _together_.  So you make it a kernel
> problem.
>
> UP/DOWN is a boolean state, that's not changing.
>
> So you need to design your stuff such that one entity takes all of the
> collective decisions together and calculates the final up/down state,
> and then asks the kernel to do that.
>
> Nothing is more bullshit than having someone ask the kernel to up the
> interface and the thing doesn't come up because of this auxiliary
> crap.  That's broken and nobody is going to expect nor be happy with
> that behavior.

I understand your position on this. And I agree .... ..however, in
this particular case ....

Unfortunately there is no way to distinguish between admin state and
error state in the user space. Currently administrators are directly
using “ip link set up/down”/IFF_UP as a means to admin-enable/disable
a device. So even if we were to consolidate all the errors in the user
space there is no way to error-disable/enable a device without
overriding the administrator’s directive.

What we are looking for is a way for user space to hold the device
down on detecting incorrect config/topology and for enabling the
device once the error condition is removed provided the administrator
didn’t intentionally disable it. And we don't see a way for a user
protocol to do this without involving the kernel.

Thanks for the review.

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[David Miller]

From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
Date: Fri, 20 Mar 2015 14:16:26 -0700

> What we are looking for is a way for user space to hold the device
> down on detecting incorrect config/topology and for enabling the
> device once the error condition is removed provided the administrator
> didn’t intentionally disable it. And we don't see a way for a user
> protocol to do this without involving the kernel.

It is not your business to override what the administrator asks for.

This is what I mean by "cooperative".

ip link up has no idea about your error states, neither does every
administrator out there.

So you're changing behavior in an incompatible way.

This is why you need a wholistic approach in userland that propagates
this knowledge all around, rather than just using a sledge hammer by
asking the kernel to enforce some new rule upon unsuspecting existing
users.

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[Anuradha Karuppiah]

On Fri, Mar 20, 2015 at 3:15 PM, David Miller <davem@davemloft.net> wrote:
> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
> Date: Fri, 20 Mar 2015 14:16:26 -0700
>
>> What we are looking for is a way for user space to hold the device
>> down on detecting incorrect config/topology and for enabling the
>> device once the error condition is removed provided the administrator
>> didn’t intentionally disable it. And we don't see a way for a user
>> protocol to do this without involving the kernel.
>
> It is not your business to override what the administrator asks for.
>
> This is what I mean by "cooperative".
>
> ip link up has no idea about your error states, neither does every
> administrator out there.
>
> So you're changing behavior in an incompatible way.
>
> This is why you need a wholistic approach in userland that propagates
> this knowledge all around, rather than just using a sledge hammer by
> asking the kernel to enforce some new rule upon unsuspecting existing
> users.

I agree with you, applications cannot/must not override the
administrator’s directive. I also agree that “ip link set up” doesn’t
need to know about errors that user daemons detect.

That is one of the reasons for not clearing IFF_UP from the user space
MLAG application on detecting errors. But I need a way to notify the
device drivers (of the devices that have MLAG enabled) that an error
has occurred allowing them to handle the error gracefully i.e. without
traffic getting black holed.

The change in the net core is only to prevent the oper state from
going UP when errors are encountered not to dev_close (Sorry, I think
I caused this confusion by saying error “disable”). This in some way
is similar to the dormant mode/state implementation which lets the
user space application/driver influence the operstate. Only additional
thing here is a notification that let’s drivers know that errors have
been encountered on the device.

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[Alexei Starovoitov]

On Fri, Mar 20, 2015 at 9:45 AM, Anuradha Karuppiah
<anuradhak@cumulusnetworks.com> wrote:
> On Fri, Mar 20, 2015 at 9:13 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Fri, Mar 20, 2015 at 8:11 AM,  <anuradhak@cumulusnetworks.com> wrote:
>>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>>>
>>> Applications can detect errors in the network that would require
>>> disabling the device independent of the admin state. In the presence of
>>> these errors traffic could be black holed or looped resulting in a
>>> network meltdown. Clearing the IFF_UP flag for error disabling the
>>> device can be problematic because -
>>>
>>> 1. The administrator cannot distinguish between a user space daemon’s
>>> error-disable and a regular device disable.
>>> 2. Applications can monitor the error state and enable the device once
>>> the error is removed. If IFF_UP is used for this purpose the application
>>> may end up enabling a device that the administrator has intentionally
>>> disabled for other reasons. This could result in network changes not
>>> expected by the admin.
>>>
>>
>> Both reasons look like workaround for user space issues.
>> Just keep this fake-down state in userspace.
>> What's the point pushing it to kernel?
>
> Applications can deal with IFF_UP being cleared and they can certainly
> clear IFF_UP as well on detecting errors. However an application
> cannot know the reason for the !IFF_UP notification. So if an
> application detected a device error being cleared it would have to
> unconditionally enable the device as a part of recovery handling
> thereby ignoring the administrator’s request to keep the device
> disabled. Separating error-disable (IFF_PROTO_DOWN) from admin-disable
> (!IFF_UP) lets the administrator have a say in keeping a device
> disabled.
>
>> looking at 3rd patch:
>> + * @IF_LINK_PROTO_DOWN_MLAG: proto_down by a multi-chassis LAG application.
>> + * @IF_LINK_PROTO_DOWN_STP: proto_down by an STP application.
>>
>> so there will be new flag for every application that cannot deal with
>> normal down?
>
> These applications can clear the error state independent of each
> other. Say for e.g.  both STP-BPDU guard and MLAG error-disabled a
> device. When the MLAG split-brain error is resolved the MLAG
> application could clear IFF_PROTO_DOWN but the BPDU guard error would
> still exist. This will create problem windows that could aggressively
> affect the network.
>

if I understand this correctly you have implementation of
stp-bpdu guard in user space instead of bridge stp core and
that is causing these issues. If you move this feature into
the kernel you won't have to add this special down state, right?

Re: [PATCH net-next 0/3] net: introduce IFF_PROTO_DOWN flag.

[Anuradha Karuppiah]

On Fri, Mar 20, 2015 at 11:50 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Fri, Mar 20, 2015 at 9:45 AM, Anuradha Karuppiah
> <anuradhak@cumulusnetworks.com> wrote:
>> On Fri, Mar 20, 2015 at 9:13 AM, Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>> On Fri, Mar 20, 2015 at 8:11 AM,  <anuradhak@cumulusnetworks.com> wrote:
>>>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>>>>
>>>> Applications can detect errors in the network that would require
>>>> disabling the device independent of the admin state. In the presence of
>>>> these errors traffic could be black holed or looped resulting in a
>>>> network meltdown. Clearing the IFF_UP flag for error disabling the
>>>> device can be problematic because -
>>>>
>>>> 1. The administrator cannot distinguish between a user space daemon’s
>>>> error-disable and a regular device disable.
>>>> 2. Applications can monitor the error state and enable the device once
>>>> the error is removed. If IFF_UP is used for this purpose the application
>>>> may end up enabling a device that the administrator has intentionally
>>>> disabled for other reasons. This could result in network changes not
>>>> expected by the admin.
>>>>
>>>
>>> Both reasons look like workaround for user space issues.
>>> Just keep this fake-down state in userspace.
>>> What's the point pushing it to kernel?
>>
>> Applications can deal with IFF_UP being cleared and they can certainly
>> clear IFF_UP as well on detecting errors. However an application
>> cannot know the reason for the !IFF_UP notification. So if an
>> application detected a device error being cleared it would have to
>> unconditionally enable the device as a part of recovery handling
>> thereby ignoring the administrator’s request to keep the device
>> disabled. Separating error-disable (IFF_PROTO_DOWN) from admin-disable
>> (!IFF_UP) lets the administrator have a say in keeping a device
>> disabled.
>>
>>> looking at 3rd patch:
>>> + * @IF_LINK_PROTO_DOWN_MLAG: proto_down by a multi-chassis LAG application.
>>> + * @IF_LINK_PROTO_DOWN_STP: proto_down by an STP application.
>>>
>>> so there will be new flag for every application that cannot deal with
>>> normal down?
>>
>> These applications can clear the error state independent of each
>> other. Say for e.g.  both STP-BPDU guard and MLAG error-disabled a
>> device. When the MLAG split-brain error is resolved the MLAG
>> application could clear IFF_PROTO_DOWN but the BPDU guard error would
>> still exist. This will create problem windows that could aggressively
>> affect the network.
>>
>
> if I understand this correctly you have implementation of
> stp-bpdu guard in user space instead of bridge stp core and
> that is causing these issues. If you move this feature into
> the kernel you won't have to add this special down state, right?

IFF_PROTO_DOWN is needed to distinguish between admin and error
disable states. Even if a kernel driver was setting or clearing the
error-disable state via dev_close/open it could still end up
overriding the administrator’s need to keep a device DOWN as a part of
its error recovery handling. To avoid that problem the kernel STP
BPDU-guard could also use dev_set_proto_down(…,IF_LINK_PROTO_DOWN_STP)
to disable misbehaving access ports.