Re: [endianness bug] cxgb4: mk_act_open_req() buggers ->{local,peer}_ip on big-endian hosts

[Al Viro <viro@ZenIV.linux.org.uk>]

	How can cxgb4/cxgb4_tc_flower.c handling of 16bit
fields possibly work on b-e?  Look:
        case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
                switch (offset) {
                case PEDIT_TCP_SPORT_DPORT:
                        if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
                                offload_pedit(fs, cpu_to_be32(val) >> 16,
                                              cpu_to_be32(mask) >> 16,
                                              TCP_SPORT);

OK, we are feeding two results of >> 16 (i.e. the values in
range 0..65535 from the host POV) to offload_pedit().  Which does

static void offload_pedit(struct ch_filter_specification *fs, u32 val, u32 mask,
                          u8 field)
{
        u32 set_val = val & ~mask;

OK, it's a value in range 0..65535.

        u32 offset = 0;
        u8 size = 1;
        int i;

        for (i = 0; i < ARRAY_SIZE(pedits); i++) {
                if (pedits[i].field == field) {
go until we finally find this:
        PEDIT_FIELDS(TCP_, SPORT, 2, nat_fport, 0),
i.e.
        {TCP_SPORT, 2, offsetof(struct ch_filter_specification, nat_fport)}
                        offset = pedits[i].offset;
                        size = pedits[i].size;
... resulting in offset = offsetof(..., nat_fport), size = 2
                        break;
                }
        }
        memcpy((u8 *)fs + offset, &set_val, size);
... and we copy the first two bytes of set_val to fs->nat_fport, right?

On little-endian, assuming that val & 0xffff was 256 * V0 + V1 and
mask & 0xffff - 256 * M0 + M1, we get cpu_to_be32(val) >> 16 equal to
256 * V1 + V0, and similar for mask, resuling in set_val containing
{V0 & ~M0, V1 & ~M1, 0, 0}, with the first two bytes copied to fs->nat_fport.

Now, think what will happen on big-endian.  The value in set_val has upper
16 bits all zero, no matter what - shift anything 32bit down by 16 and you'll
get that.  And on big-endian that's first two bytes of memory representation,
so this memcpy() is absolutely guaranteed to set fs->nat_fport to zero.
No matter how fancy the hardware is, it can't guess what had the other two
bytes been - CPU has discarded those before the NIC had a chance to see
them.

Am I right assuming that the val is supposed to be {S1, S0, D1, D0},
with sport == S1 * 256 + S0, dport == D1 * 256 + D0?  If so, the following
ought to work [== COMPLETELY UNTESTED, in other words] on l-e same as the
current code does and do the right thing on b-e.  Objections?

offload_pedit() is broken for big-endian; it's actually easier to spell the
memcpy (and in case of ports - memcpy-with-byteswap) explicitly, avoiding
both the b-e problems and getting rid of a lot of LoC, including an unpleasant
macro.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index 3db969eefba9..020ca0121fb4 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -43,27 +43,6 @@
 
 #define STATS_CHECK_PERIOD (HZ / 2)
 
-static struct ch_tc_pedit_fields pedits[] = {
-	PEDIT_FIELDS(ETH_, DMAC_31_0, 4, dmac, 0),
-	PEDIT_FIELDS(ETH_, DMAC_47_32, 2, dmac, 4),
-	PEDIT_FIELDS(ETH_, SMAC_15_0, 2, smac, 0),
-	PEDIT_FIELDS(ETH_, SMAC_47_16, 4, smac, 2),
-	PEDIT_FIELDS(IP4_, SRC, 4, nat_fip, 0),
-	PEDIT_FIELDS(IP4_, DST, 4, nat_lip, 0),
-	PEDIT_FIELDS(IP6_, SRC_31_0, 4, nat_fip, 0),
-	PEDIT_FIELDS(IP6_, SRC_63_32, 4, nat_fip, 4),
-	PEDIT_FIELDS(IP6_, SRC_95_64, 4, nat_fip, 8),
-	PEDIT_FIELDS(IP6_, SRC_127_96, 4, nat_fip, 12),
-	PEDIT_FIELDS(IP6_, DST_31_0, 4, nat_lip, 0),
-	PEDIT_FIELDS(IP6_, DST_63_32, 4, nat_lip, 4),
-	PEDIT_FIELDS(IP6_, DST_95_64, 4, nat_lip, 8),
-	PEDIT_FIELDS(IP6_, DST_127_96, 4, nat_lip, 12),
-	PEDIT_FIELDS(TCP_, SPORT, 2, nat_fport, 0),
-	PEDIT_FIELDS(TCP_, DPORT, 2, nat_lport, 0),
-	PEDIT_FIELDS(UDP_, SPORT, 2, nat_fport, 0),
-	PEDIT_FIELDS(UDP_, DPORT, 2, nat_lport, 0),
-};
-
 static struct ch_tc_flower_entry *allocate_flower_entry(void)
 {
 	struct ch_tc_flower_entry *new = kzalloc(sizeof(*new), GFP_KERNEL);
@@ -306,81 +285,63 @@ static int cxgb4_validate_flow_match(struct net_device *dev,
 	return 0;
 }
 
-static void offload_pedit(struct ch_filter_specification *fs, u32 val, u32 mask,
-			  u8 field)
-{
-	u32 set_val = val & ~mask;
-	u32 offset = 0;
-	u8 size = 1;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(pedits); i++) {
-		if (pedits[i].field == field) {
-			offset = pedits[i].offset;
-			size = pedits[i].size;
-			break;
-		}
-	}
-	memcpy((u8 *)fs + offset, &set_val, size);
-}
-
-static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
-				u32 mask, u32 offset, u8 htype)
+static void process_pedit_field(struct ch_filter_specification *fs, __be32 val,
+				__be32 mask, u32 offset, u8 htype)
 {
+	val &= ~mask;
 	switch (htype) {
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
 		switch (offset) {
 		case PEDIT_ETH_DMAC_31_0:
 			fs->newdmac = 1;
-			offload_pedit(fs, val, mask, ETH_DMAC_31_0);
+			memcpy(fs->dmac, &val, 4);
 			break;
 		case PEDIT_ETH_DMAC_47_32_SMAC_15_0:
 			if (~mask & PEDIT_ETH_DMAC_MASK)
-				offload_pedit(fs, val, mask, ETH_DMAC_47_32);
+				memcpy(fs->dmac + 4, &val, 2);
 			else
-				offload_pedit(fs, val >> 16, mask >> 16,
-					      ETH_SMAC_15_0);
+				memcpy(fs->smac, (__be16 *)&val + 1, 2);
 			break;
 		case PEDIT_ETH_SMAC_47_16:
 			fs->newsmac = 1;
-			offload_pedit(fs, val, mask, ETH_SMAC_47_16);
+			memcpy(fs->smac + 2, &val, 4);
 		}
 		break;
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
 		switch (offset) {
 		case PEDIT_IP4_SRC:
-			offload_pedit(fs, val, mask, IP4_SRC);
+			memcpy(fs->nat_fip, &val, 4);
 			break;
 		case PEDIT_IP4_DST:
-			offload_pedit(fs, val, mask, IP4_DST);
+			memcpy(fs->nat_lip, &val, 4);
 		}
 		fs->nat_mode = NAT_MODE_ALL;
 		break;
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
 		switch (offset) {
 		case PEDIT_IP6_SRC_31_0:
-			offload_pedit(fs, val, mask, IP6_SRC_31_0);
+			memcpy(fs->nat_fip, &val, 4);
 			break;
 		case PEDIT_IP6_SRC_63_32:
-			offload_pedit(fs, val, mask, IP6_SRC_63_32);
+			memcpy(fs->nat_fip + 4, &val, 4);
 			break;
 		case PEDIT_IP6_SRC_95_64:
-			offload_pedit(fs, val, mask, IP6_SRC_95_64);
+			memcpy(fs->nat_fip + 8, &val, 4);
 			break;
 		case PEDIT_IP6_SRC_127_96:
-			offload_pedit(fs, val, mask, IP6_SRC_127_96);
+			memcpy(fs->nat_fip + 12, &val, 4);
 			break;
 		case PEDIT_IP6_DST_31_0:
-			offload_pedit(fs, val, mask, IP6_DST_31_0);
+			memcpy(fs->nat_lip, &val, 4);
 			break;
 		case PEDIT_IP6_DST_63_32:
-			offload_pedit(fs, val, mask, IP6_DST_63_32);
+			memcpy(fs->nat_lip + 4, &val, 4);
 			break;
 		case PEDIT_IP6_DST_95_64:
-			offload_pedit(fs, val, mask, IP6_DST_95_64);
+			memcpy(fs->nat_lip + 8, &val, 4);
 			break;
 		case PEDIT_IP6_DST_127_96:
-			offload_pedit(fs, val, mask, IP6_DST_127_96);
+			memcpy(fs->nat_lip + 12, &val, 4);
 		}
 		fs->nat_mode = NAT_MODE_ALL;
 		break;
@@ -388,12 +349,9 @@ static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
 		switch (offset) {
 		case PEDIT_TCP_SPORT_DPORT:
 			if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
-				offload_pedit(fs, cpu_to_be32(val) >> 16,
-					      cpu_to_be32(mask) >> 16,
-					      TCP_SPORT);
+				fs->nat_fport = be16_to_cpup((__be16 *)&val);
 			else
-				offload_pedit(fs, cpu_to_be32(val),
-					      cpu_to_be32(mask), TCP_DPORT);
+				fs->nat_lport = be16_to_cpup((__be16 *)&val + 1);
 		}
 		fs->nat_mode = NAT_MODE_ALL;
 		break;
@@ -401,12 +359,9 @@ static void process_pedit_field(struct ch_filter_specification *fs, u32 val,
 		switch (offset) {
 		case PEDIT_UDP_SPORT_DPORT:
 			if (~mask & PEDIT_TCP_UDP_SPORT_MASK)
-				offload_pedit(fs, cpu_to_be32(val) >> 16,
-					      cpu_to_be32(mask) >> 16,
-					      UDP_SPORT);
+				fs->nat_fport = be16_to_cpup((__be16 *)&val);
 			else
-				offload_pedit(fs, cpu_to_be32(val),
-					      cpu_to_be32(mask), UDP_DPORT);
+				fs->nat_lport = be16_to_cpup((__be16 *)&val + 1);
 		}
 		fs->nat_mode = NAT_MODE_ALL;
 	}
@@ -453,7 +408,8 @@ static void cxgb4_process_flow_actions(struct net_device *in,
 				break;
 			}
 		} else if (is_tcf_pedit(a)) {
-			u32 mask, val, offset;
+			__be32 mask, val;
+			u32 offset;
 			int nkeys, i;
 			u8 htype;
 
@@ -471,23 +427,18 @@ static void cxgb4_process_flow_actions(struct net_device *in,
 	}
 }
 
-static bool valid_l4_mask(u32 mask)
+static bool valid_l4_mask(__be32 mask)
 {
-	u16 hi, lo;
-
-	/* Either the upper 16-bits (SPORT) OR the lower
-	 * 16-bits (DPORT) can be set, but NOT BOTH.
+	/* Either the SPORT OR DPORT can be set, but NOT BOTH.
 	 */
-	hi = (mask >> 16) & 0xFFFF;
-	lo = mask & 0xFFFF;
-
-	return hi && lo ? false : true;
+	return !(mask && htonl(0xffff)) || !(mask & htonl(0xffff0000));
 }
 
 static bool valid_pedit_action(struct net_device *dev,
 			       const struct tc_action *a)
 {
-	u32 mask, offset;
+	__be32 mask;
+	u32 offset;
 	u8 cmd, htype;
 	int nkeys, i;
 
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
index 050c8a50ae41..4da5267726a9 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h
@@ -54,44 +54,8 @@ struct ch_tc_flower_entry {
 	u32 filter_id;
 };
 
-enum {
-	ETH_DMAC_31_0,	/* dmac bits 0.. 31 */
-	ETH_DMAC_47_32,	/* dmac bits 32..47 */
-	ETH_SMAC_15_0,	/* smac bits 0.. 15 */
-	ETH_SMAC_47_16,	/* smac bits 16..47 */
-
-	IP4_SRC,	/* 32-bit IPv4 src  */
-	IP4_DST,	/* 32-bit IPv4 dst  */
-
-	IP6_SRC_31_0,	/* src bits 0..  31 */
-	IP6_SRC_63_32,	/* src bits 63.. 32 */
-	IP6_SRC_95_64,	/* src bits 95.. 64 */
-	IP6_SRC_127_96,	/* src bits 127..96 */
-
-	IP6_DST_31_0,	/* dst bits 0..  31 */
-	IP6_DST_63_32,	/* dst bits 63.. 32 */
-	IP6_DST_95_64,	/* dst bits 95.. 64 */
-	IP6_DST_127_96,	/* dst bits 127..96 */
-
-	TCP_SPORT,	/* 16-bit TCP sport */
-	TCP_DPORT,	/* 16-bit TCP dport */
-
-	UDP_SPORT,	/* 16-bit UDP sport */
-	UDP_DPORT,	/* 16-bit UDP dport */
-};
-
-struct ch_tc_pedit_fields {
-	u8 field;
-	u8 size;
-	u32 offset;
-};
-
-#define PEDIT_FIELDS(type, field, size, fs_field, offset) \
-	{ type## field, size, \
-		offsetof(struct ch_filter_specification, fs_field) + (offset) }
-
-#define PEDIT_ETH_DMAC_MASK		0xffff
-#define PEDIT_TCP_UDP_SPORT_MASK	0xffff
+#define PEDIT_ETH_DMAC_MASK		htonl(0xffff0000)
+#define PEDIT_TCP_UDP_SPORT_MASK	htonl(0xffff0000)
 #define PEDIT_ETH_DMAC_31_0		0x0
 #define PEDIT_ETH_DMAC_47_32_SMAC_15_0	0x4
 #define PEDIT_ETH_SMAC_47_16		0x8