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:

quoted

On 08.08.2019 17:05, Antoine Tenart wrote:

quoted

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.

quoted

quoted

@@ -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