Hi Sugesh
On 6/26/17, 12:51 AM, "Chandran, Sugesh" <[email protected]> wrote:
Hi Darrel,
Please find my answers below.
Regards
_Sugesh
> -----Original Message-----
> From: Darrell Ball [mailto:[email protected]]
> Sent: Saturday, June 24, 2017 10:39 PM
> To: Chandran, Sugesh <[email protected]>; [email protected]
> Subject: Re: [PATCH v2] conntrack : Use Rx checksum offload feature on
> DPDK ports for conntrack.
>
> Hi Sugesh
>
> I have a few questions:
>
> 1) There is no checking for bad hwol determined checksums If the dpdk
> documentation is correct, then this is indicated by both good and bad
flags
> set for both L3 and L4.
[Sugesh] I don’t think that’s the case. At least in my testing, the GOOD
flag is set
only when the checksum is good.
If the checksum is BAD, the BAD flag is set, not both.
Below is the full context I was referring to earlier:
What is your interpretation of:
1) PKT_RX_IP_CKSUM_NONE, which equals PKT_RX_IP_CKSUM_MASK
Packet is good but checksum needs to recalculated and updated in packet ?
2) PKT_RX_L4_CKSUM_NONE which PKT_RX_L4_CKSUM_MASK
Packet is good but checksum needs to recalculated and updated in packet ?
3) The deprecated labellings of PKT_RX_IP_CKSUM_BAD and PKT_RX_L4_CKSUM_BAD ?
**
* Deprecated.
* Checking this flag alone is deprecated: check the 2 bits of
* PKT_RX_IP_CKSUM_MASK.
* This flag was set when the IP checksum of a packet was detected as
* wrong by the hardware.
*/
#define PKT_RX_IP_CKSUM_BAD (1ULL << 4)
/**
* Mask of bits used to determine the status of RX IP checksum.
* - PKT_RX_IP_CKSUM_UNKNOWN: no information about the RX IP checksum
* - PKT_RX_IP_CKSUM_BAD: the IP checksum in the packet is wrong
* - PKT_RX_IP_CKSUM_GOOD: the IP checksum in the packet is valid
* - PKT_RX_IP_CKSUM_NONE: the IP checksum is not correct in the packet
* data, but the integrity of the IP header is verified.
*/
#define PKT_RX_IP_CKSUM_MASK ((1ULL << 4) | (1ULL << 7))
#define PKT_RX_IP_CKSUM_UNKNOWN 0
#define PKT_RX_IP_CKSUM_BAD (1ULL << 4)
#define PKT_RX_IP_CKSUM_GOOD (1ULL << 7)
#define PKT_RX_IP_CKSUM_NONE ((1ULL << 4) | (1ULL << 7))
**
* Deprecated.
* Checking this flag alone is deprecated: check the 2 bits of
* PKT_RX_L4_CKSUM_MASK.
* This flag was set when the L4 checksum of a packet was detected as
* wrong by the hardware.
*/
#define PKT_RX_L4_CKSUM_BAD (1ULL << 3)
/**
* Mask of bits used to determine the status of RX L4 checksum.
* - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
* - PKT_RX_L4_CKSUM_BAD: the L4 checksum in the packet is wrong
* - PKT_RX_L4_CKSUM_GOOD: the L4 checksum in the packet is valid
* - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
* data, but the integrity of the L4 data is verified.
*/
#define PKT_RX_L4_CKSUM_MASK ((1ULL << 3) | (1ULL << 8))
#define PKT_RX_L4_CKSUM_UNKNOWN 0
#define PKT_RX_L4_CKSUM_BAD (1ULL << 3)
#define PKT_RX_L4_CKSUM_GOOD (1ULL << 8)
#define PKT_RX_L4_CKSUM_NONE ((1ULL << 3) | (1ULL << 8))
> 2) If hwol can reliably indicate when a checksum is known to be bad, then
we
> should stop right there and say the packet is bad, which we don’t do here.
> We are checking if it is not known to be good and if not, do checksum
verify
> in software.
[Sugesh] The reason for implementing this way is, reduce number of
validations
So less cycles for validating the flag. Are you suggesting to validating
the bad checksum
flag even for the good case?
No, assuming we can determine the checksum is bad reliably from hwol, we could
check for bad only if not good. High probability of bad checksums might be an
exploit attempt.
> I added a dp-packet API to check for bad, assuming I understood the rte
> documentation and the documentation is accurate.
> Please confirm if a bad checksum can reliably be determined by rte hwol.
> 3) Now, if the checksum is not bad according to hwol, we can proceed to
> check if it is known to be good by hwol. It seems that the existing
dp-packet
> API does not definitely check for good, since if the rte documentation is
> correct, the good flag is set in both good and bad checksum cases, but in
the
> bad case, the bad flag is also set. I updated the API according to my
reading
> of the rte documentation.
[Sugesh]
the flag explanation in the DPDK code is shown as below.
/**
* Mask of bits used to determine the status of RX IP checksum.
* - PKT_RX_IP_CKSUM_UNKNOWN: no information about the RX IP checksum
* - PKT_RX_IP_CKSUM_BAD: the IP checksum in the packet is wrong
* - PKT_RX_IP_CKSUM_GOOD: the IP checksum in the packet is valid
* - PKT_RX_IP_CKSUM_NONE: the IP checksum is not correct in the packet
* data, but the integrity of the IP header is verified.
*/
/**
* Mask of bits used to determine the status of RX L4 checksum.
* - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
* - PKT_RX_L4_CKSUM_BAD: the L4 checksum in the packet is wrong
* - PKT_RX_L4_CKSUM_GOOD: the L4 checksum in the packet is valid
* - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
* data, but the integrity of the L4 data is verified.
*/
The BAD and GOOD is mutual exclusive. So will that be enough to validate
only one.??
Assuming we can determine the checksum is bad reliably from hwol, we could
check for bad only if not good. High probability of bad checksums might be an
exploit attempt.
> Please verify if the documentation is correct and my reading is correct.
> 4) If the checksum is not known to bad or good by hwol, then it is
> undetermined and must be checked by OVS.
>
> I have some code below, which is an incremental from yours, that describes
> ‘1-4’ above.
> Let me know if my read of the rte documentation is correct and the
> documentation itself is accurate. The diff includes a bit of coding
standard
> changes.
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c index 38084e9..6f40df2
100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1459,8 +1459,6 @@ conn_key_extract(struct conntrack *ct, struct
> dp_packet *pkt, ovs_be16 dl_type,
> const char *l4 = dp_packet_l4(pkt);
> const char *tail = dp_packet_tail(pkt);
> bool ok;
> - bool valid_l4_csum = 0;
> - bool valid_l3_csum = 0;
>
> memset(ctx, 0, sizeof *ctx);
>
> @@ -1504,23 +1502,32 @@ conn_key_extract(struct conntrack *ct, struct
> dp_packet *pkt, ovs_be16 dl_type,
> */
> ctx->key.dl_type = dl_type;
> if (ctx->key.dl_type == htons(ETH_TYPE_IP)) {
> - valid_l3_csum = dp_packet_ip_checksum_valid(pkt);
> - /* Validate the checksum only when the csum is invalid */
> - ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL,
> - !valid_l3_csum);
> + bool hwol_bad_l3_csum = dp_packet_ip_checksum_bad(pkt);
> + if (hwol_bad_l3_csum) {
> + ok = false;
> + } else {
> + bool hwol_good_l3_csum = dp_packet_ip_checksum_valid(pkt);
> + /* Validate the checksum only when hwol is not supported. */
> + ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL,
> + !hwol_good_l3_csum);
> + }
> } else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> ok = extract_l3_ipv6(&ctx->key, l3, tail - (char *) l3, NULL);
> } else {
> ok = false;
> }
>
> +
> if (ok) {
> - valid_l4_csum = dp_packet_l4_checksum_valid(pkt);
> - /* Validate the ckecksum only when the checksum is not
valid/unset */
> - if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3,
> - !valid_l4_csum)) {
> - ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
> - return true;
> + bool hwol_bad_l4_csum = dp_packet_l4_checksum_bad(pkt);
> + if (!hwol_bad_l4_csum) {
> + bool hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt);
> + /* Validate the checksum only when hwol is not supported. */
> + if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3,
> + !hwol_good_l4_csum)) {
> + ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
> + return true;
> + }
> }
> }
>
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index d2549b1..c5a7f1d
100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -612,7 +612,19 @@ static inline bool
> dp_packet_ip_checksum_valid(struct dp_packet *p) { #ifdef
> DPDK_NETDEV
> - return p->mbuf.ol_flags & PKT_RX_IP_CKSUM_GOOD;
> + return (p->mbuf.ol_flags & PKT_RX_IP_CKSUM_MASK) ==
> + PKT_RX_IP_CKSUM_GOOD;
> +#else
> + return 0 && p;
> +#endif
> +}
> +
> +static inline bool
> +dp_packet_ip_checksum_bad(struct dp_packet *p) { #ifdef DPDK_NETDEV
> + return (p->mbuf.ol_flags & PKT_RX_IP_CKSUM_MASK) ==
> + PKT_RX_IP_CKSUM_MASK;
> #else
> return 0 && p;
> #endif
> @@ -622,7 +634,19 @@ static inline bool
> dp_packet_l4_checksum_valid(struct dp_packet *p) { #ifdef
> DPDK_NETDEV
> - return p->mbuf.ol_flags & PKT_RX_L4_CKSUM_GOOD;
> + return (p->mbuf.ol_flags & PKT_RX_L4_CKSUM_MASK) ==
> + PKT_RX_L4_CKSUM_GOOD;
> +#else
> + return 0 && p;
> +#endif
> +}
> +
> +static inline bool
> +dp_packet_l4_checksum_bad(struct dp_packet *p) { #ifdef DPDK_NETDEV
> + return (p->mbuf.ol_flags & PKT_RX_L4_CKSUM_MASK) ==
> + PKT_RX_L4_CKSUM_MASK;
> #else
> return 0 && p;
> #endif
>
>
>
> ////////////////////////////////////////////////////////////////////
>
>
> On 6/16/17, 4:02 AM, "Sugesh Chandran" <[email protected]>
> wrote:
>
> Avoiding checksum validation in conntrack module if it is already
verified
> in DPDK physical NIC ports.
>
> Signed-off-by: Sugesh Chandran <[email protected]>
> Acked-by: Antonio Fischetti <[email protected]>
> Tested-by: Antonio Fischetti <[email protected]>
>
> ----
> v1->v2
> - Rebased on master
> - Added acked-by and tested-by tags in commit message
> ---
> lib/conntrack.c | 60 ++++++++++++++++++++++++++++++++++++---------
> ------------
> 1 file changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 90b154a..38084e9 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1119,10 +1119,13 @@ extract_l3_ipv6(struct conn_key *key, const
> void *data, size_t size,
>
> static inline bool
> checksum_valid(const struct conn_key *key, const void *data, size_t
size,
> - const void *l3)
> + const void *l3, bool validate_checksum)
> {
> uint32_t csum = 0;
>
> + if (!validate_checksum) {
> + return true;
> + }
> if (key->dl_type == htons(ETH_TYPE_IP)) {
> csum = packet_csum_pseudoheader(l3);
> } else if (key->dl_type == htons(ETH_TYPE_IPV6)) {
> @@ -1138,7 +1141,7 @@ checksum_valid(const struct conn_key *key,
> const void *data, size_t size,
>
> static inline bool
> check_l4_tcp(const struct conn_key *key, const void *data, size_t
size,
> - const void *l3)
> + const void *l3, bool validate_checksum)
> {
> const struct tcp_header *tcp = data;
> if (size < sizeof *tcp) {
> @@ -1150,12 +1153,12 @@ check_l4_tcp(const struct conn_key *key,
> const void *data, size_t size,
> return false;
> }
>
> - return checksum_valid(key, data, size, l3);
> + return checksum_valid(key, data, size, l3, validate_checksum);
> }
>
> static inline bool
> check_l4_udp(const struct conn_key *key, const void *data, size_t
size,
> - const void *l3)
> + const void *l3, bool validate_checksum)
> {
> const struct udp_header *udp = data;
> if (size < sizeof *udp) {
> @@ -1169,20 +1172,23 @@ check_l4_udp(const struct conn_key *key,
> const void *data, size_t size,
>
> /* Validation must be skipped if checksum is 0 on IPv4 packets */
> return (udp->udp_csum == 0 && key->dl_type == htons(ETH_TYPE_IP))
> - || checksum_valid(key, data, size, l3);
> + || checksum_valid(key, data, size, l3, validate_checksum);
> }
>
> static inline bool
> -check_l4_icmp(const void *data, size_t size)
> +check_l4_icmp(const void *data, size_t size, bool validate_checksum)
> {
> - return csum(data, size) == 0;
> + if (validate_checksum) {
> + return csum(data, size) == 0;
> + }
> + return true;
> }
>
> static inline bool
> check_l4_icmp6(const struct conn_key *key, const void *data, size_t
size,
> - const void *l3)
> + const void *l3, bool validate_checksum)
> {
> - return checksum_valid(key, data, size, l3);
> + return checksum_valid(key, data, size, l3, validate_checksum);
> }
>
> static inline bool
> @@ -1218,7 +1224,8 @@ extract_l4_udp(struct conn_key *key, const void
> *data, size_t size)
> }
>
> static inline bool extract_l4(struct conn_key *key, const void *data,
> - size_t size, bool *related, const void
*l3);
> + size_t size, bool *related, const void
*l3,
> + bool validate_checksum);
>
> static uint8_t
> reverse_icmp_type(uint8_t type)
> @@ -1306,7 +1313,7 @@ extract_l4_icmp(struct conn_key *key, const void
> *data, size_t size,
> key->dst = inner_key.dst;
> key->nw_proto = inner_key.nw_proto;
>
> - ok = extract_l4(key, l4, tail - l4, NULL, l3);
> + ok = extract_l4(key, l4, tail - l4, NULL, l3, false);
> if (ok) {
> conn_key_reverse(key);
> *related = true;
> @@ -1396,7 +1403,7 @@ extract_l4_icmp6(struct conn_key *key, const
> void *data, size_t size,
> key->dst = inner_key.dst;
> key->nw_proto = inner_key.nw_proto;
>
> - ok = extract_l4(key, l4, tail - l4, NULL, l3);
> + ok = extract_l4(key, l4, tail - l4, NULL, l3, false);
> if (ok) {
> conn_key_reverse(key);
> *related = true;
> @@ -1421,22 +1428,23 @@ extract_l4_icmp6(struct conn_key *key, const
> void *data, size_t size,
> * in an ICMP error. In this case, we skip checksum and length
validation.
> */
> static inline bool
> extract_l4(struct conn_key *key, const void *data, size_t size, bool
> *related,
> - const void *l3)
> + const void *l3, bool validate_checksum)
> {
> if (key->nw_proto == IPPROTO_TCP) {
> - return (!related || check_l4_tcp(key, data, size, l3))
> - && extract_l4_tcp(key, data, size);
> + return (!related || check_l4_tcp(key, data, size, l3,
> + validate_checksum)) && extract_l4_tcp(key, data,
size);
> } else if (key->nw_proto == IPPROTO_UDP) {
> - return (!related || check_l4_udp(key, data, size, l3))
> - && extract_l4_udp(key, data, size);
> + return (!related || check_l4_udp(key, data, size, l3,
> + validate_checksum)) && extract_l4_udp(key, data,
size);
> } else if (key->dl_type == htons(ETH_TYPE_IP)
> && key->nw_proto == IPPROTO_ICMP) {
> - return (!related || check_l4_icmp(data, size))
> + return (!related || check_l4_icmp(data, size,
validate_checksum))
> && extract_l4_icmp(key, data, size, related);
> } else if (key->dl_type == htons(ETH_TYPE_IPV6)
> && key->nw_proto == IPPROTO_ICMPV6) {
> - return (!related || check_l4_icmp6(key, data, size, l3))
> - && extract_l4_icmp6(key, data, size, related);
> + return (!related || check_l4_icmp6(key, data, size, l3,
> + validate_checksum)) && extract_l4_icmp6(key, data,
size,
> + related);
> } else {
> return false;
> }
> @@ -1451,6 +1459,8 @@ conn_key_extract(struct conntrack *ct, struct
> dp_packet *pkt, ovs_be16 dl_type,
> const char *l4 = dp_packet_l4(pkt);
> const char *tail = dp_packet_tail(pkt);
> bool ok;
> + bool valid_l4_csum = 0;
> + bool valid_l3_csum = 0;
>
> memset(ctx, 0, sizeof *ctx);
>
> @@ -1494,7 +1504,10 @@ conn_key_extract(struct conntrack *ct, struct
> dp_packet *pkt, ovs_be16 dl_type,
> */
> ctx->key.dl_type = dl_type;
> if (ctx->key.dl_type == htons(ETH_TYPE_IP)) {
> - ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3,
NULL, true);
> + valid_l3_csum = dp_packet_ip_checksum_valid(pkt);
> + /* Validate the checksum only when the csum is invalid */
> + ok = extract_l3_ipv4(&ctx->key, l3, tail - (char *) l3, NULL,
> + !valid_l3_csum);
> } else if (ctx->key.dl_type == htons(ETH_TYPE_IPV6)) {
> ok = extract_l3_ipv6(&ctx->key, l3, tail - (char *) l3,
NULL);
> } else {
> @@ -1502,7 +1515,10 @@ conn_key_extract(struct conntrack *ct, struct
> dp_packet *pkt, ovs_be16 dl_type,
> }
>
> if (ok) {
> - if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3))
{
> + valid_l4_csum = dp_packet_l4_checksum_valid(pkt);
> + /* Validate the ckecksum only when the checksum is not
valid/unset
> */
> + if (extract_l4(&ctx->key, l4, tail - l4, &ctx->related, l3,
> + !valid_l4_csum)) {
> ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
> return true;
> }
> --
> 2.7.4
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
