[PATCH 0/1] macsec: reflect the master interface state

[PATCH 0/1] macsec: reflect the master interface state

[Radu Rendec]

Hi everyone,

I came across an issue with macsec interfaces where they don't reflect
the master interface state. On one hand you cannot set the macsec
interface link state to up unless the master interface is already up,
but on the other hand, if the master interface goes down while the
macsec interface is already up, the macsec interface stays up.

This may also be problematic if macsec interfaces are used as bridge
ports (probably not very usual, but that is my use case anyway). In that
case the bridge has no idea of the link state of the real device, since
it only sees the macsec interface, which is always up.

I'm proposing the attached patch, which is heavily inspired from the
vlan (802.1q) driver. However, in the case of macsec, there is no
concept of "loose binding" and the state is always reflected.

I tested the patch on x86_64, but I guess it's pretty much architecture
independent.

Any comments or suggestions are welcome. Thanks!

Radu Rendec (1):
  macsec: reflect the master interface state

 drivers/net/macsec.c | 57 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH 1/1] macsec: reflect the master interface state

[Radu Rendec]

This patch makes macsec interfaces reflect the state of the underlying
interface: if the master interface changes state to up/down, the macsec
interface changes its state accordingly.

This closes a loophole in the macsec interface state logic: the macsec
interface cannot be brought up if the master interface is down (the
operation is rejected with ENETDOWN); however, if the macsec interface
is brought up while the master interface is up and then the master
interface goes down, the macsec interface stays up.

Reflecting the master interface state can also be useful if the macsec
interface is used as a bridge port: if the master (physical) interface
goes down, the state propagates through the macsec interface to the
bridge module, which can react to the state change (for example if it
runs STP).

The patch does nothing original. The same logic is implemented for vlan
interfaces in vlan_device_event() in net/8021q/vlan.c. In fact, the code
was copied and adapted from there.

