LinuxLists.cc - [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

On 08.08.2019 17:05, Antoine Tenart wrote:

> The Rx and TX handlers are modified to take in account the special case
> were the MACsec transformation happens in the hardware, whether in a PHY
> or in a MAC, as the packets seen by the networking stack on both the

Don't you think we may eventually may need xmit / handle_frame ops to be
a part of macsec_ops?

That way software macsec could be extract to just another type of offload.
The drawback of current code is it doesn't show explicitly the path of
offloaded packets. It is hidden in `handle_not_macsec` and in
`macsec_start_xmit` branch. This makes incorrect counters to tick (see my below
comment)

Another thing is that both xmit / macsec_handle_frame can't now be customized
by device driver. But this may be required.
We for example have usecases and HW features to allow specific flows to bypass
macsec encryption. This is normally used for macsec key control protocols,
identified by ethertype. Your phy is also capable on that as I see.

> @@ -2546,11 +2814,15 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
> {
> struct macsec_dev *macsec = netdev_priv(dev);
> struct macsec_secy *secy = &macsec->secy;
> + struct macsec_tx_sc *tx_sc = &secy->tx_sc;
> struct pcpu_secy_stats *secy_stats;
> + struct macsec_tx_sa *tx_sa;
> int ret, len;
>
> + tx_sa = macsec_txsa_get(tx_sc->sa[tx_sc->encoding_sa]);

Declared, but not used?

> /* 10.5 */
> - if (!secy->protect_frames) {
> + if (!secy->protect_frames || macsec_get_ops(netdev_priv(dev), NULL)) {
> secy_stats = this_cpu_ptr(macsec->stats);
> u64_stats_update_begin(&secy_stats->syncp);
> secy_stats->stats.OutPktsUntagged++;

Here you use same `if` for sw and hw flows, this making `OutPktsUntagged`
counter invalid.

> struct macsec_dev *macsec = macsec_priv(dev);
> - struct net_device *real_dev;
> + struct net_device *real_dev, *loop_dev;
> + struct macsec_context ctx;
> + const struct macsec_ops *ops;
> + struct net *loop_net;

Reverse Christmas tree is normally a formatting requirement where possible.

> + for_each_net(loop_net) {
> + for_each_netdev(loop_net, loop_dev) {
> + struct macsec_dev *priv;
> +
> + if (!netif_is_macsec(loop_dev))
> + continue;
> +
> + priv = macsec_priv(loop_dev);
> +
> + /* A limitation of the MACsec h/w offloading is only a
> + * single MACsec interface can be created for a given
> + * real interface.
> + */
> + if (macsec_get_ops(netdev_priv(dev), NULL) &&
> + priv->real_dev == real_dev)
> + return -EBUSY;
> + }
> + }
> +

There is no need to do this search loop if `macsec_get_ops(..) == NULL` ?
So you can extract this check before `for_each_net` for SW macsec...

Regards,
Igor

2019-08-10 16:37:13

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

On Thu, Aug 08, 2019 at 04:05:57PM +0200, Antoine Tenart wrote:
> This patch introduces the MACsec hardware offloading infrastructure.
>
> The main idea here is to re-use the logic and data structures of the
> software MACsec implementation. This allows not to duplicate definitions
> and structure storing the same kind of information. It also allows to
> use a unified genlink interface for both MACsec implementations (so that
> the same userspace tool, `ip macsec`, is used with the same arguments).
> The MACsec offloading support cannot be disabled if an interface
> supports it at the moment.
>
> The MACsec configuration is passed to device drivers supporting it
> through macsec_hw_offload() which is called from the MACsec genl
> helpers. This function calls the macsec ops of PHY and Ethernet
> drivers in two steps

Hi Antoine, Igor

It is great that you are thinking how a MAC driver would make use of
this. But on the flip side, we don't usual add an API unless there is
a user. And as far as i see, you only add a PHY level implementation,
not a MAC level.

Igor, what is your interest here? I know the Aquantia PHY can do
MACsec, but i guess you are more interested in the atlantic and AQC111
MAC drivers which hide the PHY behind firmware rather than make use of
the Linux aquantia PHY driver. Are you likely to be contributing a MAC
driver level implementation of MACsec soon?

Thanks
Andrew

2019-08-12 08:16:36

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi Andrew,

On Sat, Aug 10, 2019 at 06:34:23PM +0200, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 04:05:57PM +0200, Antoine Tenart wrote:
> > This patch introduces the MACsec hardware offloading infrastructure.
> >
> > The main idea here is to re-use the logic and data structures of the
> > software MACsec implementation. This allows not to duplicate definitions
> > and structure storing the same kind of information. It also allows to
> > use a unified genlink interface for both MACsec implementations (so that
> > the same userspace tool, `ip macsec`, is used with the same arguments).
> > The MACsec offloading support cannot be disabled if an interface
> > supports it at the moment.
> >
> > The MACsec configuration is passed to device drivers supporting it
> > through macsec_hw_offload() which is called from the MACsec genl
> > helpers. This function calls the macsec ops of PHY and Ethernet
> > drivers in two steps
>
> It is great that you are thinking how a MAC driver would make use of
> this. But on the flip side, we don't usual add an API unless there is
> a user. And as far as i see, you only add a PHY level implementation,
> not a MAC level.

That's right, and the only modification here is a simple patch adding
the MACsec ops within struct net_device. I can remove it as we do not
have providers as of now and it can be added easily later on.

Thanks,
Antoine

--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2019-08-13 10:48:23

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi Igor,

On Sat, Aug 10, 2019 at 01:20:32PM +0000, Igor Russkikh wrote:
> On 08.08.2019 17:05, Antoine Tenart wrote:
>
> > The Rx and TX handlers are modified to take in account the special case
> > were the MACsec transformation happens in the hardware, whether in a PHY
> > or in a MAC, as the packets seen by the networking stack on both the
>
> Don't you think we may eventually may need xmit / handle_frame ops to be
> a part of macsec_ops?
>
> That way software macsec could be extract to just another type of offload.
> The drawback of current code is it doesn't show explicitly the path of
> offloaded packets. It is hidden in `handle_not_macsec` and in
> `macsec_start_xmit` branch. This makes incorrect counters to tick (see my below
> comment)
>
> Another thing is that both xmit / macsec_handle_frame can't now be customized
> by device driver. But this may be required.
> We for example have usecases and HW features to allow specific flows to bypass
> macsec encryption. This is normally used for macsec key control protocols,
> identified by ethertype. Your phy is also capable on that as I see.

I think this question is linked to the use of a MACsec virtual interface
when using h/w offloading. The starting point for me was that I wanted
to reuse the data structures and the API exposed to the userspace by the
s/w implementation of MACsec. I then had two choices: keeping the exact
same interface for the user (having a virtual MACsec interface), or
registering the MACsec genl ops onto the real net devices (and making
the s/w implementation a virtual net dev and a provider of the MACsec
"offloading" ops).

The advantages of the first option were that nearly all the logic of the
s/w implementation could be kept and especially that it would be
transparent for the user to use both implementations of MACsec. But this
raised an issue as I had to modify the xmit / handle_frame ops to let
all the traffic pass. This is because we have no way of knowing if a
frame was handled by the MACsec h/w or not in ingress. So the virtual
interface here only serve as the entrypoint for the API...

The second option would have the advantage to better represent the actual
flow, but the way of configuring MACsec would be a bit different for the
user, whether he wants to use s/w or h/w MACsec. If we were to do this I
think we could extract the genl functions from the MACsec s/w
implementation, and let it implement the MACsec ops (exactly as the
offloading drivers).

I'm open to discussing this :)

As for the need for xmit / handle_frame ops (for a MAC w/ MACsec
offloading), I'd say the xmit / handle_frame ops of the real net device
driver could be used as the one of the MACsec virtual interface do not
do much (regardless of the implementation choice discussed above).

> > @@ -2546,11 +2814,15 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
> > {
> > struct macsec_dev *macsec = netdev_priv(dev);
> > struct macsec_secy *secy = &macsec->secy;
> > + struct macsec_tx_sc *tx_sc = &secy->tx_sc;
> > struct pcpu_secy_stats *secy_stats;
> > + struct macsec_tx_sa *tx_sa;
> > int ret, len;
> >
> > + tx_sa = macsec_txsa_get(tx_sc->sa[tx_sc->encoding_sa]);
>
> Declared, but not used?

I'll remove it then.

> > /* 10.5 */
> > - if (!secy->protect_frames) {
> > + if (!secy->protect_frames || macsec_get_ops(netdev_priv(dev), NULL)) {
> > secy_stats = this_cpu_ptr(macsec->stats);
> > u64_stats_update_begin(&secy_stats->syncp);
> > secy_stats->stats.OutPktsUntagged++;
>
> Here you use same `if` for sw and hw flows, this making `OutPktsUntagged`
> counter invalid.

Right, I'll try to fix that.

> > struct macsec_dev *macsec = macsec_priv(dev);
> > - struct net_device *real_dev;
> > + struct net_device *real_dev, *loop_dev;
> > + struct macsec_context ctx;
> > + const struct macsec_ops *ops;
> > + struct net *loop_net;
>
> Reverse Christmas tree is normally a formatting requirement where possible.

Sure.

> > + for_each_net(loop_net) {
> > + for_each_netdev(loop_net, loop_dev) {
> > + struct macsec_dev *priv;
> > +
> > + if (!netif_is_macsec(loop_dev))
> > + continue;
> > +
> > + priv = macsec_priv(loop_dev);
> > +
> > + /* A limitation of the MACsec h/w offloading is only a
> > + * single MACsec interface can be created for a given
> > + * real interface.
> > + */
> > + if (macsec_get_ops(netdev_priv(dev), NULL) &&
> > + priv->real_dev == real_dev)
> > + return -EBUSY;
> > + }
> > + }
> > +
>
> There is no need to do this search loop if `macsec_get_ops(..) == NULL` ?
> So you can extract this check before `for_each_net` for SW macsec...

Right, I'll fix it!

Thanks!
Antoine

--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2019-08-13 13:19:17

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

On Tue, Aug 13, 2019 at 10:58:17AM +0200, Antoine Tenart wrote:
> I think this question is linked to the use of a MACsec virtual interface
> when using h/w offloading. The starting point for me was that I wanted
> to reuse the data structures and the API exposed to the userspace by the
> s/w implementation of MACsec. I then had two choices: keeping the exact
> same interface for the user (having a virtual MACsec interface), or
> registering the MACsec genl ops onto the real net devices (and making
> the s/w implementation a virtual net dev and a provider of the MACsec
> "offloading" ops).
>
> The advantages of the first option were that nearly all the logic of the
> s/w implementation could be kept and especially that it would be
> transparent for the user to use both implementations of MACsec.

Hi Antoine

We have always talked about offloading operations to the hardware,
accelerating what the linux stack can do by making use of hardware
accelerators. The basic user API should not change because of
acceleration. Those are the general guidelines.

It would however be interesting to get comments from those who did the
software implementation and what they think of this architecture. I've
no personal experience with MACSec, so it is hard for me to say if the
current architecture makes sense when using accelerators.

Andrew

2019-08-13 15:26:38

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

On 10.08.2019 19:34, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 04:05:57PM +0200, Antoine Tenart wrote:

>> The MACsec configuration is passed to device drivers supporting it
>> through macsec_hw_offload() which is called from the MACsec genl
>> helpers. This function calls the macsec ops of PHY and Ethernet
>> drivers in two steps
>
> Hi Antoine, Igor
>
> It is great that you are thinking how a MAC driver would make use of
> this. But on the flip side, we don't usual add an API unless there is
> a user. And as far as i see, you only add a PHY level implementation,
> not a MAC level.
>
> Igor, what is your interest here? I know the Aquantia PHY can do
> MACsec, but i guess you are more interested in the atlantic and AQC111
> MAC drivers which hide the PHY behind firmware rather than make use of
> the Linux aquantia PHY driver. Are you likely to be contributing a MAC
> driver level implementation of MACsec soon?

Hi Andrew,

Yes, we are interested in MAC level MACSec offload implementation.
Although in our solution macsec engine itself is in Phy, we do
actively use firmware support in areas of configuration, interrupt management.

So from SW perspective that'll be MAC driver level macsec.

Regards,
Igor

2019-08-13 16:20:57

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

On 13.08.2019 16:17, Andrew Lunn wrote:
> On Tue, Aug 13, 2019 at 10:58:17AM +0200, Antoine Tenart wrote:
>> I think this question is linked to the use of a MACsec virtual interface
>> when using h/w offloading. The starting point for me was that I wanted
>> to reuse the data structures and the API exposed to the userspace by the
>> s/w implementation of MACsec. I then had two choices: keeping the exact
>> same interface for the user (having a virtual MACsec interface), or
>> registering the MACsec genl ops onto the real net devices (and making
>> the s/w implementation a virtual net dev and a provider of the MACsec
>> "offloading" ops).
>>
>> The advantages of the first option were that nearly all the logic of the
>> s/w implementation could be kept and especially that it would be
>> transparent for the user to use both implementations of MACsec.
>
> Hi Antoine
>
> We have always talked about offloading operations to the hardware,
> accelerating what the linux stack can do by making use of hardware
> accelerators. The basic user API should not change because of
> acceleration. Those are the general guidelines.
>
> It would however be interesting to get comments from those who did the
> software implementation and what they think of this architecture. I've
> no personal experience with MACSec, so it is hard for me to say if the
> current architecture makes sense when using accelerators.

In terms of overall concepts, I'd add the following:

1) With current implementation it's impossible to install SW macsec engine onto
the device which supports HW offload. That could be a strong limitation in
cases when user sees HW macsec offload is broken or work differently, and he/she
wants to replace it with SW one.
MACSec is a complex feature, and it may happen something is missing in HW.
Trivial example is 256bit encryption, which is not always a musthave in HW
implementations.

2) I think, Antoine, its not totally true that otherwise the user macsec API
will be broken/changed. netlink api is the same, the only thing we may want to
add is an optional parameter to force selection of SW macsec engine.

I'm also eager to hear from sw macsec users/devs on whats better here.

Regards,
Igor

2019-08-13 16:29:44

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

> 1) With current implementation it's impossible to install SW macsec engine onto
> the device which supports HW offload. That could be a strong limitation in
> cases when user sees HW macsec offload is broken or work differently, and he/she
> wants to replace it with SW one.
> MACSec is a complex feature, and it may happen something is missing in HW.
> Trivial example is 256bit encryption, which is not always a musthave in HW
> implementations.

Ideally, we want the driver to return EOPNOTSUPP if it does not
support something and the software implement should be used.

If the offload is broken, we want a bug report! And if it works
differently, it suggests there is also a bug we need to fix, or the
standard is ambiguous.

It would also be nice to add extra information to the netlink API to
indicate if HW or SW is being used. In other places where we offload
to accelerators we have such additional information.

Andrew

2019-08-14 08:32:58

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi Igor,

On Tue, Aug 13, 2019 at 04:18:40PM +0000, Igor Russkikh wrote:
> On 13.08.2019 16:17, Andrew Lunn wrote:
> > On Tue, Aug 13, 2019 at 10:58:17AM +0200, Antoine Tenart wrote:
> >> I think this question is linked to the use of a MACsec virtual interface
> >> when using h/w offloading. The starting point for me was that I wanted
> >> to reuse the data structures and the API exposed to the userspace by the
> >> s/w implementation of MACsec. I then had two choices: keeping the exact
> >> same interface for the user (having a virtual MACsec interface), or
> >> registering the MACsec genl ops onto the real net devices (and making
> >> the s/w implementation a virtual net dev and a provider of the MACsec
> >> "offloading" ops).
> >>
> >> The advantages of the first option were that nearly all the logic of the
> >> s/w implementation could be kept and especially that it would be
> >> transparent for the user to use both implementations of MACsec.
> >
> > We have always talked about offloading operations to the hardware,
> > accelerating what the linux stack can do by making use of hardware
> > accelerators. The basic user API should not change because of
> > acceleration. Those are the general guidelines.
> >
> > It would however be interesting to get comments from those who did the
> > software implementation and what they think of this architecture. I've
> > no personal experience with MACSec, so it is hard for me to say if the
> > current architecture makes sense when using accelerators.
>
> In terms of overall concepts, I'd add the following:
>
> 1) With current implementation it's impossible to install SW macsec engine onto
> the device which supports HW offload. That could be a strong limitation in
> cases when user sees HW macsec offload is broken or work differently, and he/she
> wants to replace it with SW one.
> MACSec is a complex feature, and it may happen something is missing in HW.
> Trivial example is 256bit encryption, which is not always a musthave in HW
> implementations.

Agreed. I'm not sure it would be possible to have both used at the same
time but there should be a way to switch between the two
implementations. That is not supported for now, but I think that would
be a good thing, and can probably come later on.

> 2) I think, Antoine, its not totally true that otherwise the user macsec API
> will be broken/changed. netlink api is the same, the only thing we may want to
> add is an optional parameter to force selection of SW macsec engine.

I meant that we can either have a virtual net device representing the
MACsec feature and being the iface used to configure it, or we could
have it only when s/w MACsec is used. That to me is part of the "API",
or at least part of what's exposed to the user.

> I'm also eager to hear from sw macsec users/devs on whats better here.

I'd like more comments as well :)

Thanks!
Antoine

--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2019-08-14 08:34:45

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi Andrew,

On Tue, Aug 13, 2019 at 06:28:23PM +0200, Andrew Lunn wrote:
>
> It would also be nice to add extra information to the netlink API to
> indicate if HW or SW is being used. In other places where we offload
> to accelerators we have such additional information.

Yes, that would be very nice to have.

Thanks!
Antoine

--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2019-08-14 23:59:36

by Florian Fainelli

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

On 8/13/19 9:28 AM, Andrew Lunn wrote:
>> 1) With current implementation it's impossible to install SW macsec engine onto
>> the device which supports HW offload. That could be a strong limitation in
>> cases when user sees HW macsec offload is broken or work differently, and he/she
>> wants to replace it with SW one.
>> MACSec is a complex feature, and it may happen something is missing in HW.
>> Trivial example is 256bit encryption, which is not always a musthave in HW
>> implementations.
>
> Ideally, we want the driver to return EOPNOTSUPP if it does not
> support something and the software implement should be used.
>
> If the offload is broken, we want a bug report! And if it works
> differently, it suggests there is also a bug we need to fix, or the
> standard is ambiguous.
>
> It would also be nice to add extra information to the netlink API to
> indicate if HW or SW is being used. In other places where we offload
> to accelerators we have such additional information.

Igor's point is entirely valid in that you should allow both offload to
HW for what is possible and offload to a software implementation for
what is not supported in HW.

Let's an example that is hopefully more familiar to the people in this
thread. Consider a NIC that can do single VLAN tag offload on xmit (or
receive, does not matter), and you find yourself using a double VLAN tag
configuration. You would create a first VLAN stacked network device
which is fully offloaded onto the underlying NIC, and a second VLAN
stacked network device on top of the first once which is not offloaded.

The way I would imagine a MACsec offload is kind of similar here, in
that it would be a macsec virtual network device on top of an underlying
physical device. If no offload is possible, the virtual network device's
xmit/receive path is used. If the NIC driver can offload it, it does
not. How it does it, whether at the MAC/DMA level, or at the PHY level
can be a check added at the appropriate level.
--
Florian

2019-08-16 13:26:12

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

2019-08-13, 10:58:17 +0200, Antoine Tenart wrote:
> Hi Igor,
>
> On Sat, Aug 10, 2019 at 01:20:32PM +0000, Igor Russkikh wrote:
> > On 08.08.2019 17:05, Antoine Tenart wrote:
> >
> > > The Rx and TX handlers are modified to take in account the special case
> > > were the MACsec transformation happens in the hardware, whether in a PHY
> > > or in a MAC, as the packets seen by the networking stack on both the
> >
> > Don't you think we may eventually may need xmit / handle_frame ops to be
> > a part of macsec_ops?
> >
> > That way software macsec could be extract to just another type of offload.
> > The drawback of current code is it doesn't show explicitly the path of
> > offloaded packets. It is hidden in `handle_not_macsec` and in
> > `macsec_start_xmit` branch. This makes incorrect counters to tick (see my below
> > comment)
> >
> > Another thing is that both xmit / macsec_handle_frame can't now be customized
> > by device driver. But this may be required.
> > We for example have usecases and HW features to allow specific flows to bypass
> > macsec encryption. This is normally used for macsec key control protocols,
> > identified by ethertype. Your phy is also capable on that as I see.
>
> I think this question is linked to the use of a MACsec virtual interface
> when using h/w offloading. The starting point for me was that I wanted
> to reuse the data structures and the API exposed to the userspace by the
> s/w implementation of MACsec. I then had two choices: keeping the exact
> same interface for the user (having a virtual MACsec interface), or

Unless it's really infeasible, yes, that's how things should be done IMO.

> registering the MACsec genl ops onto the real net devices (and making
> the s/w implementation a virtual net dev and a provider of the MACsec
> "offloading" ops).

Please, no :( Let's keep it as close as possible to the software
implementation, unless there's a really good reason not to. It's not
just "ip macsec" btw, wpa_supplicant can also configure MACsec and
would also need some logic to pick the device on which to do the genl
operations in that case.

> The advantages of the first option were that nearly all the logic of the
> s/w implementation could be kept and especially that it would be
> transparent for the user to use both implementations of MACsec. But this
> raised an issue as I had to modify the xmit / handle_frame ops to let
> all the traffic pass. This is because we have no way of knowing if a
> frame was handled by the MACsec h/w or not in ingress. So the virtual
> interface here only serve as the entrypoint for the API...

It's also the interface on which you'll run DHCP or install IP addresses.

> The second option would have the advantage to better represent the actual
> flow, but the way of configuring MACsec would be a bit different for the
> user, whether he wants to use s/w or h/w MACsec. If we were to do this I
> think we could extract the genl functions from the MACsec s/w
> implementation, and let it implement the MACsec ops (exactly as the
> offloading drivers).
>
> I'm open to discussing this :)
>
> As for the need for xmit / handle_frame ops (for a MAC w/ MACsec
> offloading), I'd say the xmit / handle_frame ops of the real net device
> driver could be used as the one of the MACsec virtual interface do not
> do much (regardless of the implementation choice discussed above).

There's no "handle_frame" op on a real device. macsec_handle_frame is
an rx_handler specificity that grabs packets from a real device and
sends them into a virtual device stacked on top of it. A real device
just hands packets over to the stack via NAPI.

> > > @@ -2546,11 +2814,15 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
> > > {
> > > struct macsec_dev *macsec = netdev_priv(dev);
> > > struct macsec_secy *secy = &macsec->secy;
> > > + struct macsec_tx_sc *tx_sc = &secy->tx_sc;
> > > struct pcpu_secy_stats *secy_stats;
> > > + struct macsec_tx_sa *tx_sa;
> > > int ret, len;
> > >
> > > + tx_sa = macsec_txsa_get(tx_sc->sa[tx_sc->encoding_sa]);
> >
> > Declared, but not used?
>
> I'll remove it then.

That's also a refcount leak, so, yes, please get rid of it.

[I'll answer the rest of the patch separately]

--
Sabrina

2019-08-16 13:27:35

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

2019-08-13, 18:28:23 +0200, Andrew Lunn wrote:
> > 1) With current implementation it's impossible to install SW macsec engine onto
> > the device which supports HW offload. That could be a strong limitation in
> > cases when user sees HW macsec offload is broken or work differently, and he/she
> > wants to replace it with SW one.
> > MACSec is a complex feature, and it may happen something is missing in HW.
> > Trivial example is 256bit encryption, which is not always a musthave in HW
> > implementations.
>
> Ideally, we want the driver to return EOPNOTSUPP if it does not
> support something and the software implement should be used.
>
> If the offload is broken, we want a bug report! And if it works
> differently, it suggests there is also a bug we need to fix, or the
> standard is ambiguous.

Yes. But in the meantime, we want the user to be able to disable the
offload. It's helpful for debugging purposes, and it can provide some
level of functionality until the bug is fixed or non-buggy hardware
becomes available.

> It would also be nice to add extra information to the netlink API to
> indicate if HW or SW is being used. In other places where we offload
> to accelerators we have such additional information.

+1

--
Sabrina

2019-08-16 13:31:24

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

2019-08-13, 16:18:40 +0000, Igor Russkikh wrote:
> On 13.08.2019 16:17, Andrew Lunn wrote:
> > On Tue, Aug 13, 2019 at 10:58:17AM +0200, Antoine Tenart wrote:
> >> I think this question is linked to the use of a MACsec virtual interface
> >> when using h/w offloading. The starting point for me was that I wanted
> >> to reuse the data structures and the API exposed to the userspace by the
> >> s/w implementation of MACsec. I then had two choices: keeping the exact
> >> same interface for the user (having a virtual MACsec interface), or
> >> registering the MACsec genl ops onto the real net devices (and making
> >> the s/w implementation a virtual net dev and a provider of the MACsec
> >> "offloading" ops).
> >>
> >> The advantages of the first option were that nearly all the logic of the
> >> s/w implementation could be kept and especially that it would be
> >> transparent for the user to use both implementations of MACsec.
> >
> > Hi Antoine
> >
> > We have always talked about offloading operations to the hardware,
> > accelerating what the linux stack can do by making use of hardware
> > accelerators. The basic user API should not change because of
> > acceleration. Those are the general guidelines.
> >
> > It would however be interesting to get comments from those who did the
> > software implementation and what they think of this architecture. I've
> > no personal experience with MACSec, so it is hard for me to say if the
> > current architecture makes sense when using accelerators.
>
> In terms of overall concepts, I'd add the following:
>
> 1) With current implementation it's impossible to install SW macsec engine onto
> the device which supports HW offload.

You mean how it's implemented in this patchset?

> That could be a strong limitation in
> cases when user sees HW macsec offload is broken or work differently, and he/she
> wants to replace it with SW one.

Agreed, I think an offload that cannot be disabled is quite problematic.

> MACSec is a complex feature, and it may happen something is missing in HW.
> Trivial example is 256bit encryption, which is not always a musthave in HW
> implementations.

+1

> 2) I think, Antoine, its not totally true that otherwise the user macsec API
> will be broken/changed. netlink api is the same, the only thing we may want to
> add is an optional parameter to force selection of SW macsec engine.

Yes, I think we need an offload on/off parameter (and IMO it should
probably be off by default). Then, if offloading is requested but
cannot be satisfied (unsupported key length, too many SAs, etc), or if
incompatible settings are requested (mixing offloaded and
non-offloaded SCs on a device that cannot do it), return an error.

If we also export that offload parameter during netlink dumps, we can
inspect the state of the system, which helps for debugging.

> I'm also eager to hear from sw macsec users/devs on whats better here.

I don't do much development on MACsec these days, and I don't
personally use it outside of testing and development.

--
Sabrina

2019-08-20 10:04:29

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi Sabrina,

On Fri, Aug 16, 2019 at 03:29:59PM +0200, Sabrina Dubroca wrote:
> 2019-08-13, 16:18:40 +0000, Igor Russkikh wrote:
> > On 13.08.2019 16:17, Andrew Lunn wrote:
>
> > That could be a strong limitation in
> > cases when user sees HW macsec offload is broken or work differently, and he/she
> > wants to replace it with SW one.
>
> Agreed, I think an offload that cannot be disabled is quite problematic.
>
> > MACSec is a complex feature, and it may happen something is missing in HW.
> > Trivial example is 256bit encryption, which is not always a musthave in HW
> > implementations.
>
> +1
>
> > 2) I think, Antoine, its not totally true that otherwise the user macsec API
> > will be broken/changed. netlink api is the same, the only thing we may want to
> > add is an optional parameter to force selection of SW macsec engine.
>
> Yes, I think we need an offload on/off parameter (and IMO it should
> probably be off by default). Then, if offloading is requested but
> cannot be satisfied (unsupported key length, too many SAs, etc), or if
> incompatible settings are requested (mixing offloaded and
> non-offloaded SCs on a device that cannot do it), return an error.
>
> If we also export that offload parameter during netlink dumps, we can
> inspect the state of the system, which helps for debugging.

