2016-03-08, 21:13:53 +0100, Johannes Berg wrote:

On Mon, 2016-03-07 at 18:12 +0100, Sabrina Dubroca wrote:

quoted

+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?

quoted

+/**
+ * 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.

quoted

+/**
+ * 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.

quoted

+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).

quoted

+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().

quoted

+/* minimum secure data length deemed "not short", see IEEE 802.1AE-
2006 9.7 */
+#define MIN_NON_SHORT_LEN 48

I saw

quoted

+#define MACSEC_SHORTLEN_THR 48

before, are they different? Seem very similar.

They are exactly the same, good catch.

quoted

+	tx_sa->next_pn++;
+	if (tx_sa->next_pn == 0) {
+		pr_debug("PN wrapped, transitionning to !oper\n");

typo: transitioning

Fixed.

quoted

+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