Re: [PATCH net-next 06/10] net: introduce a net_device_ops macsec helper

[Florian Fainelli]

On 1/24/19 1:23 AM, Antoine Tenart wrote:
> Hi Florian,
> 
> On Wed, Jan 23, 2019 at 12:16:08PM -0800, Florian Fainelli wrote:
>> On 1/23/19 7:56 AM, Antoine Tenart wrote:
>>> This patch introduces a net_device_ops MACsec helper to allow net device
>>> drivers to implement a MACsec offloading solution.
>>>
>>> Signed-off-by: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
>>> ---
>>>  include/linux/netdevice.h | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index e675ef97a426..ee2f40dca515 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -53,6 +53,10 @@
>>>  #include <uapi/linux/pkt_cls.h>
>>>  #include <linux/hashtable.h>
>>>  
>>> +#ifdef CONFIG_MACSEC
>>> +#include <net/macsec.h>
>>> +#endif
>>
>> You can provide a forward declaration for struct netdev_macsec and not
>> have to include that header file.
> 
> OK.
> 
>>> +
>>>  struct netpoll_info;
>>>  struct device;
>>>  struct phy_device;
>>> @@ -1441,6 +1445,10 @@ struct net_device_ops {
>>>  						u32 flags);
>>>  	int			(*ndo_xsk_async_xmit)(struct net_device *dev,
>>>  						      u32 queue_id);
>>> +#ifdef CONFIG_MACSEC
>>> +	int			(*ndo_macsec)(struct net_device *dev,
>>> +					      struct netdev_macsec *macsec);
>>
>> You would really want to define an API which is more oriented towards
>> configuring/deconfiguring a MACsec association here, e.g.: similar to
>> what the IPsec offload ndos offer.
> 
> This means mostly moving from a single function using a command field to
> multiple specialized functions to add/remove each element of MACsec
> configuration.
> 
> I don't have strong opinion on the single helper vs a structure
> containing pointers to specialized ones, but out of curiosity what's the
> benefit of such a move? Future additions and maintainability?

Having multiple operations typically allows for better granularity when
you have HW that may not be capable of offloading an entire protocol
that way you can easily implement fallbacks within the core of that
protocol handling in Linux.

Maybe if you just rename this netdev_macsec_context that will make it
clearer what this does.

> 
>> It is not clear to me whether after your patch series we still need to
>> create a macsec virtual device, and that gets offloaded onto its real
>> device/PHY device, or if we don't need that all?
> 
> After this series, we will still need the virtual MACsec interface. When
> using hardware offloading this interface isn't doing much, but it's the
> interface used to configure all the MACsec connexions.

By not doing much, you mean its data path is basically unused? That
would be quite a deviation from any other type of offload that Linux has
AFAICT, for instance on VLAN devices you still have some amount of data
on the VLAN net_device, etc.

> 
> This is because, and that's specific to MACsec (vs IPsec), a software
> implementation is already supported and it's using a virtual interface
> to perform all the MACsec related operations (vs hooks in the Rx/Tx
> paths). I really wanted to avoid having two interfaces and ways of
> configuring MACsec depending on if the offloading is used.

The virtual network device makes sense when there is some special
treatment (encap/decap, encryption/decryption) that must happen before
sending a frame/PDU onto the wire. It's the same thing here AFAICT, but
since the HW supports doing it in the PHY directly, it's a tough one.

> 
> This should also allow in the future to disable at run-time the
> offloading on a given interface, and to still have MACsec working in
> software (or the opposite, with extra work). For this to work, the
> virtual interface still has to provide an Rx and a Tx functions so that
> programs can bind onto the same interface, regardless of if the
> offloading is enabled.

It would really be good to hear from Sabrina since she authored support
for MACsec to begin with.
-- 
Florian