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

[Sabrina Dubroca <sd@queasysnail.net>]

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