| Message ID | 20180919001612.2636-2-radu.rendec@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | macsec: reflect the master interface state | expand |
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 >
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
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 :/
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
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;
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(-)