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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAD5jUk8gpiX_AyiRPf7fnBouyxWS7V8FEAiiug1A3iMdr_YJBA@mail.gmail.com>
Date:   Wed, 19 Sep 2018 12:44:41 -0400
From:   Radu Rendec <radu.rendec@...il.com>
To:     sd@...asysnail.net
Cc:     netdev@...r.kernel.org, davem@...emloft.net, ptalbert@...hat.com
Subject: Re: [PATCH 1/1] macsec: reflect the master interface state

Hello,

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.

> 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