2016-03-08, 21:13:53 +0100, Johannes Berg wrote:
> 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?
I think that's not necessary here.
> 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)
Ok, I rearranged macsec_rx_sa and macsec_tx_sa.
> > +/**
> > + * 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
duh, I never noticed that :(
Thanks.
> > +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?
Good point. I added:
#define MACSEC_NUM_AN 4 /* 2 bits for the association number */
and used it in all the range tests (< 4, >= 3).
> > +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.
Yes, the bits in the SCI have some meaning.
> Is there really much point in using a __bitwise u64 typedef, rather
> than a small packed struct then?
I can use == tests, as in find_rx_sc().
> > +/* 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.
They are exactly the same, good catch.
> > + tx_sa->next_pn++;
> > + if (tx_sa->next_pn == 0) {
> > + pr_debug("PN wrapped, transitionning to !oper\n");
>
> typo: transitioning
Fixed.
> > +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.
That's a good idea, I'll have a look today.
I don't really like the aliasing games I have to play, but I'd like to
keep the RXSC attributes separate from the SA attributes, I think it
looks cleaner (the first RFC had everything in the same policy:
http://www.spinics.net/lists/netdev/msg358152.html).
Thanks for the review.
--
Sabrina
