Re: [PATCH net-next 1/3] uapi: add MACsec bits

[Sabrina Dubroca <sd@queasysnail.net>]

2016-03-08, 20:52:48 +0100, Johannes Berg wrote:
> On Mon, 2016-03-07 at 18:12 +0100, Sabrina Dubroca wrote:
> 
> > +++ b/include/uapi/linux/if_macsec.h
> 
> Some bits of documentation in this file, regarding all the various
> operations and attributes, might be nice. At least the netlink types?

ok. Most of them are already indicated in the policies, but I can add
some comments here.


> E.g., given this:
> 
> > +#define DEFAULT_CIPHER_NAME "GCM-AES-128"
> > +#define DEFAULT_CIPHER_ID   0x0080020001000001ULL
> > +#define DEFAULT_CIPHER_ALT  0x0080C20001000001ULL
> 
> > +enum macsec_attrs {
> [...]
> > +	MACSEC_ATTR_CIPHER_SUITE,
> 
> should that be the ID, the alternate ID, or the string?

uh, the string is never actually used, I could get rid of it.


> > +	MACSEC_ATTR_ICV_LEN,
> > +	MACSEC_TXSA_LIST,
> > +	MACSEC_RXSC_LIST,
> > +	MACSEC_TXSC_STATS,
> > +	MACSEC_SECY_STATS,
> > +	MACSEC_ATTR_PROTECT,
> 
> This seems a bit inconsistent, MACSEC_ATTR_* vs. MACSEC_*?

Only the MACSEC_ATTR_* can be set, the others are just for dumping.


> > +enum macsec_sa_list_attrs {
> > +	MACSEC_SA_LIST_UNSPEC,
> > +	MACSEC_SA,
> > +	__MACSEC_ATTR_SA_LIST_MAX,
> > +	MACSEC_ATTR_SA_LIST_MAX = __MACSEC_ATTR_SA_LIST_MAX - 1,
> > +};
> 
> Again, without documentation, it seems odd to have an enum with just a
> single useful entry? If you just wanted an array you don't need this at
> all? The netlink nesting properties could be specified somewhere.

Yes, in dump_secy(), I nest the TXSA_LIST, and then each SA underneath
it.  I'm not sure how that would work without the list.  Can you have
an array without the dummy level of nesting?


> > +enum macsec_rxsc_list_attrs {
> > +	MACSEC_RXSC_LIST_UNSPEC,
> > +	MACSEC_RXSC,
> 
> similarly here
> 
> > +enum macsec_rxsc_attrs {
> > +	MACSEC_ATTR_SC_UNSPEC,
> > +	/* use the same value to allow generic helper function, see
> > +	 * get_*_from_nl in drivers/net/macsec.c */
> > +	MACSEC_ATTR_SC_IFINDEX = MACSEC_ATTR_IFINDEX,
> > +	MACSEC_ATTR_SC_SCI = MACSEC_ATTR_SCI,
> 
> This seems odd, this must be nested inside the top-level attributes
> since it's a single genl family, so why not use the top-level
> attributes to start with?
> 
> If you need multiple you can use dump with multiple messages.
> 
> > +enum macsec_sa_attrs {
> > +	MACSEC_ATTR_SA_UNSPEC,
> > +	/* use the same value to allow generic helper function, see
> > +	 * get_*_from_nl in drivers/net/macsec.c */
> > +	MACSEC_ATTR_SA_IFINDEX = MACSEC_ATTR_IFINDEX,
> > +	MACSEC_ATTR_SA_SCI = MACSEC_ATTR_SCI,
> 
> likewise here
> 
> > +enum validation_type {
> > +	MACSEC_VALIDATE_DISABLED = 0,
> > +	MACSEC_VALIDATE_CHECK = 1,
> > +	MACSEC_VALIDATE_STRICT = 2,
> > +	__MACSEC_VALIDATE_MAX,
> > +};
> > +#define MACSEC_VALIDATE_MAX (__MACSEC_VALIDATE_MAX - 1)
> 
> everywhere else you put that into the enum, why not here?

Will fix.


> > +struct macsec_rx_sc_stats {
> > +	__u64 InOctetsValidated;
> > +	__u64 InOctetsDecrypted;
> > +	__u64 InPktsUnchecked;
> > +	__u64 InPktsDelayed;
> > +	__u64 InPktsOK;
> > +	__u64 InPktsInvalid;
> > +	__u64 InPktsLate;
> > +	__u64 InPktsNotValid;
> > +	__u64 InPktsNotUsingSA;
> > +	__u64 InPktsUnusedSA;
> > +};
> 
> It might be worth splitting those into attributes so that, if some
> hardware offload can't provide all of the counters in the future, they
> can just be left out of the netlink message?

These stats are defined by the standard, but marked optional.
A hardware device that doesn't implement some stat could just ignore
it and export 0.
I don't have a strong opinion about this.


Thanks,

-- 
Sabrina