| Message ID | 20230315060725.61286-4-nmiki@yahoo-corp.jp |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | userspace: Add SRv6 tunnel support. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Wed, Mar 15, 2023 at 03:07:23PM +0900, Nobuhiro MIKI wrote: > Checks whether IPPROTO_ROUTING exists in the IPv6 extension headers. > If it exists, the first address is retrieved. > > Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> Hi Miki-san, this also looks good to me. I have made a few suggestions for improvements inline. > --- > lib/conntrack.c | 4 +++- > lib/flow.c | 26 ++++++++++++++++++++------ > lib/flow.h | 3 ++- > lib/ipf.c | 12 ++++++++---- > lib/packets.h | 13 +++++++++++++ > 5 files changed, 46 insertions(+), 12 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 8cf7779c6703..8dca813e2c06 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -1618,7 +1618,9 @@ extract_l3_ipv6(struct conn_key *key, const void *data, size_t size, > uint8_t nw_frag = 0; > > const struct ovs_16aligned_ip6_frag *frag_hdr; > - if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, &frag_hdr)) { > + const struct ip6_rt_hdr *rt_hdr; > + if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, &frag_hdr, > + &rt_hdr)) { Neither frag_hdr nor rt_hdr are used by this function, other than to fill in arguments for parse_ipv6_ext_hdrs(). Perhaps an alternative would be to teach parse_ipv6_ext_hdrs() (probably parse_ipv6_ext_hdrs__()) how to handle NULL values for these arguments. > return false; > } > > diff --git a/lib/flow.c b/lib/flow.c > index c3a3aa3ce45d..b263343855a8 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -479,9 +479,11 @@ invalid: > static inline bool > parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, > uint8_t *nw_frag, > - const struct ovs_16aligned_ip6_frag **frag_hdr) > + const struct ovs_16aligned_ip6_frag **frag_hdr, > + const struct ip6_rt_hdr **rt_hdr) > { > *frag_hdr = NULL; > + *rt_hdr = NULL; > while (1) { > if (OVS_LIKELY((*nw_proto != IPPROTO_HOPOPTS) > && (*nw_proto != IPPROTO_ROUTING) > @@ -504,7 +506,6 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, > } > > if ((*nw_proto == IPPROTO_HOPOPTS) > - || (*nw_proto == IPPROTO_ROUTING) > || (*nw_proto == IPPROTO_DSTOPTS)) { > /* These headers, while different, have the fields we care > * about in the same location and with the same > @@ -515,6 +516,13 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, > (ext_hdr->ip6e_len + 1) * 8))) { > return false; > } > + } else if (*nw_proto == IPPROTO_ROUTING) { > + *rt_hdr = *datap; > + *nw_proto = (*rt_hdr)->nexthdr; > + if (OVS_UNLIKELY(!data_try_pull(datap, sizep, > + ((*rt_hdr)->hdrlen + 1) * 8))) { > + return false; > + } > } else if (*nw_proto == IPPROTO_AH) { > /* A standard AH definition isn't available, but the fields > * we care about are in the same location as the generic > @@ -561,15 +569,19 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, > * has FLOW_NW_FRAG_LATER set. Both first and later fragments have > * FLOW_NW_FRAG_ANY set in 'nw_frag'. > * > + * If a routing header is found, '*rt_hdr' is set to the routing > + * header and otherwise set to NULL. > + * > * A return value of false indicates that there was a problem parsing > * the extension headers.*/ > bool > parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, > uint8_t *nw_frag, > - const struct ovs_16aligned_ip6_frag **frag_hdr) > + const struct ovs_16aligned_ip6_frag **frag_hdr, > + const struct ip6_rt_hdr **rt_hdr) > { > return parse_ipv6_ext_hdrs__(datap, sizep, nw_proto, nw_frag, > - frag_hdr); > + frag_hdr, rt_hdr); > } > > bool > @@ -946,8 +958,9 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) > nw_proto = nh->ip6_nxt; > > const struct ovs_16aligned_ip6_frag *frag_hdr; > + const struct ip6_rt_hdr *rt_hdr; > if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag, > - &frag_hdr)) { > + &frag_hdr, &rt_hdr)) { > goto out; > } > > @@ -1201,9 +1214,10 @@ parse_tcp_flags(struct dp_packet *packet, > dp_packet_set_l2_pad_size(packet, size - plen); > size = plen; > const struct ovs_16aligned_ip6_frag *frag_hdr; > + const struct ip6_rt_hdr *rt_hdr; > nw_proto = nh->ip6_nxt; > if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag, > - &frag_hdr)) { > + &frag_hdr, &rt_hdr)) { > return 0; > } > } else { > diff --git a/lib/flow.h b/lib/flow.h > index c647ad83c256..a9d026e1ce3b 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -132,7 +132,8 @@ void packet_expand(struct dp_packet *, const struct flow *, size_t size); > > bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, > uint8_t *nw_frag, > - const struct ovs_16aligned_ip6_frag **frag_hdr); > + const struct ovs_16aligned_ip6_frag **frag_hdr, > + const struct ip6_rt_hdr **rt_hdr); > bool parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh *key); > uint16_t parse_tcp_flags(struct dp_packet *packet, ovs_be16 *dl_type_p, > uint8_t *nw_frag_p, ovs_be16 *first_vlan_tci_p); > diff --git a/lib/ipf.c b/lib/ipf.c > index d452663743c5..fae2b2d72ab0 100644 > --- a/lib/ipf.c > +++ b/lib/ipf.c > @@ -486,8 +486,9 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list) > size_t datasize = pl; > > const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; > - if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr) > - || !nw_frag || !frag_hdr) { > + const struct ip6_rt_hdr *rt_hdr = NULL; The first thing that parse_ipv6_ext_hdrs() does, via parse_ipv6_ext_hdrs__(), is to set frag_hdr and rt_hrd to NULL. Perhaps it's a style issue, but IMHO there is no need to do it here too. > + if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr, > + &rt_hdr) || !nw_frag || !frag_hdr) { > > ipf_print_reass_packet("Unparsed reassembled v6 packet; v6 hdr:", l3); > dp_packet_delete(pkt); > @@ -679,8 +680,9 @@ ipf_is_valid_v6_frag(struct ipf *ipf, struct dp_packet *pkt) > const void *data = l3 + 1; > size_t datasize = l3_size - l3_hdr_size; > const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; > + const struct ip6_rt_hdr *rt_hdr = NULL; > if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, > - &frag_hdr) || !nw_frag || !frag_hdr) { > + &frag_hdr, &rt_hdr) || !nw_frag || !frag_hdr) { > return false; > } > > @@ -722,8 +724,10 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16 dl_type, uint16_t zone, > const void *data = l3 + 1; > size_t datasize = dp_packet_l3_size(pkt) - sizeof *l3; > const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; > + const struct ip6_rt_hdr *rt_hdr = NULL; > > - parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr); > + parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr, > + &rt_hdr); > ovs_assert(nw_frag && frag_hdr); > ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg; > *start_data_byte = ntohs(ip6f_offlg & IP6F_OFF_MASK) + > diff --git a/lib/packets.h b/lib/packets.h > index 8626aac8d53f..abff24db016e 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -710,6 +710,10 @@ char *ip_parse_cidr_len(const char *s, int *n, ovs_be32 *ip, > #define IPPROTO_UDPLITE 136 > #endif > > +#ifndef IPPROTO_IPIP > +#define IPPROTO_IPIP 4 > +#endif > + IPPROTO_IPIP has been around for a long time. And I don't think other very old values, like IPPROTO_UDP, are defined in this file So, perhaps this isn't needed here? > /* TOS fields. */ > #define IP_ECN_NOT_ECT 0x0 > #define IP_ECN_ECT_1 0x01 > @@ -988,6 +992,15 @@ struct ovs_16aligned_ip6_frag { > ovs_16aligned_be32 ip6f_ident; > }; > > +#define IP6_RT_HDR_LEN 4 > +struct ip6_rt_hdr { > + uint8_t nexthdr; > + uint8_t hdrlen; > + uint8_t type; > + uint8_t segments_left; > +}; > +BUILD_ASSERT_DECL(IP6_RT_HDR_LEN == sizeof(struct ip6_rt_hdr)); > + > #define ICMP6_HEADER_LEN 4 > struct icmp6_header { > uint8_t icmp6_type;
On 2023/03/22 21:21, Simon Horman wrote: > On Wed, Mar 15, 2023 at 03:07:23PM +0900, Nobuhiro MIKI wrote: >> Checks whether IPPROTO_ROUTING exists in the IPv6 extension headers. >> If it exists, the first address is retrieved. >> >> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> > > Hi Miki-san, > > this also looks good to me. > I have made a few suggestions for improvements inline. > Hi Simon-san, Thanks for your review. Best Regards, Nobuhiro MIKI >> --- >> lib/conntrack.c | 4 +++- >> lib/flow.c | 26 ++++++++++++++++++++------ >> lib/flow.h | 3 ++- >> lib/ipf.c | 12 ++++++++---- >> lib/packets.h | 13 +++++++++++++ >> 5 files changed, 46 insertions(+), 12 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 8cf7779c6703..8dca813e2c06 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -1618,7 +1618,9 @@ extract_l3_ipv6(struct conn_key *key, const void *data, size_t size, >> uint8_t nw_frag = 0; >> >> const struct ovs_16aligned_ip6_frag *frag_hdr; >> - if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, &frag_hdr)) { >> + const struct ip6_rt_hdr *rt_hdr; >> + if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, &frag_hdr, >> + &rt_hdr)) { > > Neither frag_hdr nor rt_hdr are used by this function, other than > to fill in arguments for parse_ipv6_ext_hdrs(). > > Perhaps an alternative would be to teach parse_ipv6_ext_hdrs() > (probably parse_ipv6_ext_hdrs__()) how to handle NULL values > for these arguments. > Yes, since frag_hdr and rt_hdr are optional, it is more convenient to simply use NULL values on the caller side if they are not needed. As seen from the caller, it should look like the followings: parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, NULL, NULL); parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, NULL, &rt_hdr); parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, &frag_hdr, &rt_hdr); I'll refactor. >> return false; >> } >> >> diff --git a/lib/flow.c b/lib/flow.c >> index c3a3aa3ce45d..b263343855a8 100644 >> --- a/lib/flow.c >> +++ b/lib/flow.c >> @@ -479,9 +479,11 @@ invalid: >> static inline bool >> parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, >> uint8_t *nw_frag, >> - const struct ovs_16aligned_ip6_frag **frag_hdr) >> + const struct ovs_16aligned_ip6_frag **frag_hdr, >> + const struct ip6_rt_hdr **rt_hdr) >> { >> *frag_hdr = NULL; >> + *rt_hdr = NULL; >> while (1) { >> if (OVS_LIKELY((*nw_proto != IPPROTO_HOPOPTS) >> && (*nw_proto != IPPROTO_ROUTING) >> @@ -504,7 +506,6 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, >> } >> >> if ((*nw_proto == IPPROTO_HOPOPTS) >> - || (*nw_proto == IPPROTO_ROUTING) >> || (*nw_proto == IPPROTO_DSTOPTS)) { >> /* These headers, while different, have the fields we care >> * about in the same location and with the same >> @@ -515,6 +516,13 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, >> (ext_hdr->ip6e_len + 1) * 8))) { >> return false; >> } >> + } else if (*nw_proto == IPPROTO_ROUTING) { >> + *rt_hdr = *datap; >> + *nw_proto = (*rt_hdr)->nexthdr; >> + if (OVS_UNLIKELY(!data_try_pull(datap, sizep, >> + ((*rt_hdr)->hdrlen + 1) * 8))) { >> + return false; >> + } >> } else if (*nw_proto == IPPROTO_AH) { >> /* A standard AH definition isn't available, but the fields >> * we care about are in the same location as the generic >> @@ -561,15 +569,19 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, >> * has FLOW_NW_FRAG_LATER set. Both first and later fragments have >> * FLOW_NW_FRAG_ANY set in 'nw_frag'. >> * >> + * If a routing header is found, '*rt_hdr' is set to the routing >> + * header and otherwise set to NULL. >> + * >> * A return value of false indicates that there was a problem parsing >> * the extension headers.*/ >> bool >> parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, >> uint8_t *nw_frag, >> - const struct ovs_16aligned_ip6_frag **frag_hdr) >> + const struct ovs_16aligned_ip6_frag **frag_hdr, >> + const struct ip6_rt_hdr **rt_hdr) >> { >> return parse_ipv6_ext_hdrs__(datap, sizep, nw_proto, nw_frag, >> - frag_hdr); >> + frag_hdr, rt_hdr); >> } >> >> bool >> @@ -946,8 +958,9 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) >> nw_proto = nh->ip6_nxt; >> >> const struct ovs_16aligned_ip6_frag *frag_hdr; >> + const struct ip6_rt_hdr *rt_hdr; >> if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag, >> - &frag_hdr)) { >> + &frag_hdr, &rt_hdr)) { >> goto out; >> } >> >> @@ -1201,9 +1214,10 @@ parse_tcp_flags(struct dp_packet *packet, >> dp_packet_set_l2_pad_size(packet, size - plen); >> size = plen; >> const struct ovs_16aligned_ip6_frag *frag_hdr; >> + const struct ip6_rt_hdr *rt_hdr; >> nw_proto = nh->ip6_nxt; >> if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag, >> - &frag_hdr)) { >> + &frag_hdr, &rt_hdr)) { >> return 0; >> } >> } else { >> diff --git a/lib/flow.h b/lib/flow.h >> index c647ad83c256..a9d026e1ce3b 100644 >> --- a/lib/flow.h >> +++ b/lib/flow.h >> @@ -132,7 +132,8 @@ void packet_expand(struct dp_packet *, const struct flow *, size_t size); >> >> bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, >> uint8_t *nw_frag, >> - const struct ovs_16aligned_ip6_frag **frag_hdr); >> + const struct ovs_16aligned_ip6_frag **frag_hdr, >> + const struct ip6_rt_hdr **rt_hdr); >> bool parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh *key); >> uint16_t parse_tcp_flags(struct dp_packet *packet, ovs_be16 *dl_type_p, >> uint8_t *nw_frag_p, ovs_be16 *first_vlan_tci_p); >> diff --git a/lib/ipf.c b/lib/ipf.c >> index d452663743c5..fae2b2d72ab0 100644 >> --- a/lib/ipf.c >> +++ b/lib/ipf.c >> @@ -486,8 +486,9 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list) >> size_t datasize = pl; >> >> const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; >> - if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr) >> - || !nw_frag || !frag_hdr) { >> + const struct ip6_rt_hdr *rt_hdr = NULL; > > The first thing that parse_ipv6_ext_hdrs() does, > via parse_ipv6_ext_hdrs__(), is to set frag_hdr and rt_hrd to NULL. > Perhaps it's a style issue, but IMHO there is no need to do it here too. > You are right, the caller does not need to initialize frag_hdr and rt_hdr with NULL. I'll remove. >> + if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr, >> + &rt_hdr) || !nw_frag || !frag_hdr) { >> >> ipf_print_reass_packet("Unparsed reassembled v6 packet; v6 hdr:", l3); >> dp_packet_delete(pkt); >> @@ -679,8 +680,9 @@ ipf_is_valid_v6_frag(struct ipf *ipf, struct dp_packet *pkt) >> const void *data = l3 + 1; >> size_t datasize = l3_size - l3_hdr_size; >> const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; >> + const struct ip6_rt_hdr *rt_hdr = NULL; >> if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, >> - &frag_hdr) || !nw_frag || !frag_hdr) { >> + &frag_hdr, &rt_hdr) || !nw_frag || !frag_hdr) { >> return false; >> } >> >> @@ -722,8 +724,10 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16 dl_type, uint16_t zone, >> const void *data = l3 + 1; >> size_t datasize = dp_packet_l3_size(pkt) - sizeof *l3; >> const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; >> + const struct ip6_rt_hdr *rt_hdr = NULL; >> >> - parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr); >> + parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr, >> + &rt_hdr); >> ovs_assert(nw_frag && frag_hdr); >> ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg; >> *start_data_byte = ntohs(ip6f_offlg & IP6F_OFF_MASK) + >> diff --git a/lib/packets.h b/lib/packets.h >> index 8626aac8d53f..abff24db016e 100644 >> --- a/lib/packets.h >> +++ b/lib/packets.h >> @@ -710,6 +710,10 @@ char *ip_parse_cidr_len(const char *s, int *n, ovs_be32 *ip, >> #define IPPROTO_UDPLITE 136 >> #endif >> >> +#ifndef IPPROTO_IPIP >> +#define IPPROTO_IPIP 4 >> +#endif >> + > > IPPROTO_IPIP has been around for a long time. > And I don't think other very old values, > like IPPROTO_UDP, are defined in this file > So, perhaps this isn't needed here? > Perhaps it is not necessary. I'll remove it for now. >> /* TOS fields. */ >> #define IP_ECN_NOT_ECT 0x0 >> #define IP_ECN_ECT_1 0x01 >> @@ -988,6 +992,15 @@ struct ovs_16aligned_ip6_frag { >> ovs_16aligned_be32 ip6f_ident; >> }; >> >> +#define IP6_RT_HDR_LEN 4 >> +struct ip6_rt_hdr { >> + uint8_t nexthdr; >> + uint8_t hdrlen; >> + uint8_t type; >> + uint8_t segments_left; >> +}; >> +BUILD_ASSERT_DECL(IP6_RT_HDR_LEN == sizeof(struct ip6_rt_hdr)); >> + >> #define ICMP6_HEADER_LEN 4 >> struct icmp6_header { >> uint8_t icmp6_type;
On 3/23/23 03:57, Nobuhiro MIKI wrote: > On 2023/03/22 21:21, Simon Horman wrote: >> On Wed, Mar 15, 2023 at 03:07:23PM +0900, Nobuhiro MIKI wrote: >>> Checks whether IPPROTO_ROUTING exists in the IPv6 extension headers. >>> If it exists, the first address is retrieved. >>> >>> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> >> >> Hi Miki-san, >> >> this also looks good to me. >> I have made a few suggestions for improvements inline. >> > > Hi Simon-san, > > Thanks for your review. > > Best Regards, > Nobuhiro MIKI > >>> --- >>> lib/conntrack.c | 4 +++- >>> lib/flow.c | 26 ++++++++++++++++++++------ >>> lib/flow.h | 3 ++- >>> lib/ipf.c | 12 ++++++++---- >>> lib/packets.h | 13 +++++++++++++ >>> 5 files changed, 46 insertions(+), 12 deletions(-) <snip> >>> diff --git a/lib/packets.h b/lib/packets.h >>> index 8626aac8d53f..abff24db016e 100644 >>> --- a/lib/packets.h >>> +++ b/lib/packets.h >>> @@ -710,6 +710,10 @@ char *ip_parse_cidr_len(const char *s, int *n, ovs_be32 *ip, >>> #define IPPROTO_UDPLITE 136 >>> #endif >>> >>> +#ifndef IPPROTO_IPIP >>> +#define IPPROTO_IPIP 4 >>> +#endif >>> + >> >> IPPROTO_IPIP has been around for a long time. >> And I don't think other very old values, >> like IPPROTO_UDP, are defined in this file >> So, perhaps this isn't needed here? >> > > Perhaps it is not necessary. > I'll remove it for now. It seems still necessary to define it for sparse that doesn't use system in.h. I.e. we need to define it in include/sparse/netinet/in.h, otherwise builds with --enable-sparse are failing. Best regards, Ilya Maximets.
On Fri, Mar 24, 2023 at 01:03:00PM +0100, Ilya Maximets wrote: > On 3/23/23 03:57, Nobuhiro MIKI wrote: > > On 2023/03/22 21:21, Simon Horman wrote: > >> On Wed, Mar 15, 2023 at 03:07:23PM +0900, Nobuhiro MIKI wrote: > >>> Checks whether IPPROTO_ROUTING exists in the IPv6 extension headers. > >>> If it exists, the first address is retrieved. > >>> > >>> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> > >> > >> Hi Miki-san, > >> > >> this also looks good to me. > >> I have made a few suggestions for improvements inline. > >> > > > > Hi Simon-san, > > > > Thanks for your review. > > > > Best Regards, > > Nobuhiro MIKI > > > >>> --- > >>> lib/conntrack.c | 4 +++- > >>> lib/flow.c | 26 ++++++++++++++++++++------ > >>> lib/flow.h | 3 ++- > >>> lib/ipf.c | 12 ++++++++---- > >>> lib/packets.h | 13 +++++++++++++ > >>> 5 files changed, 46 insertions(+), 12 deletions(-) > > <snip> > > >>> diff --git a/lib/packets.h b/lib/packets.h > >>> index 8626aac8d53f..abff24db016e 100644 > >>> --- a/lib/packets.h > >>> +++ b/lib/packets.h > >>> @@ -710,6 +710,10 @@ char *ip_parse_cidr_len(const char *s, int *n, ovs_be32 *ip, > >>> #define IPPROTO_UDPLITE 136 > >>> #endif > >>> > >>> +#ifndef IPPROTO_IPIP > >>> +#define IPPROTO_IPIP 4 > >>> +#endif > >>> + > >> > >> IPPROTO_IPIP has been around for a long time. > >> And I don't think other very old values, > >> like IPPROTO_UDP, are defined in this file > >> So, perhaps this isn't needed here? > >> > > > > Perhaps it is not necessary. > > I'll remove it for now. > > It seems still necessary to define it for sparse that doesn't use system in.h. > I.e. we need to define it in include/sparse/netinet/in.h, otherwise builds > with --enable-sparse are failing. Ack, sorry for the noise.
On 2023/03/24 21:06, Simon Horman wrote: > On Fri, Mar 24, 2023 at 01:03:00PM +0100, Ilya Maximets wrote: >> On 3/23/23 03:57, Nobuhiro MIKI wrote: >>> On 2023/03/22 21:21, Simon Horman wrote: >>>> On Wed, Mar 15, 2023 at 03:07:23PM +0900, Nobuhiro MIKI wrote: >>>>> Checks whether IPPROTO_ROUTING exists in the IPv6 extension headers. >>>>> If it exists, the first address is retrieved. >>>>> >>>>> Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> >>>> >>>> Hi Miki-san, >>>> >>>> this also looks good to me. >>>> I have made a few suggestions for improvements inline. >>>> >>> >>> Hi Simon-san, >>> >>> Thanks for your review. >>> >>> Best Regards, >>> Nobuhiro MIKI >>> >>>>> --- >>>>> lib/conntrack.c | 4 +++- >>>>> lib/flow.c | 26 ++++++++++++++++++++------ >>>>> lib/flow.h | 3 ++- >>>>> lib/ipf.c | 12 ++++++++---- >>>>> lib/packets.h | 13 +++++++++++++ >>>>> 5 files changed, 46 insertions(+), 12 deletions(-) >> >> <snip> >> >>>>> diff --git a/lib/packets.h b/lib/packets.h >>>>> index 8626aac8d53f..abff24db016e 100644 >>>>> --- a/lib/packets.h >>>>> +++ b/lib/packets.h >>>>> @@ -710,6 +710,10 @@ char *ip_parse_cidr_len(const char *s, int *n, ovs_be32 *ip, >>>>> #define IPPROTO_UDPLITE 136 >>>>> #endif >>>>> >>>>> +#ifndef IPPROTO_IPIP >>>>> +#define IPPROTO_IPIP 4 >>>>> +#endif >>>>> + >>>> >>>> IPPROTO_IPIP has been around for a long time. >>>> And I don't think other very old values, >>>> like IPPROTO_UDP, are defined in this file >>>> So, perhaps this isn't needed here? >>>> >>> >>> Perhaps it is not necessary. >>> I'll remove it for now. >> >> It seems still necessary to define it for sparse that doesn't use system in.h. >> I.e. we need to define it in include/sparse/netinet/in.h, otherwise builds >> with --enable-sparse are failing. > > Ack, sorry for the noise. Thanks for confirming the CI. I'll define IPPROTO_IPIP in include/sparse/netinet/in.h. Best Regards, Nobuhiro MIKI
diff --git a/lib/conntrack.c b/lib/conntrack.c index 8cf7779c6703..8dca813e2c06 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1618,7 +1618,9 @@ extract_l3_ipv6(struct conn_key *key, const void *data, size_t size, uint8_t nw_frag = 0; const struct ovs_16aligned_ip6_frag *frag_hdr; - if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, &frag_hdr)) { + const struct ip6_rt_hdr *rt_hdr; + if (!parse_ipv6_ext_hdrs(&data, &size, &nw_proto, &nw_frag, &frag_hdr, + &rt_hdr)) { return false; } diff --git a/lib/flow.c b/lib/flow.c index c3a3aa3ce45d..b263343855a8 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -479,9 +479,11 @@ invalid: static inline bool parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, uint8_t *nw_frag, - const struct ovs_16aligned_ip6_frag **frag_hdr) + const struct ovs_16aligned_ip6_frag **frag_hdr, + const struct ip6_rt_hdr **rt_hdr) { *frag_hdr = NULL; + *rt_hdr = NULL; while (1) { if (OVS_LIKELY((*nw_proto != IPPROTO_HOPOPTS) && (*nw_proto != IPPROTO_ROUTING) @@ -504,7 +506,6 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, } if ((*nw_proto == IPPROTO_HOPOPTS) - || (*nw_proto == IPPROTO_ROUTING) || (*nw_proto == IPPROTO_DSTOPTS)) { /* These headers, while different, have the fields we care * about in the same location and with the same @@ -515,6 +516,13 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, (ext_hdr->ip6e_len + 1) * 8))) { return false; } + } else if (*nw_proto == IPPROTO_ROUTING) { + *rt_hdr = *datap; + *nw_proto = (*rt_hdr)->nexthdr; + if (OVS_UNLIKELY(!data_try_pull(datap, sizep, + ((*rt_hdr)->hdrlen + 1) * 8))) { + return false; + } } else if (*nw_proto == IPPROTO_AH) { /* A standard AH definition isn't available, but the fields * we care about are in the same location as the generic @@ -561,15 +569,19 @@ parse_ipv6_ext_hdrs__(const void **datap, size_t *sizep, uint8_t *nw_proto, * has FLOW_NW_FRAG_LATER set. Both first and later fragments have * FLOW_NW_FRAG_ANY set in 'nw_frag'. * + * If a routing header is found, '*rt_hdr' is set to the routing + * header and otherwise set to NULL. + * * A return value of false indicates that there was a problem parsing * the extension headers.*/ bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, uint8_t *nw_frag, - const struct ovs_16aligned_ip6_frag **frag_hdr) + const struct ovs_16aligned_ip6_frag **frag_hdr, + const struct ip6_rt_hdr **rt_hdr) { return parse_ipv6_ext_hdrs__(datap, sizep, nw_proto, nw_frag, - frag_hdr); + frag_hdr, rt_hdr); } bool @@ -946,8 +958,9 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) nw_proto = nh->ip6_nxt; const struct ovs_16aligned_ip6_frag *frag_hdr; + const struct ip6_rt_hdr *rt_hdr; if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag, - &frag_hdr)) { + &frag_hdr, &rt_hdr)) { goto out; } @@ -1201,9 +1214,10 @@ parse_tcp_flags(struct dp_packet *packet, dp_packet_set_l2_pad_size(packet, size - plen); size = plen; const struct ovs_16aligned_ip6_frag *frag_hdr; + const struct ip6_rt_hdr *rt_hdr; nw_proto = nh->ip6_nxt; if (!parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag, - &frag_hdr)) { + &frag_hdr, &rt_hdr)) { return 0; } } else { diff --git a/lib/flow.h b/lib/flow.h index c647ad83c256..a9d026e1ce3b 100644 --- a/lib/flow.h +++ b/lib/flow.h @@ -132,7 +132,8 @@ void packet_expand(struct dp_packet *, const struct flow *, size_t size); bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto, uint8_t *nw_frag, - const struct ovs_16aligned_ip6_frag **frag_hdr); + const struct ovs_16aligned_ip6_frag **frag_hdr, + const struct ip6_rt_hdr **rt_hdr); bool parse_nsh(const void **datap, size_t *sizep, struct ovs_key_nsh *key); uint16_t parse_tcp_flags(struct dp_packet *packet, ovs_be16 *dl_type_p, uint8_t *nw_frag_p, ovs_be16 *first_vlan_tci_p); diff --git a/lib/ipf.c b/lib/ipf.c index d452663743c5..fae2b2d72ab0 100644 --- a/lib/ipf.c +++ b/lib/ipf.c @@ -486,8 +486,9 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list) size_t datasize = pl; const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; - if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr) - || !nw_frag || !frag_hdr) { + const struct ip6_rt_hdr *rt_hdr = NULL; + if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr, + &rt_hdr) || !nw_frag || !frag_hdr) { ipf_print_reass_packet("Unparsed reassembled v6 packet; v6 hdr:", l3); dp_packet_delete(pkt); @@ -679,8 +680,9 @@ ipf_is_valid_v6_frag(struct ipf *ipf, struct dp_packet *pkt) const void *data = l3 + 1; size_t datasize = l3_size - l3_hdr_size; const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; + const struct ip6_rt_hdr *rt_hdr = NULL; if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, - &frag_hdr) || !nw_frag || !frag_hdr) { + &frag_hdr, &rt_hdr) || !nw_frag || !frag_hdr) { return false; } @@ -722,8 +724,10 @@ ipf_v6_key_extract(struct dp_packet *pkt, ovs_be16 dl_type, uint16_t zone, const void *data = l3 + 1; size_t datasize = dp_packet_l3_size(pkt) - sizeof *l3; const struct ovs_16aligned_ip6_frag *frag_hdr = NULL; + const struct ip6_rt_hdr *rt_hdr = NULL; - parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr); + parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr, + &rt_hdr); ovs_assert(nw_frag && frag_hdr); ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg; *start_data_byte = ntohs(ip6f_offlg & IP6F_OFF_MASK) + diff --git a/lib/packets.h b/lib/packets.h index 8626aac8d53f..abff24db016e 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -710,6 +710,10 @@ char *ip_parse_cidr_len(const char *s, int *n, ovs_be32 *ip, #define IPPROTO_UDPLITE 136 #endif +#ifndef IPPROTO_IPIP +#define IPPROTO_IPIP 4 +#endif + /* TOS fields. */ #define IP_ECN_NOT_ECT 0x0 #define IP_ECN_ECT_1 0x01 @@ -988,6 +992,15 @@ struct ovs_16aligned_ip6_frag { ovs_16aligned_be32 ip6f_ident; }; +#define IP6_RT_HDR_LEN 4 +struct ip6_rt_hdr { + uint8_t nexthdr; + uint8_t hdrlen; + uint8_t type; + uint8_t segments_left; +}; +BUILD_ASSERT_DECL(IP6_RT_HDR_LEN == sizeof(struct ip6_rt_hdr)); + #define ICMP6_HEADER_LEN 4 struct icmp6_header { uint8_t icmp6_type;
Checks whether IPPROTO_ROUTING exists in the IPv6 extension headers. If it exists, the first address is retrieved. Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp> --- lib/conntrack.c | 4 +++- lib/flow.c | 26 ++++++++++++++++++++------ lib/flow.h | 3 ++- lib/ipf.c | 12 ++++++++---- lib/packets.h | 13 +++++++++++++ 5 files changed, 46 insertions(+), 12 deletions(-)