Hi Manu, On Mon, Feb 05, 2018 at 05:10:05PM +0100, Emmanuel Hocdet wrote: > Hi, > > Series of patches to support CRC32c checksum to proxy protocol v2 header > (as describe in "doc/proxy-protocol.txt ») > . add hash_crc32c function > . add « crc32c » option to proxy-v2-options > . check crc32c checksum when CRC32C tlv is received.
Thanks. I'm having a few comments here : > From a62ca59e175631a0adbb9b8adf1d58492f6bce5a Mon Sep 17 00:00:00 2001 > From: Emmanuel Hocdet <[email protected]> > Date: Mon, 5 Feb 2018 15:26:43 +0100 > Subject: [PATCH 2/3] MINOR: proxy-v2-options: add crc32c > > This patch add option crc32c (PP2_TYPE_CRC32C) to proxy protocol v2. > It compute the checksum of proxy protocol v2 header as describe in > "doc/proxy-protocol.txt". > (...) > diff --git a/include/types/server.h b/include/types/server.h > index 6d0566be2..e68b2036b 100644 > --- a/include/types/server.h > +++ b/include/types/server.h > @@ -195,7 +196,7 @@ struct server { > enum obj_type obj_type; /* object type == > OBJ_TYPE_SERVER */ > enum srv_state next_state, cur_state; /* server state among SRV_ST_* > */ > enum srv_admin next_admin, cur_admin; /* server maintenance status : > SRV_ADMF_* */ > - unsigned char pp_opts; /* proxy protocol options > (SRV_PP_*) */ > + unsigned int pp_opts; /* proxy protocol options > (SRV_PP_*) */ > struct server *next; This is going to punch a 8-bit hole before the struct member. While not dramatic for such a struct, it's a good practice to place a comment before indicating that 8 bits are available there. > diff --git a/src/connection.c b/src/connection.c > index 412fe3f41..833763e4c 100644 > --- a/src/connection.c > +++ b/src/connection.c > @@ -990,6 +991,7 @@ static int make_tlv(char *dest, int dest_len, char type, > uint16_t length, const > int make_proxy_line_v2(char *buf, int buf_len, struct server *srv, struct > connection *remote) > { > const char pp2_signature[] = PP2_SIGNATURE; > + uint32_t *tlv_crc32c_p = NULL; > int ret = 0; Spaces instead of tab above, lazy copy-paste ? :-) > @@ -1037,6 +1039,14 @@ int make_proxy_line_v2(char *buf, int buf_len, struct > server *srv, struct connec > ret = PP2_HDR_LEN_UNSPEC; > } > > + if (srv->pp_opts & SRV_PP_V2_CRC32C) { > + uint32_t zero_crc32c = 0; > + if ((buf_len - ret) < sizeof(struct tlv)) > + return 0; > + tlv_crc32c_p = (uint32_t *)((struct tlv *)&buf[ret])->value; > > + ret += make_tlv(&buf[ret], (buf_len - ret), PP2_TYPE_CRC32C, > sizeof(zero_crc32c), (const char *)&zero_crc32c); > + } > + > if (conn_get_alpn(remote, &value, &value_len)) { > if ((buf_len - ret) < sizeof(struct tlv)) > return 0; > @@ -1115,6 +1125,10 @@ int make_proxy_line_v2(char *buf, int buf_len, struct > server *srv, struct connec > > hdr->len = htons((uint16_t)(ret - PP2_HEADER_LEN)); > > + if (tlv_crc32c_p) { > + *tlv_crc32c_p = htonl(hash_crc32c(buf, ret)); How can you be sure the pointer is 32-bit aligned there ? I don't think we have anything guaranteeing it. Please take a look at write_u32() in net_helper.h, it's provided exactly for this, and does exactly the same thing on architectures supporting unaligned accesses (x86, armv7/8). And in order to avoid any accidental misuse you can then declare tlv_crc32c_p as a void*. > From 4a22d7ca1d4f711b0d777095e5ce793e04a93c78 Mon Sep 17 00:00:00 2001 > From: Emmanuel Hocdet <[email protected]> > Date: Mon, 5 Feb 2018 16:23:23 +0100 > Subject: [PATCH 3/3] MINOR: accept-proxy: support proxy protocol v2 CRC32c > checksum > > When proxy protocol v2 CRC32c tlv is received, check it before accept > connection (as describe in "doc/proxy-protocol.txt"). > --- > src/connection.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/connection.c b/src/connection.c > index 833763e4c..8fd3209d5 100644 > --- a/src/connection.c > +++ b/src/connection.c > @@ -612,6 +612,13 @@ int conn_recv_proxy(struct connection *conn, int flag) > tlv_offset += tlv_len + TLV_HEADER_SIZE; > > switch (tlv_packet->type) { > + case PP2_TYPE_CRC32C: { > + uint32_t n_crc32c = ntohl(*(uint32_t > *)tlv_packet->value); > + *(uint32_t *)tlv_packet->value = 0; Same here, please see read_u32() and write_u32(). Otherwise looks good. Thanks! Willy
