[RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[anuradhak]

From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>

This patch introduces an IFF_PROTO_DOWN flag that can be used by
user space applications to notify drivers that errors have been
detected on the device.

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>
---
 include/uapi/linux/if.h |    4 ++++
 net/8021q/vlan_dev.c    |    3 ++-
 net/core/dev.c          |    2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index 9cf2394..e263bd2 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -66,6 +66,8 @@
  * @IFF_LOWER_UP: driver signals L1 up. Volatile.
  * @IFF_DORMANT: driver signals dormant. Volatile.
  * @IFF_ECHO: echo sent packets. Volatile.
+ * @IFF_PROTO_DOWN: protocol is down on the interface. Can be toggled
+ *	through sysfs.
  */
 enum net_device_flags {
 	IFF_UP				= 1<<0,  /* sysfs */
@@ -87,6 +89,7 @@ enum net_device_flags {
 	IFF_LOWER_UP			= 1<<16, /* volatile */
 	IFF_DORMANT			= 1<<17, /* volatile */
 	IFF_ECHO			= 1<<18, /* volatile */
+	IFF_PROTO_DOWN			= 1<<19, /* sysfs */
 };
 
 #define IFF_UP				IFF_UP
@@ -108,6 +111,7 @@ enum net_device_flags {
 #define IFF_LOWER_UP			IFF_LOWER_UP
 #define IFF_DORMANT			IFF_DORMANT
 #define IFF_ECHO			IFF_ECHO
+#define IFF_PROTO_DOWN			IFF_PROTO_DOWN
 
 #define IFF_VOLATILE	(IFF_LOOPBACK|IFF_POINTOPOINT|IFF_BROADCAST|IFF_ECHO|\
 		IFF_MASTER|IFF_SLAVE|IFF_RUNNING|IFF_LOWER_UP|IFF_DORMANT)
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 01d7ba8..7073ee1 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -537,7 +537,8 @@ static int vlan_dev_init(struct net_device *dev)
 
 	/* IFF_BROADCAST|IFF_MULTICAST; ??? */
 	dev->flags  = real_dev->flags & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI |
-					  IFF_MASTER | IFF_SLAVE);
+					  IFF_MASTER | IFF_SLAVE |
+					  IFF_PROTO_DOWN);
 	dev->state  = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) |
 					  (1<<__LINK_STATE_DORMANT))) |
 		      (1<<__LINK_STATE_PRESENT);
diff --git a/net/core/dev.c b/net/core/dev.c
index af4a1b0..e9600fa 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5725,7 +5725,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags)
 
 	dev->flags = (flags & (IFF_DEBUG | IFF_NOTRAILERS | IFF_NOARP |
 			       IFF_DYNAMIC | IFF_MULTICAST | IFF_PORTSEL |
-			       IFF_AUTOMEDIA)) |
+			       IFF_AUTOMEDIA | IFF_PROTO_DOWN)) |
 		     (dev->flags & (IFF_UP | IFF_VOLATILE | IFF_PROMISC |
 				    IFF_ALLMULTI));
 
-- 
1.7.10.4

