Skip to content

Commit

Permalink
pinctrl: Handle arp/nd for other address families.
Browse files Browse the repository at this point in the history 
Previously we could only generate ARP requests from IPv4 packets
and NS requests from IPv6 packets. This was the case because we rely on
information in the packet to generate the ARP/NS requests.

However in case of ARP/NS requests originating from the Logical_Router
pipeline for nexthop lookups we overwrite the affected fields
afterwards. This overwrite is done by the userdata openflow actions.
Because of this we actually do not rely on any information of the IPv4/6
packets in these cases.

Unfortunately we can not easily determine if we are actually later
overwriting the affected fields. The approach now is to use the fields
from the IP header if we have a matching IP version and default to some
values otherwise. In case we overwrite this data afterwards we are
generally good. If we do not overwrite this data because of some bug we
will send out invalid ARP/NS requests. They will hopefully be dropped by
the rest of the network.

The alternative would have been to introduce new arp/nd_ns actions where
we guarantee this overwrite. This would not suffer from the above
limitations, but would require a coordination on upgrades between all
ovn-controllers and northd.

Signed-off-by: Felix Huettner <[email protected]>
Signed-off-by: Martin Kalcok <[email protected]>
Co-authored-by: Martin Kalcok <[email protected]>
Signed-off-by: Dumitru Ceara <[email protected]>
  • Loading branch information
