On Mon, 2016-03-07 at 18:12 +0100, Sabrina Dubroca wrote:
>
> +struct gcm_iv {
> + union {
> + u8 secure_channel_id[8];
> + sci_t sci;
> + };
> + __be32 pn;
> +};
Should this be __packed?
But the struct is confusing; sci_t is a host type (that depends on
endianness), and __be32 would seem to be a network type, how can they
both be in the same struct? Or does sci_t have to be __be64?
> +/**
> + * struct macsec_rx_sa - receive secure association
> + * @active
> + * @next_pn packet number expected for the next packet
> + * @lock protects next_pn manipulations
> + * @key key structure
> + * @stats per-SA stats
> + */
> +struct macsec_rx_sa {
> + bool active;
> + u32 next_pn;
> + spinlock_t lock;
If you put the spinlock first or at least next to active you can get
rid of some padding (on arches/configs where spinlock is small, at
least)
> +/**
> + * struct macsec_rx_sc - receive secure channel
> + * @sci secure channel identifier for this SC
> + * @active channel is active
> + * @sa array of secure associations
> + * @stats per-SC stats
> + */
Btw, all your kernel-doc comments are actually malformed, they're
missing a colon after the @member, e.g.
@stats: per-SC stats
> +struct macsec_tx_sc {
> + bool active;
> + u8 encoding_sa;
> + bool encrypt;
> + bool send_sci;
> + bool end_station;
> + bool scb;
> + struct macsec_tx_sa __rcu *sa[4];
What's 4?
> +static sci_t make_sci(u8 *addr, __be16 port)
> +{
> + sci_t sci;
> +
> + memcpy(&sci, addr, ETH_ALEN);
> + memcpy(((char *)&sci) + ETH_ALEN, &port, sizeof(port));
> +
> + return sci;
> +}
Oh, maybe this explains my earlier question - looks like the sci_t
isn't really a 64-bit integer but some kind of structure.
Is there really much point in using a __bitwise u64 typedef, rather
than a small packed struct then?
> +/* minimum secure data length deemed "not short", see IEEE 802.1AE-
> 2006 9.7 */
> +#define MIN_NON_SHORT_LEN 48
I saw
> +#define MACSEC_SHORTLEN_THR 48
before, are they different? Seem very similar.
> + tx_sa->next_pn++;
> + if (tx_sa->next_pn == 0) {
> + pr_debug("PN wrapped, transitionning to !oper\n");
typo: transitioning
> +static const struct genl_ops macsec_genl_ops[] = {
> + {
> + .cmd = MACSEC_CMD_GET_TXSC,
> + .dumpit = macsec_dump_txsc,
> + .policy = macsec_genl_policy,
> + },
> + {
> + .cmd = MACSEC_CMD_ADD_RXSC,
> + .doit = macsec_add_rxsc,
> + .policy = macsec_genl_rxsc_policy,
> + .flags = GENL_ADMIN_PERM,
IMHO, having different policies for different operations is pretty
confusing. I now slowly start to understand why you had to do all this
aliasing with the IDs. However, perhaps it'd be better to define a top-
level attribute list with the ifindex etc., and make all the
*additional* data needed for RXSC operations for example go into a
nested attribute in the top-level.
That way, you have the same policy for everything and also don't have
to play tricks with the aliasing since the top-level attributes
actually exist now, coming from the same namespace & policy.
johannes