Re: [RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[Scott Feldman]

On Mon, Apr 27, 2015 at 10:38 AM,  <anuradhak@cumulusnetworks.com> wrote:
> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>
> This patch introduces an IFF_PROTO_DOWN flag that can be used by
> user space applications to notify drivers that errors have been
> detected on the device.
>
> 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>
> ---
>  include/uapi/linux/if.h |    4 ++++
>  net/8021q/vlan_dev.c    |    3 ++-
>  net/core/dev.c          |    2 +-
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
> index 9cf2394..e263bd2 100644
> --- a/include/uapi/linux/if.h
> +++ b/include/uapi/linux/if.h
> @@ -66,6 +66,8 @@
>   * @IFF_LOWER_UP: driver signals L1 up. Volatile.
>   * @IFF_DORMANT: driver signals dormant. Volatile.
>   * @IFF_ECHO: echo sent packets. Volatile.
> + * @IFF_PROTO_DOWN: protocol is down on the interface. Can be toggled
> + *     through sysfs.

This comment is stale, right?  I didn't see any sysfs support in the patchset.

Re: [RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[Anuradha Karuppiah]

On Mon, Apr 27, 2015 at 11:05 PM, Scott Feldman <sfeldma@gmail.com> wrote:
> On Mon, Apr 27, 2015 at 10:38 AM,  <anuradhak@cumulusnetworks.com> wrote:
>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>>
>> This patch introduces an IFF_PROTO_DOWN flag that can be used by
>> user space applications to notify drivers that errors have been
>> detected on the device.
>>
>> 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>
>> ---
>>  include/uapi/linux/if.h |    4 ++++
>>  net/8021q/vlan_dev.c    |    3 ++-
>>  net/core/dev.c          |    2 +-
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
>> index 9cf2394..e263bd2 100644
>> --- a/include/uapi/linux/if.h
>> +++ b/include/uapi/linux/if.h
>> @@ -66,6 +66,8 @@
>>   * @IFF_LOWER_UP: driver signals L1 up. Volatile.
>>   * @IFF_DORMANT: driver signals dormant. Volatile.
>>   * @IFF_ECHO: echo sent packets. Volatile.
>> + * @IFF_PROTO_DOWN: protocol is down on the interface. Can be toggled
>> + *     through sysfs.
>
> This comment is stale, right?  I didn't see any sysfs support in the patchset.

Correct, No new sysfs has been added for this flag. But IFF_PROTO_DOWN
can be toggled via the already existing generic flags sysfs (similar
to say IFF_MULTICAST); hence the comment –

root@net-next:~# echo 0x81003 > /sys/class/net/eth0/flags

root@net-next:~# cat /sys/class/net/eth0/flags

0x81003

Re: [RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[Stephen Hemminger]

On Mon, 27 Apr 2015 10:38:21 -0700
anuradhak@cumulusnetworks.com wrote:

> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
> 
> This patch introduces an IFF_PROTO_DOWN flag that can be used by
> user space applications to notify drivers that errors have been
> detected on the device.
> 
> 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>

I worry that adding another bit to an already complex state API
will break userspace.

There are lots of things besides iproute2 which look at those
flags including routing daemons (quagga), network manager, netplugd,
and switch controllers.

Re: [RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[Anuradha Karuppiah]

On Wed, Apr 29, 2015 at 3:13 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Mon, 27 Apr 2015 10:38:21 -0700
> anuradhak@cumulusnetworks.com wrote:
>
>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>>
>> This patch introduces an IFF_PROTO_DOWN flag that can be used by
>> user space applications to notify drivers that errors have been
>> detected on the device.
>>
>> 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>
>
> I worry that adding another bit to an already complex state API
> will break userspace.
>
> There are lots of things besides iproute2 which look at those
> flags including routing daemons (quagga), network manager, netplugd,
> and switch controllers.

Yes, I understand your concerns here. And tried to work around introducing
a separate error flag by clearing IFF_UP on proto_down/detecting errors (as
Scott also brought up earlier).

That implementation failed because of the following reasons -
1. There is no way to disambiguate between admin_down (!IFF_UP) and an
APP/driver enforced error_down (IFF_PROTO_DOWN). Administrator or
automation-scripts that monitor the config assumed that switch-port
configuration had somehow fallen out of sync (and attempted to reinstate the
admin_up repeatedly).

2. Automatic error recovery was not possible; consider the following scenario
for e.g.
   a. The MLAG peer-link is down so the MLAG app on the secondary switch has
      proto_down’ed all the MLAG ports (including switch-port swp1) by clearing
      IFF_UP.
   b. At the same time the administrator is in the process of making some
      changes on the network connected to swp1. To avoid doing it live he would
      admin_disable swp1 (!IFF_UP) by doing an "ip link set swp1 down" (this
      is a no-op as event #a has already cleared IFF_UP on swp1).
   c. If the MLAG peer-link recovers at this point the MLAG app on the
      secondary switch would try to automatically recover the MLAG ports
      by clearing proto_down (i.e. setting IFF_UP); including on swp1. Doing
      that overrides the administrator’s directive to keep swp1 admin_down.
      Overriding an admin-down in a live network can be very dangerous so it
      is not possible to do auto-error-recovery unless we have a way to
      disambiguate between the admin and error states.

Re: [RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[Anuradha Karuppiah]

On Wed, Apr 29, 2015 at 4:07 PM, Anuradha Karuppiah
<anuradhak@cumulusnetworks.com> wrote:
> On Wed, Apr 29, 2015 at 3:13 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>> On Mon, 27 Apr 2015 10:38:21 -0700
>> anuradhak@cumulusnetworks.com wrote:
>>
>>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>>>
>>> This patch introduces an IFF_PROTO_DOWN flag that can be used by
>>> user space applications to notify drivers that errors have been
>>> detected on the device.
>>>
>>> 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>
>>
>> I worry that adding another bit to an already complex state API
>> will break userspace.
>>
>> There are lots of things besides iproute2 which look at those
>> flags including routing daemons (quagga), network manager, netplugd,
>> and switch controllers.
>
> Yes, I understand your concerns here. And tried to work around introducing
> a separate error flag by clearing IFF_UP on proto_down/detecting errors (as
> Scott also brought up earlier).
>
> That implementation failed because of the following reasons -
> 1. There is no way to disambiguate between admin_down (!IFF_UP) and an
> APP/driver enforced error_down (IFF_PROTO_DOWN). Administrator or
> automation-scripts that monitor the config assumed that switch-port
> configuration had somehow fallen out of sync (and attempted to reinstate the
> admin_up repeatedly).
>
> 2. Automatic error recovery was not possible; consider the following scenario
> for e.g.
>    a. The MLAG peer-link is down so the MLAG app on the secondary switch has
>       proto_down’ed all the MLAG ports (including switch-port swp1) by clearing
>       IFF_UP.
>    b. At the same time the administrator is in the process of making some
>       changes on the network connected to swp1. To avoid doing it live he would
>       admin_disable swp1 (!IFF_UP) by doing an "ip link set swp1 down" (this
>       is a no-op as event #a has already cleared IFF_UP on swp1).
>    c. If the MLAG peer-link recovers at this point the MLAG app on the
>       secondary switch would try to automatically recover the MLAG ports
>       by clearing proto_down (i.e. setting IFF_UP); including on swp1. Doing
>       that overrides the administrator’s directive to keep swp1 admin_down.
>       Overriding an admin-down in a live network can be very dangerous so it
>       is not possible to do auto-error-recovery unless we have a way to
>       disambiguate between the admin and error states.

I have the need to disambiguate the error state but it doesn't have to be an
IFF_X attribute. Stephen, Do you think it would be more easily consumable if
it were a new/separate net_device attribute instead of being a new bit in
"&struct net_device flags"?

Re: [RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[Stephen Hemminger]

On Wed, 29 Apr 2015 16:25:07 -0700
Anuradha Karuppiah <anuradhak@cumulusnetworks.com> wrote:

> On Wed, Apr 29, 2015 at 4:07 PM, Anuradha Karuppiah
> <anuradhak@cumulusnetworks.com> wrote:
> > On Wed, Apr 29, 2015 at 3:13 PM, Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> >> On Mon, 27 Apr 2015 10:38:21 -0700
> >> anuradhak@cumulusnetworks.com wrote:
> >>
> >>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
> >>>
> >>> This patch introduces an IFF_PROTO_DOWN flag that can be used by
> >>> user space applications to notify drivers that errors have been
> >>> detected on the device.
> >>>
> >>> 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>
> >>
> >> I worry that adding another bit to an already complex state API
> >> will break userspace.
> >>
> >> There are lots of things besides iproute2 which look at those
> >> flags including routing daemons (quagga), network manager, netplugd,
> >> and switch controllers.
> >
> > Yes, I understand your concerns here. And tried to work around introducing
> > a separate error flag by clearing IFF_UP on proto_down/detecting errors (as
> > Scott also brought up earlier).
> >
> > That implementation failed because of the following reasons -
> > 1. There is no way to disambiguate between admin_down (!IFF_UP) and an
> > APP/driver enforced error_down (IFF_PROTO_DOWN). Administrator or
> > automation-scripts that monitor the config assumed that switch-port
> > configuration had somehow fallen out of sync (and attempted to reinstate the
> > admin_up repeatedly).
> >
> > 2. Automatic error recovery was not possible; consider the following scenario
> > for e.g.
> >    a. The MLAG peer-link is down so the MLAG app on the secondary switch has
> >       proto_down’ed all the MLAG ports (including switch-port swp1) by clearing
> >       IFF_UP.
> >    b. At the same time the administrator is in the process of making some
> >       changes on the network connected to swp1. To avoid doing it live he would
> >       admin_disable swp1 (!IFF_UP) by doing an "ip link set swp1 down" (this
> >       is a no-op as event #a has already cleared IFF_UP on swp1).
> >    c. If the MLAG peer-link recovers at this point the MLAG app on the
> >       secondary switch would try to automatically recover the MLAG ports
> >       by clearing proto_down (i.e. setting IFF_UP); including on swp1. Doing
> >       that overrides the administrator’s directive to keep swp1 admin_down.
> >       Overriding an admin-down in a live network can be very dangerous so it
> >       is not possible to do auto-error-recovery unless we have a way to
> >       disambiguate between the admin and error states.
> 
> I have the need to disambiguate the error state but it doesn't have to be an
> IFF_X attribute. Stephen, Do you think it would be more easily consumable if
> it were a new/separate net_device attribute instead of being a new bit in
> "&struct net_device flags"?

You need to separate.
  1. Flags visible by existing user applications. These really can't change.
  2. Flags required for special management applications. These are best handled
     by additional netlink nested attributes
  3. Internal kernel flags. As long as they are not visible to userspace
     nothing should care.

Re: [RFC PATCH net-next v3 1/4] net core: Add IFF_PROTO_DOWN support.

[Anuradha Karuppiah]

On Wed, Apr 29, 2015 at 4:33 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed, 29 Apr 2015 16:25:07 -0700
> Anuradha Karuppiah <anuradhak@cumulusnetworks.com> wrote:
>
>> On Wed, Apr 29, 2015 at 4:07 PM, Anuradha Karuppiah
>> <anuradhak@cumulusnetworks.com> wrote:
>> > On Wed, Apr 29, 2015 at 3:13 PM, Stephen Hemminger
>> > <stephen@networkplumber.org> wrote:
>> >> On Mon, 27 Apr 2015 10:38:21 -0700
>> >> anuradhak@cumulusnetworks.com wrote:
>> >>
>> >>> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>
>> >>>
>> >>> This patch introduces an IFF_PROTO_DOWN flag that can be used by
>> >>> user space applications to notify drivers that errors have been
>> >>> detected on the device.
>> >>>
>> >>> 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>
>> >>
>> >> I worry that adding another bit to an already complex state API
>> >> will break userspace.
>> >>
>> >> There are lots of things besides iproute2 which look at those
>> >> flags including routing daemons (quagga), network manager, netplugd,
>> >> and switch controllers.
>> >
>> > Yes, I understand your concerns here. And tried to work around introducing
>> > a separate error flag by clearing IFF_UP on proto_down/detecting errors (as
>> > Scott also brought up earlier).
>> >
>> > That implementation failed because of the following reasons -
>> > 1. There is no way to disambiguate between admin_down (!IFF_UP) and an
>> > APP/driver enforced error_down (IFF_PROTO_DOWN). Administrator or
>> > automation-scripts that monitor the config assumed that switch-port
>> > configuration had somehow fallen out of sync (and attempted to reinstate the
>> > admin_up repeatedly).
>> >
>> > 2. Automatic error recovery was not possible; consider the following scenario
>> > for e.g.
>> >    a. The MLAG peer-link is down so the MLAG app on the secondary switch has
>> >       proto_down’ed all the MLAG ports (including switch-port swp1) by clearing
>> >       IFF_UP.
>> >    b. At the same time the administrator is in the process of making some
>> >       changes on the network connected to swp1. To avoid doing it live he would
>> >       admin_disable swp1 (!IFF_UP) by doing an "ip link set swp1 down" (this
>> >       is a no-op as event #a has already cleared IFF_UP on swp1).
>> >    c. If the MLAG peer-link recovers at this point the MLAG app on the
>> >       secondary switch would try to automatically recover the MLAG ports
>> >       by clearing proto_down (i.e. setting IFF_UP); including on swp1. Doing
>> >       that overrides the administrator’s directive to keep swp1 admin_down.
>> >       Overriding an admin-down in a live network can be very dangerous so it
>> >       is not possible to do auto-error-recovery unless we have a way to
>> >       disambiguate between the admin and error states.
>>
>> I have the need to disambiguate the error state but it doesn't have to be an
>> IFF_X attribute. Stephen, Do you think it would be more easily consumable if
>> it were a new/separate net_device attribute instead of being a new bit in
>> "&struct net_device flags"?
>
> You need to separate.
>   1. Flags visible by existing user applications. These really can't change.
>   2. Flags required for special management applications. These are best handled
>      by additional netlink nested attributes
>   3. Internal kernel flags. As long as they are not visible to userspace
>      nothing should care.

Ack. I will move IFF_PROTO_DOWN out of "&struct net_device flags".

PROTO_DOWN needs to be configurable/visible only to new APPs so I will add a
separate "&struct net_device protodown" field and the corresponding netlink
attribute.