So it seems the ability to enable or disable the offloading on a given
interface is the main missing feature. I'll add that, however I'll
probably (at least at first):

- Have the interface to be fully offloaded or fully handled in s/w (with
errors being thrown if a given configuration isn't supported). Having
both at the same time on a given interface would be tricky because of
the MACsec validation parameter.

- Won't allow to enable/disable the offloading of there are rules in
place, as we're not sure the same rules would be accepted by the other
implementation.

I'm not sure if we should allow to mix the implementations on a given
physical interface (by having two MACsec interfaces attached) as the
validation would be impossible to do (we would have no idea if a
packet was correctly handled by the offloading part or just not being
a MACsec packet in the first place, in Rx).

I agree the offloading should be disabled by default, and only enabled
by an user explicitly.

Thanks,
Antoine

--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2019-08-20 10:05:02

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi Andrew,

On Tue, Aug 13, 2019 at 06:28:23PM +0200, Andrew Lunn wrote:
> > 1) With current implementation it's impossible to install SW macsec engine onto
> > the device which supports HW offload. That could be a strong limitation in
> > cases when user sees HW macsec offload is broken or work differently, and he/she
> > wants to replace it with SW one.
> > MACSec is a complex feature, and it may happen something is missing in HW.
> > Trivial example is 256bit encryption, which is not always a musthave in HW
> > implementations.
>
> It would also be nice to add extra information to the netlink API to
> indicate if HW or SW is being used. In other places where we offload
> to accelerators we have such additional information.

