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