Signed-off-by: Radu Rendec <radu.rendec@gmail.com>
---
 drivers/net/macsec.c | 57 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 7de88b33d5b9..cb93a1290f85 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3486,20 +3486,21 @@ static int macsec_notify(struct notifier_block *this, unsigned long event,
 			 void *ptr)
 {
 	struct net_device *real_dev = netdev_notifier_info_to_dev(ptr);
-	LIST_HEAD(head);
+	struct macsec_dev *m;
+	struct macsec_rxh_data *rxd;
 
 	if (!is_macsec_master(real_dev))
 		return NOTIFY_DONE;
 
+	rxd = macsec_data_rtnl(real_dev);
+
 	switch (event) {
 	case NETDEV_UNREGISTER: {
-		struct macsec_dev *m, *n;
-		struct macsec_rxh_data *rxd;
+		struct macsec_dev *n;
+		LIST_HEAD(head);
 
-		rxd = macsec_data_rtnl(real_dev);
-		list_for_each_entry_safe(m, n, &rxd->secys, secys) {
+		list_for_each_entry_safe(m, n, &rxd->secys, secys)
 			macsec_common_dellink(m->secy.netdev, &head);
-		}
 
 		netdev_rx_handler_unregister(real_dev);
 		kfree(rxd);
@@ -3507,11 +3508,7 @@ static int macsec_notify(struct notifier_block *this, unsigned long event,
 		unregister_netdevice_many(&head);
 		break;
 	}
-	case NETDEV_CHANGEMTU: {
-		struct macsec_dev *m;
-		struct macsec_rxh_data *rxd;
-
-		rxd = macsec_data_rtnl(real_dev);
+	case NETDEV_CHANGEMTU:
 		list_for_each_entry(m, &rxd->secys, secys) {
 			struct net_device *dev = m->secy.netdev;
 			unsigned int mtu = real_dev->mtu - (m->secy.icv_len +
@@ -3520,7 +3517,45 @@ static int macsec_notify(struct notifier_block *this, unsigned long event,
 			if (dev->mtu > mtu)
 				dev_set_mtu(dev, mtu);
 		}
+		break;
+	case NETDEV_CHANGE:
+		list_for_each_entry(m, &rxd->secys, secys) {
+			struct net_device *dev = m->secy.netdev;
+
+			netif_stacked_transfer_operstate(real_dev, dev);
+		}
+		break;
+	case NETDEV_DOWN: {
+		struct net_device *dev, *tmp;
+		LIST_HEAD(close_list);
+
+		list_for_each_entry(m, &rxd->secys, secys) {
+			dev = m->secy.netdev;
+
+			if (dev->flags & IFF_UP)
+				list_add(&dev->close_list, &close_list);
+		}
+
+		dev_close_many(&close_list, false);
+
+		list_for_each_entry_safe(dev, tmp, &close_list, close_list) {
+			netif_stacked_transfer_operstate(real_dev, dev);
+			list_del_init(&dev->close_list);
+		}
+		list_del(&close_list);
+		break;
 	}
+	case NETDEV_UP:
+		list_for_each_entry(m, &rxd->secys, secys) {
+			struct net_device *dev = m->secy.netdev;
+			int flags = dev_get_flags(dev);
+
+			if (!(flags & IFF_UP)) {
+				dev_change_flags(dev, flags | IFF_UP);
+				netif_stacked_transfer_operstate(real_dev, dev);
+			}
+		}
+		break;
 	}
 
 	return NOTIFY_OK;
-- 
2.17.1

Re: [PATCH 1/1] macsec: reflect the master interface state

[Sabrina Dubroca]

Hello,

2018-09-18, 20:16:12 -0400, Radu Rendec wrote:
> This patch makes macsec interfaces reflect the state of the underlying
> interface: if the master interface changes state to up/down, the macsec
> interface changes its state accordingly.

I got a separate report of the same issue and I've been looking in
that area too.

> This closes a loophole in the macsec interface state logic: the macsec
> interface cannot be brought up if the master interface is down (the
> operation is rejected with ENETDOWN); however, if the macsec interface
> is brought up while the master interface is up and then the master
> interface goes down, the macsec interface stays up.

Yes, that's a bit unfortunate. I was wondering if we should just allow
setting the device up, and let link state handle the rest.

> Reflecting the master interface state can also be useful if the macsec
> interface is used as a bridge port: if the master (physical) interface
> goes down, the state propagates through the macsec interface to the
> bridge module, which can react to the state change (for example if it
> runs STP).
> 
> The patch does nothing original. The same logic is implemented for vlan
> interfaces in vlan_device_event() in net/8021q/vlan.c. In fact, the code
> was copied and adapted from there.

It's missing small parts of link state handling that I have been
testing, mainly when creating a new device.

> Signed-off-by: Radu Rendec <radu.rendec@gmail.com>
> ---
>  drivers/net/macsec.c | 57 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 7de88b33d5b9..cb93a1290f85 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -3486,20 +3486,21 @@ static int macsec_notify(struct notifier_block *this, unsigned long event,
>  			 void *ptr)
>  {
>  	struct net_device *real_dev = netdev_notifier_info_to_dev(ptr);
> -	LIST_HEAD(head);
> +	struct macsec_dev *m;
> +	struct macsec_rxh_data *rxd;
>  
>  	if (!is_macsec_master(real_dev))
>  		return NOTIFY_DONE;
>  
> +	rxd = macsec_data_rtnl(real_dev);
> +
>  	switch (event) {
>  	case NETDEV_UNREGISTER: {
> -		struct macsec_dev *m, *n;
> -		struct macsec_rxh_data *rxd;
> +		struct macsec_dev *n;
> +		LIST_HEAD(head);
>  
> -		rxd = macsec_data_rtnl(real_dev);
> -		list_for_each_entry_safe(m, n, &rxd->secys, secys) {
> +		list_for_each_entry_safe(m, n, &rxd->secys, secys)
>  			macsec_common_dellink(m->secy.netdev, &head);
> -		}

Please don't mix coding style changes with bug fixes.

[...]
> +	case NETDEV_DOWN: {
> +		struct net_device *dev, *tmp;
> +		LIST_HEAD(close_list);
> +
> +		list_for_each_entry(m, &rxd->secys, secys) {
> +			dev = m->secy.netdev;
> +
> +			if (dev->flags & IFF_UP)
> +				list_add(&dev->close_list, &close_list);
> +		}
> +
> +		dev_close_many(&close_list, false);
> +
> +		list_for_each_entry_safe(dev, tmp, &close_list, close_list) {
> +			netif_stacked_transfer_operstate(real_dev, dev);
> +			list_del_init(&dev->close_list);
> +		}
> +		list_del(&close_list);
> +		break;

My version of the patch only does netif_stacked_transfer_operstate(),
and skips setting the device administratively down (ie, same as
NETDEV_CHANGE).

>  	}
> +	case NETDEV_UP:
> +		list_for_each_entry(m, &rxd->secys, secys) {
> +			struct net_device *dev = m->secy.netdev;
> +			int flags = dev_get_flags(dev);
> +
> +			if (!(flags & IFF_UP)) {
> +				dev_change_flags(dev, flags | IFF_UP);
> +				netif_stacked_transfer_operstate(real_dev, dev);
> +			}
> +		}
> +		break;

I don't like that this completely ignores whether the device was done
independently of the lower link. Maybe the administrator actually
wants the macsec device down, regardless of state changes on the lower
device.

I know it's consistent with what vlan is doing, but I'm not convinced
macsec should adopt this behavior.

>  	}
>  
>  	return NOTIFY_OK;
> -- 
> 2.17.1
> 

-- 
Sabrina

Re: [PATCH 1/1] macsec: reflect the master interface state

[Radu Rendec]

Hello,

On Wed, Sep 19, 2018 at 11:24 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2018-09-18, 20:16:12 -0400, Radu Rendec wrote:
> > This patch makes macsec interfaces reflect the state of the underlying
> > interface: if the master interface changes state to up/down, the macsec
> > interface changes its state accordingly.
>
> Yes, that's a bit unfortunate. I was wondering if we should just allow
> setting the device up, and let link state handle the rest.

Yes, that could work. It would also be consistent with the idea of
propagating only the link state.

> It's missing small parts of link state handling that I have been
> testing, mainly when creating a new device.

Can you please be more specific? I've just looked at macsec_newlink()
and I didn't notice anything related to link state handling.

> > -             list_for_each_entry_safe(m, n, &rxd->secys, secys) {
> > +             list_for_each_entry_safe(m, n, &rxd->secys, secys)
> >                       macsec_common_dellink(m->secy.netdev, &head);
> > -             }
>
> Please don't mix coding style changes with bug fixes.

Noted.

> > +     case NETDEV_DOWN: {
> > +             struct net_device *dev, *tmp;
> > +             LIST_HEAD(close_list);
> > +
> > +             list_for_each_entry(m, &rxd->secys, secys) {
> > +                     dev = m->secy.netdev;
> > +
> > +                     if (dev->flags & IFF_UP)
> > +                             list_add(&dev->close_list, &close_list);
> > +             }
> > +
> > +             dev_close_many(&close_list, false);
> > +
> > +             list_for_each_entry_safe(dev, tmp, &close_list, close_list) {
> > +                     netif_stacked_transfer_operstate(real_dev, dev);
> > +                     list_del_init(&dev->close_list);
> > +             }
> > +             list_del(&close_list);
> > +             break;
>
> My version of the patch only does netif_stacked_transfer_operstate(),
> and skips setting the device administratively down (ie, same as
> NETDEV_CHANGE).

That makes sense. But, since you mentioned you had your own patch, does
it make sense for me to continue working on mine?

> >       }
> > +     case NETDEV_UP:
> > +             list_for_each_entry(m, &rxd->secys, secys) {
> > +                     struct net_device *dev = m->secy.netdev;
> > +                     int flags = dev_get_flags(dev);
> > +
> > +                     if (!(flags & IFF_UP)) {
> > +                             dev_change_flags(dev, flags | IFF_UP);
> > +                             netif_stacked_transfer_operstate(real_dev, dev);
> > +                     }
> > +             }
> > +             break;
>
> I don't like that this completely ignores whether the device was done
> independently of the lower link. Maybe the administrator actually
> wants the macsec device down, regardless of state changes on the lower
> device.

I thought about that too and I don't like it either. Perhaps the vlan
approach of having a "loose binding" flag is worth considering. Then the
admin can decice for themselves.

However, I guess the problem disappears if only the link state is
propagated. The only caveat to that is to not propagate an "up" link
state while the macsec interface is administratively down, but
"remember" to propagate it later if the macsec interface is
administratively set to "up".

Thanks for the feedback!

Radu Rendec

Re: [PATCH 1/1] macsec: reflect the master interface state

[Sabrina Dubroca]

2018-09-19, 12:44:41 -0400, Radu Rendec wrote:
> Hello,

Gah, sorry, this fell into the "week-end" crack and I forgot to answer :(

> On Wed, Sep 19, 2018 at 11:24 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> > 2018-09-18, 20:16:12 -0400, Radu Rendec wrote:
> > > This patch makes macsec interfaces reflect the state of the underlying
> > > interface: if the master interface changes state to up/down, the macsec
> > > interface changes its state accordingly.
> >
> > Yes, that's a bit unfortunate. I was wondering if we should just allow
> > setting the device up, and let link state handle the rest.
> 
> Yes, that could work. It would also be consistent with the idea of
> propagating only the link state.

Do you want to handle it, or should I?

> > It's missing small parts of link state handling that I have been
> > testing, mainly when creating a new device.
> 
> Can you please be more specific? I've just looked at macsec_newlink()
> and I didn't notice anything related to link state handling.

Yes, that's actually the problem. For example,
macvlan_common_newlink() does:

        netif_stacked_transfer_operstate(lowerdev, dev);
        linkwatch_fire_event(dev);

If you try to create a macsec device UP with its lowerdev UP:

    ip link set $lower up
    ip link add link $lower up type macsec


You'll get:

    macsec0@$lower: <BROADCAST,MULTICAST,UP,LOWER_UP> [...] state UNKNOWN [...]

and you want "state UP".


> > My version of the patch only does netif_stacked_transfer_operstate(),
> > and skips setting the device administratively down (ie, same as
> > NETDEV_CHANGE).
> 
> That makes sense. But, since you mentioned you had your own patch, does
> it make sense for me to continue working on mine?

Here's what I have (without a commit message, because I hadn't written
one yet -- yours looks pretty good, other than missing a "Fixes:" tag):

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 7de88b33d5b9..3532cdee2761 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3308,6 +3308,9 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
 	if (err < 0)
 		goto del_dev;
 
+	netif_stacked_transfer_operstate(real_dev, dev);
+	linkwatch_fire_event(dev);
+
 	macsec_generation++;
 
 	return 0;
@@ -3492,6 +3495,20 @@ static int macsec_notify(struct notifier_block *this, unsigned long event,
 		return NOTIFY_DONE;
 
 	switch (event) {
+	case NETDEV_DOWN:
+	case NETDEV_UP:
+	case NETDEV_CHANGE: {
+		struct macsec_dev *m, *n;
+		struct macsec_rxh_data *rxd;
+
+		rxd = macsec_data_rtnl(real_dev);
+		list_for_each_entry_safe(m, n, &rxd->secys, secys) {
+			struct net_device *dev = m->secy.netdev;
+
+			netif_stacked_transfer_operstate(real_dev, dev);
+		}
+		break;
+	}
 	case NETDEV_UNREGISTER: {
 		struct macsec_dev *m, *n;
 		struct macsec_rxh_data *rxd;



If you want to keep working on this, that's okay for me. I'm not hung
up on who gets authorship.


> > >       }
> > > +     case NETDEV_UP:
> > > +             list_for_each_entry(m, &rxd->secys, secys) {
> > > +                     struct net_device *dev = m->secy.netdev;
> > > +                     int flags = dev_get_flags(dev);
> > > +
> > > +                     if (!(flags & IFF_UP)) {
> > > +                             dev_change_flags(dev, flags | IFF_UP);
> > > +                             netif_stacked_transfer_operstate(real_dev, dev);
> > > +                     }
> > > +             }
> > > +             break;
> >
> > I don't like that this completely ignores whether the device was done
> > independently of the lower link. Maybe the administrator actually
> > wants the macsec device down, regardless of state changes on the lower
> > device.
> 
> I thought about that too and I don't like it either. Perhaps the vlan
> approach of having a "loose binding" flag is worth considering. Then the
> admin can decice for themselves.

Do you have a use case where you'd want that? If that solves some
problem for you (a problem that can't be solved just by fixing the
current bug), then ok, we can consider it. I prefer to avoid adding
obscure options and more code unless they're necessary.

> However, I guess the problem disappears if only the link state is
> propagated. The only caveat to that is to not propagate an "up" link
> state while the macsec interface is administratively down, but
> "remember" to propagate it later if the macsec interface is
> administratively set to "up".
> 
> Thanks for the feedback!

And sorry for the delay in answering :/


-- 
Sabrina

Re: [PATCH 1/1] macsec: reflect the master interface state

[Radu Rendec]

On Mon, 2018-10-01 at 14:52 +0200, Sabrina Dubroca wrote:
> 2018-09-19, 12:44:41 -0400, Radu Rendec wrote:
> > Hello,
>
> Gah, sorry, this fell into the "week-end" crack and I forgot to answer :(

No worries. I was on vacation in the meantime, hence my late reply.

> > On Wed, Sep 19, 2018 at 11:24 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> > > 2018-09-18, 20:16:12 -0400, Radu Rendec wrote:
> > > > This patch makes macsec interfaces reflect the state of the underlying
> > > > interface: if the master interface changes state to up/down, the macsec
> > > > interface changes its state accordingly.
> > >
> > > Yes, that's a bit unfortunate. I was wondering if we should just allow
> > > setting the device up, and let link state handle the rest.
> >
> > Yes, that could work. It would also be consistent with the idea of
> > propagating only the link state.
>
> Do you want to handle it, or should I?

Too late. I saw your patches this morning. Thanks for sending them out
and sorry for not looking into this sooner.

> > > It's missing small parts of link state handling that I have been
> > > testing, mainly when creating a new device.
> >
> > Can you please be more specific? I've just looked at macsec_newlink()
> > and I didn't notice anything related to link state handling.
>
> Yes, that's actually the problem. For example,
> macvlan_common_newlink() does:
>
>         netif_stacked_transfer_operstate(lowerdev, dev);
>         linkwatch_fire_event(dev);
>
> If you try to create a macsec device UP with its lowerdev UP:
>
>     ip link set $lower up
>     ip link add link $lower up type macsec
>
>
> You'll get:
>
>     macsec0@$lower: <BROADCAST,MULTICAST,UP,LOWER_UP> [...] state UNKNOWN [...]
>
> and you want "state UP".

That makes sense. Thanks for taking the time to explain this!

> > > I don't like that this completely ignores whether the device was done
> > > independently of the lower link. Maybe the administrator actually
> > > wants the macsec device down, regardless of state changes on the lower
> > > device.
> >
> > I thought about that too and I don't like it either. Perhaps the vlan
> > approach of having a "loose binding" flag is worth considering. Then the
> > admin can decice for themselves.
>
> Do you have a use case where you'd want that? If that solves some
> problem for you (a problem that can't be solved just by fixing the
> current bug), then ok, we can consider it. I prefer to avoid adding
> obscure options and more code unless they're necessary.

I do have a use case, but with the current bugs fixed, it can be
implemented at the application level by setting the admin state on both
the underlying interface and the macsec interface at the same time.
There is no need to implement this in the kernel and complicate the
macsec code unnecessarily.

> And sorry for the delay in answering :/

No need to apologize. I had a huge delay myself in getting back.

Best regards,
Radu Rendec