Agreed, in addition to being able to enable/disable the offloading we
should have a way to know if a MACsec interface is being offloaded or
not.

Thanks!
Antoine

--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2019-08-20 10:08:34

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi Sabrina,

On Fri, Aug 16, 2019 at 03:25:00PM +0200, Sabrina Dubroca wrote:
> 2019-08-13, 10:58:17 +0200, Antoine Tenart wrote:
> >
> > As for the need for xmit / handle_frame ops (for a MAC w/ MACsec
> > offloading), I'd say the xmit / handle_frame ops of the real net device
> > driver could be used as the one of the MACsec virtual interface do not
> > do much (regardless of the implementation choice discussed above).
>
> There's no "handle_frame" op on a real device. macsec_handle_frame is
> an rx_handler specificity that grabs packets from a real device and
> sends them into a virtual device stacked on top of it. A real device
> just hands packets over to the stack via NAPI.

Right, so if there is a need for a custom implementation of xmit /
hamdle_frame we could let the offloading driver provide it. I'll
probably let the first MAC implementation add it, as we agreed not to
add an API with no user (and mine is done in the PHY).

Thanks!
Antoine

--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2019-08-20 14:44:34

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

2019-08-20, 12:01:40 +0200, Antoine Tenart wrote:
> So it seems the ability to enable or disable the offloading on a given
> interface is the main missing feature. I'll add that, however I'll
> probably (at least at first):
>
> - Have the interface to be fully offloaded or fully handled in s/w (with
> errors being thrown if a given configuration isn't supported). Having
> both at the same time on a given interface would be tricky because of
> the MACsec validation parameter.
>
> - Won't allow to enable/disable the offloading of there are rules in
> place, as we're not sure the same rules would be accepted by the other
> implementation.

That's probably quite problematic actually, because to do that you
need to be able to resync the state between software and hardware,
particularly packet numbers. So, yeah, we're better off having it
completely blocked until we have a working implementation (if that
ever happens).

However, that means in case the user wants to set up something that's
not offloadable, they'll have to:
- configure the offloaded version until EOPNOTSUPP
- tear everything down
- restart from scratch without offloading

That's inconvenient.

Talking about packet numbers, can you describe how PN exhaustion is
handled? I couldn't find much about packet numbers at all in the
driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
the same SA). At some point userspace needs to know that we're
getting close to 2^32 and that it's time to re-key. Since the whole
TX path of the software implementation is bypassed, it looks like the
PN (as far as drivers/net/macsec.c is concerned) never increases, so
userspace can't know when to negotiate a new SA.

