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 <v...@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