@felixhuettner @mkalcok @dceara
2 people authored and dceara committed Dec 17, 2024
1 parent 5599242 commit 9fa1f90
Show file tree
Hide file tree
Showing 6 changed files with 648 additions and 21 deletions.
52 changes: 44 additions & 8 deletions controller/pinctrl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1567,9 +1567,11 @@ pinctrl_handle_arp(struct rconn *swconn, const struct flow *ip_flow,
const struct ofputil_packet_in *pin,
struct ofpbuf *userdata, const struct ofpbuf *continuation)
{
/* This action only works for IP packets, and the switch should only send
* us IP packets this way, but check here just to be sure. */
if (ip_flow->dl_type != htons(ETH_TYPE_IP)) {
uint16_t dl_type = ntohs(ip_flow->dl_type);

/* This action only works for IPv4 or IPv6 packets, and the switch should
* only send us IP packets this way, but check here just to be sure. */
if (dl_type != ETH_TYPE_IP && dl_type != ETH_TYPE_IPV6) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "ARP action on non-IP packet (Ethertype %"PRIx16")",
ntohs(ip_flow->dl_type));
Expand All @@ -1593,9 +1595,25 @@ pinctrl_handle_arp(struct rconn *swconn, const struct flow *ip_flow,
struct arp_eth_header *arp = dp_packet_l3(&packet);
arp->ar_op = htons(ARP_OP_REQUEST);
arp->ar_sha = ip_flow->dl_src;
put_16aligned_be32(&arp->ar_spa, ip_flow->nw_src);
arp->ar_tha = eth_addr_zero;
put_16aligned_be32(&arp->ar_tpa, ip_flow->nw_dst);

/* We might be here without actually currently handling an IPv4 packet.
* This can happen in the case where we route IPv6 packets over an IPv4
* link.
* In these cases we have no destination IPv4 address from the packet that
* we can reuse. But we receive the actual destination IPv4 address via
* userdata anyway, so what we set for now is irrelevant.
* This is just a hope since we do not parse the userdata. If we land here
* for whatever reason without being an IPv4 packet and without userdata we
* will send out a wrong packet.
*/
if (ip_flow->dl_type == htons(ETH_TYPE_IP)) {
put_16aligned_be32(&arp->ar_spa, ip_flow->nw_src);
put_16aligned_be32(&arp->ar_tpa, ip_flow->nw_dst);
} else {
put_16aligned_be32(&arp->ar_spa, 0);
put_16aligned_be32(&arp->ar_tpa, 0);
}

if (ip_flow->vlans[0].tci & htons(VLAN_CFI)) {
eth_push_vlan(&packet, htons(ETH_TYPE_VLAN_8021Q),
Expand Down Expand Up @@ -6620,8 +6638,11 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const struct flow *ip_flow,
struct ofpbuf *userdata,
const struct ofpbuf *continuation)
{
/* This action only works for IPv6 packets. */
if (get_dl_type(ip_flow) != htons(ETH_TYPE_IPV6)) {
uint16_t dl_type = ntohs(ip_flow->dl_type);

/* This action only works for IPv4 or IPv6 packets, and the switch should
* only send us IP packets this way, but check here just to be sure. */
if (dl_type != ETH_TYPE_IP && dl_type != ETH_TYPE_IPV6) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "NS action on non-IPv6 packet");
return;
Expand All @@ -6637,8 +6658,23 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const struct flow *ip_flow,
dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);

in6_generate_lla(ip_flow->dl_src, &ipv6_src);

/* We might be here without actually currently handling an IPv6 packet.
* This can happen in the case where we route IPv4 packets over an IPv6
* link.
* In these cases we have no destination IPv6 address from the packet that
* we can reuse. But we receive the actual destination IPv6 address via
* userdata anyway, so what we pass to compose_nd_ns is irrelevant.
* This is just a hope since we do not parse the userdata. If we land here
* for whatever reason without being an IPv6 packet and without userdata we
* will send out a wrong packet.
*/
struct in6_addr ipv6_dst = IN6ADDR_EXACT_INIT;
if (get_dl_type(ip_flow) == htons(ETH_TYPE_IPV6)) {
ipv6_dst = ip_flow->ipv6_dst;
}
compose_nd_ns(&packet, ip_flow->dl_src, &ipv6_src,
&ip_flow->ipv6_dst);
&ipv6_dst);

/* Reload previous packet metadata and set actions from userdata. */
set_actions_and_enqueue_msg(swconn, &packet,
Expand Down
4 changes: 2 additions & 2 deletions lib/actions.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1765,7 +1765,7 @@ parse_nested_action(struct action_context *ctx, enum ovnact_type type,
static void
parse_ARP(struct action_context *ctx)
{
parse_nested_action(ctx, OVNACT_ARP, "ip4", ctx->scope);
parse_nested_action(ctx, OVNACT_ARP, "ip", ctx->scope);
}

static void
Expand Down Expand Up @@ -1819,7 +1819,7 @@ parse_ND_NA_ROUTER(struct action_context *ctx)
static void
parse_ND_NS(struct action_context *ctx)
{
parse_nested_action(ctx, OVNACT_ND_NS, "ip6", ctx->scope);
parse_nested_action(ctx, OVNACT_ND_NS, "ip", ctx->scope);
}

static void
Expand Down
9 changes: 6 additions & 3 deletions northd/northd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15001,7 +15001,8 @@ build_arp_request_flows_for_lrouter(

ds_clear(match);
ds_put_format(match, "eth.dst == 00:00:00:00:00:00 && "
"ip6 && " REG_NEXT_HOP_IPV6 " == %s",
REGBIT_NEXTHOP_IS_IPV4" == 0 && "
REG_NEXT_HOP_IPV6 " == %s",
route->nexthop);
struct in6_addr sn_addr;
struct eth_addr eth_dst;
Expand Down Expand Up @@ -15031,7 +15032,8 @@ build_arp_request_flows_for_lrouter(
}

ovn_lflow_metered(lflows, od, S_ROUTER_IN_ARP_REQUEST, 100,
"eth.dst == 00:00:00:00:00:00 && ip4",
"eth.dst == 00:00:00:00:00:00 && "
REGBIT_NEXTHOP_IS_IPV4" == 1",
"arp { "
"eth.dst = ff:ff:ff:ff:ff:ff; "
"arp.spa = " REG_SRC_IPV4 "; "
Expand All @@ -15043,7 +15045,8 @@ build_arp_request_flows_for_lrouter(
meter_groups),
lflow_ref);
ovn_lflow_metered(lflows, od, S_ROUTER_IN_ARP_REQUEST, 100,
"eth.dst == 00:00:00:00:00:00 && ip6",
"eth.dst == 00:00:00:00:00:00 && "
REGBIT_NEXTHOP_IS_IPV4" == 0",
"nd_ns { "
"nd.target = " REG_NEXT_HOP_IPV6 "; "
"output; "
Expand Down
8 changes: 4 additions & 4 deletions tests/ovn-northd.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6979,10 +6979,10 @@ AT_CHECK([grep -e "lr_in_arp_resolve" lr0flows | ovn_strip_lflows], [0], [dnl

AT_CHECK([grep -e "lr_in_arp_request" lr0flows | ovn_strip_lflows], [0], [dnl
table=??(lr_in_arp_request ), priority=0 , match=(1), action=(output;)
table=??(lr_in_arp_request ), priority=100 , match=(eth.dst == 00:00:00:00:00:00 && ip4), action=(arp { eth.dst = ff:ff:ff:ff:ff:ff; arp.spa = reg5; arp.tpa = reg0; arp.op = 1; output; }; output;)
table=??(lr_in_arp_request ), priority=100 , match=(eth.dst == 00:00:00:00:00:00 && ip6), action=(nd_ns { nd.target = xxreg0; output; }; output;)
table=??(lr_in_arp_request ), priority=200 , match=(eth.dst == 00:00:00:00:00:00 && ip6 && xxreg0 == 2001:db8::10), action=(nd_ns { eth.dst = 33:33:ff:00:00:10; ip6.dst = ff02::1:ff00:10; nd.target = 2001:db8::10; output; }; output;)
table=??(lr_in_arp_request ), priority=200 , match=(eth.dst == 00:00:00:00:00:00 && ip6 && xxreg0 == 2001:db8::20), action=(nd_ns { eth.dst = 33:33:ff:00:00:20; ip6.dst = ff02::1:ff00:20; nd.target = 2001:db8::20; output; }; output;)
table=??(lr_in_arp_request ), priority=100 , match=(eth.dst == 00:00:00:00:00:00 && reg9[[9]] == 0), action=(nd_ns { nd.target = xxreg0; output; }; output;)
table=??(lr_in_arp_request ), priority=100 , match=(eth.dst == 00:00:00:00:00:00 && reg9[[9]] == 1), action=(arp { eth.dst = ff:ff:ff:ff:ff:ff; arp.spa = reg5; arp.tpa = reg0; arp.op = 1; output; }; output;)
table=??(lr_in_arp_request ), priority=200 , match=(eth.dst == 00:00:00:00:00:00 && reg9[[9]] == 0 && xxreg0 == 2001:db8::10), action=(nd_ns { eth.dst = 33:33:ff:00:00:10; ip6.dst = ff02::1:ff00:10; nd.target = 2001:db8::10; output; }; output;)
table=??(lr_in_arp_request ), priority=200 , match=(eth.dst == 00:00:00:00:00:00 && reg9[[9]] == 0 && xxreg0 == 2001:db8::20), action=(nd_ns { eth.dst = 33:33:ff:00:00:20; ip6.dst = ff02::1:ff00:20; nd.target = 2001:db8::20; output; }; output;)
])

AT_CLEANUP
Expand Down
Loading

0 comments on commit 9fa1f90

Please sign in to comment.