> I'm not sure if we should allow to mix the implementations on a given
> physical interface (by having two MACsec interfaces attached) as the
> validation would be impossible to do (we would have no idea if a
> packet was correctly handled by the offloading part or just not being
> a MACsec packet in the first place, in Rx).

That's something that really bothers me with this proposal. Quoting
from the commit message:

> the packets seen by the networking stack on both the physical and
> MACsec virtual interface are exactly the same

If the HW/driver is expected to strip the sectag, I don't see how we
could ever have multiple secy's at the same time and demultiplex
properly between them. That's part of the reason why I chose to have
virtual interfaces: without them, picking the right secy on TX gets
weird.

AFAICT, it means that any filters (tc, tcpdump) need to be different
between offloaded and non-offloaded cases.

And it's going to be confusing to the administrator when they look at
tcpdumps expecting to see MACsec frames.

I don't see how this implementation handles non-macsec traffic (on TX,
that would be packets sent directly through the real interface, for
example by wpa_supplicant - on RX, incoming MKA traffic for
wpa_supplicant). Unless I missed something, incoming MKA traffic will
end up on the macsec interface as well as the lower interface (not
entirely critical, as long as wpa_supplicant can grab it on the lower
device, but not consistent with the software implementation). How does
the driver distinguish traffic that should pass through unmodified
from traffic that the HW needs to encapsulate and encrypt?

