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

[Radu Rendec <radu.rendec@gmail.com>]

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