[PATCH v2] add new macsec offload modes to abstract device-level details

[Sabrina Dubroca]

2024-03-03, 21:09:13 +0200, Jouni Malinen wrote:
> On Mon, Feb 12, 2024 at 10:03:05AM +0100, Sabrina Dubroca wrote:
> > Currently, users have to guess which offload mode their hardware
> > provides, as the kernel doesn't expose this information.
> > 
> > Add an "on" mode which requires offload (either to mac or phy) and a
> > "prefer" mode which allows fallback to SW when offload (either to mac
> > or phy) is unavailable.
> > 
> > Also rename the existing modes to string values to make the config
> > file a bit clearer.
> 
> Wouldn't that renaming break all existing users? At minimum, there would
> need to be backwards compatibility aliases to allow the previously used
> integers to be used as well as the new names.

Since there hasn't been an official release containing this change, I
thought it was safe to change it (I doubt many people are using it,
especially considering how niche macsec offload is). It's also
*really* not user-friendly with those numbers (on top of having to
know which type of device you have).

But if you insist on compatibility, I'll try to implement it.

> > diff --git a/hostapd/config_file.c b/hostapd/config_file.c
> > +#ifdef CONFIG_MACSEC
> > +const char *macsec_offload_strings[NUM_CONFIG_MACSEC_OFFLOAD_MODES] = {
> > +	"off",
> > +	"on",
> > +	"mac",
> > +	"phy",
> > +	"prefer",
> > +};
> 
> Having this array here as a globally available array and also in
> wpa_supplicant/config.c feels strange.

It is, but I couldn't figure out a reasonable location to put the
array that would allow sharing it between hostapd and
wpa_supplicant. The existing mka files are dealing more with
implementation than config, so it also felt wrong to put that there.

> Maybe a single helper function that maps a string to an enum
> macsec_offload_modes value would seem cleaner. That helper function
> could also include mapping of the old values (0..2) to their
> appropriate new enum values.

I can do that, but I don't know where to put it so that both hostapd
and wpa_supplicant build. src/ap/ap_config.c seems to be compiled into
both and have some config-related code. Would that work for you? If
not, can you suggest something?

> > diff --git a/src/common/ieee802_1x_defs.h b/src/common/ieee802_1x_defs.h
> > index e7acff108eb3..ea964e1dd338 100644
> > --- a/src/common/ieee802_1x_defs.h
> > +++ b/src/common/ieee802_1x_defs.h
> > +enum macsec_offload_modes {
> > +	CONFIG_MACSEC_OFFLOAD_OFF,
> > +	CONFIG_MACSEC_OFFLOAD_ON,
> > +	CONFIG_MACSEC_OFFLOAD_MAC,
> > +	CONFIG_MACSEC_OFFLOAD_PHY,
> > +	CONFIG_MACSEC_OFFLOAD_PREFER,
> > +	NUM_CONFIG_MACSEC_OFFLOAD_MODES,
> > +};
> > +extern const char *macsec_offload_strings[NUM_CONFIG_MACSEC_OFFLOAD_MODES];
> 
> This file looks a bit strange place for implementation internal values.
> I would expect to see values defined in the IEEE standard here..
> Something under src/pae might be more appropriate for this (and the
> shared helper function for string->enum value mapping).

It felt wrong to put it there because src/pae contains implementation,
not config.

> > diff --git a/wpa_supplicant/config_file.c b/wpa_supplicant/config_file.c
> > +static void write_macsec_offload(FILE *f, struct wpa_ssid *ssid)
> > +{
> > +	if (ssid->macsec_offload < ARRAY_SIZE(macsec_offload_strings))
> > +		fprintf(f, "\tmacsecoffload=%s\n", macsec_offload_strings[ssid->macsec_offload]);
> 
> That should have been macsec_offload as the field name (this would
> result in writing a configuration file that would be rejected).

Oops, yes, sorry for the typo.

-- 
Sabrina