If you look at IPsec offloading, the networking stack builds up the
ESP header, and passes the unencrypted data down to the driver. I'm
wondering if the same would be possible with MACsec offloading: the
macsec virtual interface adds the header (and maybe a dummy ICV), and
then the HW does the encryption. In case of HW that needs to add the
sectag itself, the driver would first strip the headers that the stack
created. On receive, the driver would recreate a sectag and the macsec
interface would just skip all verification (decrypt, PN).

--
Sabrina

2019-08-21 00:31:20

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

> If you look at IPsec offloading, the networking stack builds up the
> ESP header, and passes the unencrypted data down to the driver. I'm
> wondering if the same would be possible with MACsec offloading: the
> macsec virtual interface adds the header (and maybe a dummy ICV), and
> then the HW does the encryption. In case of HW that needs to add the
> sectag itself, the driver would first strip the headers that the stack
> created. On receive, the driver would recreate a sectag and the macsec
> interface would just skip all verification (decrypt, PN).

Hi Sabrina

I assume the software implementation cannot make use of TSO or GSO,
letting the hardware segment a big buffer up into Ethernet frames?
When using hardware MACSEC, is it possible to enable these? By the
time the frames have reach the PHY GSO has been done. So it sees a
stream of frames it needs to encode/decode.

But if you are suggesting the extra headers are added by the virtual
interface, i don't think GSO can be used? My guess would be, we get a
performance boost from using hardware MAC sec, but there will also be
a performance boost if GSO can be enabled when it was disabled before.

