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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181001125240.GA8219@bistromath.localdomain>
Date:   Mon, 1 Oct 2018 14:52:40 +0200
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     Radu Rendec <radu.rendec@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, ptalbert@...hat.com
Subject: Re: [PATCH 1/1] macsec: reflect the master interface state

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@...asysnail.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@...wer: <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