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