Andrew

2019-08-21 09:56:02

by [email protected]

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi,

I can add some information to the HW Antoine is working on, general design of it
and the thoughts behind it. See below.

The 08/20/2019 16:41, Sabrina Dubroca wrote:
> 2019-08-20, 12:01:40 +0200, Antoine Tenart wrote:
> > So it seems the ability to enable or disable the offloading on a given
> > interface is the main missing feature. I'll add that, however I'll
> > probably (at least at first):
> >
> > - Have the interface to be fully offloaded or fully handled in s/w (with
> > errors being thrown if a given configuration isn't supported). Having
> > both at the same time on a given interface would be tricky because of
> > the MACsec validation parameter.
> >
> > - Won't allow to enable/disable the offloading of there are rules in
> > place, as we're not sure the same rules would be accepted by the other
> > implementation.
>
> That's probably quite problematic actually, because to do that you
> need to be able to resync the state between software and hardware,
> particularly packet numbers. So, yeah, we're better off having it
> completely blocked until we have a working implementation (if that
> ever happens).
>
> However, that means in case the user wants to set up something that's
> not offloadable, they'll have to:
> - configure the offloaded version until EOPNOTSUPP
> - tear everything down
> - restart from scratch without offloading
>
> That's inconvenient.
>
> Talking about packet numbers, can you describe how PN exhaustion is
> handled? I couldn't find much about packet numbers at all in the
> driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
> the same SA).
New SA's are suppose to be installed ahead of time. The HW will automatic move
to the next SA and reset the PN.

