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
| ||
|
Message-ID: <ee545162-7695-4089-804d-64438e1de620@lunn.ch> Date: Fri, 11 Aug 2023 19:42:14 +0200 From: Andrew Lunn <andrew@...n.ch> To: "Radu Pirea (NXP OSS)" <radu-nicolae.pirea@....nxp.com> Cc: hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, richardcochran@...il.com, sd@...asysnail.net, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [RFC net-next v1 4/5] net: macsec: introduce mdo_insert_tx_tag On Fri, Aug 11, 2023 at 06:32:48PM +0300, Radu Pirea (NXP OSS) wrote: > Offloading MACsec in PHYs requires inserting the SecTAG and the ICV in > the ethernet frame. This operation will increase the frame size with 32 > bytes. If the frames are sent at line rate, the PHY will not have enough > room to insert the SecTAG and the ICV. > > To mitigate this scenario, the PHY offer to use require a specific > ethertype with some padding bytes present in the ethernet frame. This > ethertype and its associated bytes will be replaced by the SecTAG and ICV. I think this could be worded better, to take into account different implementations. As far as i understand, some PHYs include a MAC, which reassembles the frame, and then places the frame into a queue for processing. After processing, a second MAC does the actual send on the wire. The queue allows for some number of back to back frames without having problems. The PHY then uses flow control pause to slow down the SoC MAC when there is a long burst of line rate frames which would otherwise overflow the queue. So: > If the frames are sent at line rate, the PHY will not have enough > room to insert the SecTAG and the ICV. This probably want to clarify that a PHY which does not buffer.... > To mitigate this scenario, the PHY offer to use require a specific and here you want to say some PHYs offer, since not all PHYs will do this. > + if (macsec->offload == MACSEC_OFFLOAD_OFF) { > + dev->needed_headroom -= ops->needed_headroom; > + dev->needed_headroom += MACSEC_NEEDED_HEADROOM; > + dev->needed_tailroom -= ops->needed_tailroom; > + dev->needed_tailroom += MACSEC_NEEDED_TAILROOM; > + } else { > + dev->needed_headroom -= MACSEC_NEEDED_HEADROOM; > + dev->needed_headroom += ops->needed_headroom; > + dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM; > + dev->needed_tailroom += ops->needed_tailroom; > + } It is not obvious to me what this is doing. Should this actually be in macsec_dev_init()? My main problem is why there is an else condition? > +static struct sk_buff *insert_tx_tag(struct sk_buff *skb, > + struct net_device *dev) > +{ > + struct macsec_dev *macsec = macsec_priv(dev); > + const struct macsec_ops *ops; > + struct macsec_context ctx; > + int err; > + > + if (!macsec_is_offloaded(macsec)) > + return ERR_PTR(-EINVAL); Hasn't this already been checked in macsec_start_xmit()? > + > + ops = macsec_get_ops(macsec, &ctx); > + if (!ops) > + return ERR_PTR(-EINVAL); > + > + if (!ops->mdo_insert_tx_tag) > + return skb; You are in the hot path here. You don't expect this to change from frame to frame. So could you evaluate this once and store it somewhere? Maybe in macsec_dev ? Andrew
Powered by blists - more mailing lists