> At some point userspace needs to know that we're
> getting close to 2^32 and that it's time to re-key. Since the whole
> TX path of the software implementation is bypassed, it looks like the
> PN (as far as drivers/net/macsec.c is concerned) never increases, so
> userspace can't know when to negotiate a new SA.
>
> > I'm not sure if we should allow to mix the implementations on a given
> > physical interface (by having two MACsec interfaces attached) as the
> > validation would be impossible to do (we would have no idea if a
> > packet was correctly handled by the offloading part or just not being
> > a MACsec packet in the first place, in Rx).
>
> That's something that really bothers me with this proposal. Quoting
> from the commit message:
>
> > the packets seen by the networking stack on both the physical and
> > MACsec virtual interface are exactly the same
>
> If the HW/driver is expected to strip the sectag, I don't see how we
> could ever have multiple secy's at the same time and demultiplex
> properly between them. That's part of the reason why I chose to have
> virtual interfaces: without them, picking the right secy on TX gets
> weird.

The HW does frame clasification, and use the claisfication to associate frames
to a given secy.

We we in SW have eth0, with 2 vlan-sub interfaces, and enable macsec on those,
then we have:

eth0
eth0.10
eth0.10.macsec
eth0.20
eth0.20.macsec

In this case the HW needs to be configured to match vlan 10 to one secy, and
vlan 20 to an other one.

This is nor supported in the current patch, but is something we can add later.
We just wanted to get the basic functionallity done right before moving on to
this.

But in the current design, there is nothing that prevent us from adding this.

If anyone is interested in the details of this then it is described in section
3.6.3 in http://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10455.pdf

It is possible to construct an encapsulation that the HW can not classify
correctly. If that is the case, then we should reject the HW offload, and use
the SW.

But it is a good point, which I think is missing. We should properly reject
MACsec HW offload (with this driver, in this state) on virtual interfaces, if
the encapsulation can not be handled (could start by reject all virtual
interfaces)

> AFAICT, it means that any filters (tc, tcpdump) need to be different
> between offloaded and non-offloaded cases.
It will see the result of the offloaded operation. But I guess that this is no
different from when 'tc' operations are offloaded to HW.

> How does the driver distinguish traffic that should pass through unmodified
> from traffic that the HW needs to encapsulate and encrypt?
It relay on frame classification (I think Antoine is missing a flow
configuration to bypass all MKA traffic).

> If you look at IPsec offloading, the networking stack builds up the
> ESP header, and passes the unencrypted data down to the driver. I'm
> wondering if the same would be possible with MACsec offloading: the
> macsec virtual interface adds the header (and maybe a dummy ICV), and
> then the HW does the encryption. In case of HW that needs to add the
> sectag itself, the driver would first strip the headers that the stack
> created. On receive, the driver would recreate a sectag and the macsec
> interface would just skip all verification (decrypt, PN).
I do not think this is possible with this HW, nor do I think this is desirable.

One of the big differences between MACsec and IPsec, is the fact that it is a L2
protocol, designed to be running on switches (this is how MACsec claims to limit
key-distributions issues, as all frames are decrypted when entering a switch,
and encrypted with a new key when leaving).

If a SW stack needs to add a tag to the frame, then we cannot offload traffic
being bridged in HW, and never goes to the CPU.

/Allan

2019-08-21 09:57:34

by [email protected]

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

The 08/21/2019 09:20, Igor Russkikh wrote:
> > Talking about packet numbers, can you describe how PN exhaustion is
> > handled? I couldn't find much about packet numbers at all in the
> > driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
> > the same SA). At some point userspace needs to know that we're
> > getting close to 2^32 and that it's time to re-key. Since the whole
> > TX path of the software implementation is bypassed, it looks like the
> > PN (as far as drivers/net/macsec.c is concerned) never increases, so
> > userspace can't know when to negotiate a new SA.
>
> I think there should be driver specific implementation of this functionality.
> As an example, our macsec HW issues an interrupt towards the host to indicate
> PN threshold has reached and it's time for userspace to change the keys.
>
> In contrast, current SW macsec implementation just stops this SA/secy.
>
> > I don't see how this implementation handles non-macsec traffic (on TX,
> > that would be packets sent directly through the real interface, for
> > example by wpa_supplicant - on RX, incoming MKA traffic for
> > wpa_supplicant). Unless I missed something, incoming MKA traffic will
> > end up on the macsec interface as well as the lower interface (not
> > entirely critical, as long as wpa_supplicant can grab it on the lower
> > device, but not consistent with the software implementation). How does
> > the driver distinguish traffic that should pass through unmodified
> > from traffic that the HW needs to encapsulate and encrypt?
>
> I can comment on our HW engine - where it has special bypass rules
> for configured ethertypes. This way macsec engine skips encryption on TX and
> passes in RX unencrypted for the selected control packets.
In our case it is a TCAM which can look at various fields (including ethertype),
but is sounds like we have a vary similar design.

> But thats true, realdev driver is hard to distinguish encrypted/unencrypted
> packets. In case realdev should make a decision where to put RX packet,
> it only may do some heuristic (since after macsec decription all the
> macsec related info is dropped. Thats true at least for our HW implementation).
Same for ours.

> > If you look at IPsec offloading, the networking stack builds up the
> > ESP header, and passes the unencrypted data down to the driver. I'm
> > wondering if the same would be possible with MACsec offloading: the
> > macsec virtual interface adds the header (and maybe a dummy ICV), and
> > then the HW does the encryption. In case of HW that needs to add the
> > sectag itself, the driver would first strip the headers that the stack
> > created. On receive, the driver would recreate a sectag and the macsec
> > interface would just skip all verification (decrypt, PN).
>
> I don't think this way is good, as driver have to do per packet header mangling.
> That'll harm linerate performance heavily.
Agree, and it will also prevent MACsec offload in offloaded bridge devices.

/Allan

2019-08-21 10:26:34

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

>
> Talking about packet numbers, can you describe how PN exhaustion is
> handled? I couldn't find much about packet numbers at all in the
> driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
> the same SA). At some point userspace needs to know that we're
> getting close to 2^32 and that it's time to re-key. Since the whole
> TX path of the software implementation is bypassed, it looks like the
> PN (as far as drivers/net/macsec.c is concerned) never increases, so
> userspace can't know when to negotiate a new SA.

I think there should be driver specific implementation of this functionality.
As an example, our macsec HW issues an interrupt towards the host to indicate
PN threshold has reached and it's time for userspace to change the keys.

In contrast, current SW macsec implementation just stops this SA/secy.

> I don't see how this implementation handles non-macsec traffic (on TX,
> that would be packets sent directly through the real interface, for
> example by wpa_supplicant - on RX, incoming MKA traffic for
> wpa_supplicant). Unless I missed something, incoming MKA traffic will
> end up on the macsec interface as well as the lower interface (not
> entirely critical, as long as wpa_supplicant can grab it on the lower
> device, but not consistent with the software implementation). How does
> the driver distinguish traffic that should pass through unmodified
> from traffic that the HW needs to encapsulate and encrypt?

I can comment on our HW engine - where it has special bypass rules
for configured ethertypes. This way macsec engine skips encryption on TX and
passes in RX unencrypted for the selected control packets.

But thats true, realdev driver is hard to distinguish encrypted/unencrypted
packets. In case realdev should make a decision where to put RX packet,
it only may do some heuristic (since after macsec decription all the
macsec related info is dropped. Thats true at least for our HW implementation).

> If you look at IPsec offloading, the networking stack builds up the
> ESP header, and passes the unencrypted data down to the driver. I'm
> wondering if the same would be possible with MACsec offloading: the
> macsec virtual interface adds the header (and maybe a dummy ICV), and
> then the HW does the encryption. In case of HW that needs to add the
> sectag itself, the driver would first strip the headers that the stack
> created. On receive, the driver would recreate a sectag and the macsec
> interface would just skip all verification (decrypt, PN).

I don't think this way is good, as driver have to do per packet header mangling.
That'll harm linerate performance heavily.

Regards,
Igor

2019-08-21 10:44:20

[permalink] [raw]

Subject: Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

Hi Sabrina,

On Tue, Aug 20, 2019 at 04:41:19PM +0200, Sabrina Dubroca wrote:
> 2019-08-20, 12:01:40 +0200, Antoine Tenart wrote:
> > So it seems the ability to enable or disable the offloading on a given
> > interface is the main missing feature. I'll add that, however I'll
> > probably (at least at first):
> >
> > - Have the interface to be fully offloaded or fully handled in s/w (with
> > errors being thrown if a given configuration isn't supported). Having
> > both at the same time on a given interface would be tricky because of
> > the MACsec validation parameter.
> >
> > - Won't allow to enable/disable the offloading of there are rules in
> > place, as we're not sure the same rules would be accepted by the other
> > implementation.
>
> That's probably quite problematic actually, because to do that you
> need to be able to resync the state between software and hardware,
> particularly packet numbers. So, yeah, we're better off having it
> completely blocked until we have a working implementation (if that
> ever happens).
>
> However, that means in case the user wants to set up something that's
> not offloadable, they'll have to:
> - configure the offloaded version until EOPNOTSUPP
> - tear everything down
> - restart from scratch without offloading
>
> That's inconvenient.

That's right, the user might have to replay the whole configuration if
on rule failed to match the h/w requirements. It's inconvenient, but I
think it's better to be safe until we have (if that happens) a working
implementation of synchronizing the rules' state.

> Talking about packet numbers, can you describe how PN exhaustion is
> handled? I couldn't find much about packet numbers at all in the
> driver patches (I hope the hw doesn't wrap around from 2^32-1 to 0 on
> the same SA). At some point userspace needs to know that we're
> getting close to 2^32 and that it's time to re-key. Since the whole
> TX path of the software implementation is bypassed, it looks like the
> PN (as far as drivers/net/macsec.c is concerned) never increases, so
> userspace can't know when to negotiate a new SA.

That's a very good point. It actually was on my todo list for the next
version (I wanted to discuss the other points first). We would also
need to sync the stats at some point.

> > I'm not sure if we should allow to mix the implementations on a given
> > physical interface (by having two MACsec interfaces attached) as the
> > validation would be impossible to do (we would have no idea if a
> > packet was correctly handled by the offloading part or just not being
> > a MACsec packet in the first place, in Rx).
>
> That's something that really bothers me with this proposal. Quoting
> from the commit message:
>
> > the packets seen by the networking stack on both the physical and
> > MACsec virtual interface are exactly the same

That bothers me as well.

> If the HW/driver is expected to strip the sectag, I don't see how we
> could ever have multiple secy's at the same time and demultiplex
> properly between them. That's part of the reason why I chose to have
> virtual interfaces: without them, picking the right secy on TX gets
> weird.
>
> AFAICT, it means that any filters (tc, tcpdump) need to be different
> between offloaded and non-offloaded cases.
>
> And it's going to be confusing to the administrator when they look at
> tcpdumps expecting to see MACsec frames.

Right. I did not see how *not* to strip the sectag in the h/w back then,
I'll have another look because that would improve things a lot.

@all: do other MACsec offloading hardware allow not to stip the sectag?

> I don't see how this implementation handles non-macsec traffic (on TX,
> that would be packets sent directly through the real interface, for
> example by wpa_supplicant - on RX, incoming MKA traffic for
> wpa_supplicant). Unless I missed something, incoming MKA traffic will
> end up on the macsec interface as well as the lower interface (not
> entirely critical, as long as wpa_supplicant can grab it on the lower
> device, but not consistent with the software implementation).

That's right, as we have no way to tell if an Rx packet was MACsec or
non-MACsec traffic, both will end up on both interfaces. Some h/w may be
able to insert a custom header (or may allow not to strip the sectag),
but I did not find anything related to this in mine (I'll double check).

> How does the driver distinguish traffic that should pass through
> unmodified from traffic that the HW needs to encapsulate and encrypt?

At least in PHYs, packets go in a classification unit (that can match on
multiple parts of the packet, given the hardware capabilities, eg. the
MAC addresses). The result of the match is an action, which can be
"bypass the MACsec block" or "go through it (which links the packet to a
given configuration)". This is done in Rx and in Tx, and this is how the
h/w block will know what to encapsulate and encrypt.

Thanks,
Antoine

--
Antoine T?nart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2019-08-21 12:02:56