| Message ID | 1564392607-27395-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev,ovn,v2] OVN: Fix learning of neighbors from ARP/ND packets. | expand |
On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara <dceara@redhat.com> wrote: > > Add a restriction on the target protocol addresses to match the > configured subnets. All other ARP/ND packets are invalid in this > context. > > One exception is for ARP replies that are received for route next-hops > that are only reachable via a port but can't be directly resolved > through route lookups. Such support was introduced by commit: > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.") > > Reported-at: https://bugzilla.redhat.com/1729846 > Reported-by: Haidong Li <haili@redhat.com> > CC: Han Zhou <zhouhan@gmail.com> > CC: Guru Shetty <guru@ovn.org> > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP request.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Hi Dumitru, Sorry for my slow response, and thanks a lot for revising the patch for a bigger scope of validations. However, the exception of /32 address makes me thinking more about this patch. If ARP replies is allowed from any nexthop for a LR port with /32, at least ARP request for GARP purpose should also be allowed. But before asking for a v3, I'd hold on to rethink the purpose of this patch. The nexthop specific flows are now from static routes. What if OVN support dynamical routing protocols in the future? Of course we can then take those dynamic nexthops into allowed peers. But then I am thinking what is the real benefit of all these restrictions? Why can't we just have simpler logic to handle all these situations without validation? I think the major benefit of the validation is to avoid handling the noise ARP/NDs when multiple subnets shares same L2, but most cases it is really not a big deal, right? For the CPU problem caused by ARP flooding as mentioned by the bug report, it is a real problem, but this patch seems not really helpful for that, because the attacker can just trigger the same CPU problem with *valid* packets. So I am not sure if the benefit of the change is worth the complexity it introduced. Please share your thought and correct me if I missed something. Thanks, Han
On Mon, Aug 5, 2019 at 1:41 AM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > Add a restriction on the target protocol addresses to match the > > configured subnets. All other ARP/ND packets are invalid in this > > context. > > > > One exception is for ARP replies that are received for route next-hops > > that are only reachable via a port but can't be directly resolved > > through route lookups. Such support was introduced by commit: > > > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.") > > > > Reported-at: https://bugzilla.redhat.com/1729846 > > Reported-by: Haidong Li <haili@redhat.com> > > CC: Han Zhou <zhouhan@gmail.com> > > CC: Guru Shetty <guru@ovn.org> > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP request.") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Hi Dumitru, Hi Han, Thanks for reviewing this. > > Sorry for my slow response, and thanks a lot for revising the patch for a bigger scope of validations. However, the exception of /32 address makes me thinking more about this patch. If ARP replies is allowed from any nexthop for a LR port with /32, at least ARP request for GARP purpose should also be allowed. But before asking for a v3, I'd hold on to rethink the purpose of this patch. Right, we should allow GARP requests too. If we decide to go ahead with this patch I'll add a function in v3 to handle all types of ARPs and call it both for unreachable static route next-hops and attached subnets. > > The nexthop specific flows are now from static routes. What if OVN support dynamical routing protocols in the future? Of course we can then take those dynamic nexthops into allowed peers. But then I am thinking what is the real benefit of all these restrictions? Why can't we just have simpler logic to handle all these situations without validation? I think the major benefit of the validation is to avoid handling the noise ARP/NDs when multiple subnets shares same L2, but most cases it is really not a big deal, right? For the CPU problem caused by ARP flooding as mentioned by the bug report, it is a real problem, but this patch seems not really helpful for that, because the attacker can just trigger the same CPU problem with *valid* packets. So I am not sure if the benefit of the change is worth the complexity it introduced. Please share your thought and correct me if I missed something. > > Thanks, > Han I assume the simpler logic to handle all these situations without validation is to add rate limiting for ARP packets that get punted to the controller. I agree that this should be implemented too. But I think rate limiting all ARP packets regardless of IP addresses is not enough. In the following scenario and if we would have a way to rate limit ARP packets: - Subnet 42.42.42.0/24 configured on the OVN - "Invalid" ARP packets are injected at high rate in the network for 41.41.41.41 - Host 42.42.42.42 sends GARP. - Rate limiting of ARP packets towards controller at 100pps With the current code, ARP packets for 41.41.41.41 will be punted to controller at a rate of at most 100 per second. But the chances that the valid 42.42.42.42 GARP is dropped is really high. If we instead use the following approach: a. Punt only useful ARPs to controller. b. Rate limit ARPs that are sent to controller. Then ARP packets outside 42.42.42./24 are never punted to the controller and don't consume any rate limiting tokens. For the second case, when an attacker would flood with valid ARP packets we would have the rate limit in place to protect the controller CPU. My commit addresses point "a" above as I think point "b" should be done in a generic way for all protocol packets that need to reach the controller. For dynamic routing protocols on the other hand I think we should not install routes with next-hops that are unreachable through other direct or indirect routes. Thanks, Dumitru
On Mon, Aug 5, 2019 at 1:06 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On Mon, Aug 5, 2019 at 1:41 AM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > > > Add a restriction on the target protocol addresses to match the > > > configured subnets. All other ARP/ND packets are invalid in this > > > context. > > > > > > One exception is for ARP replies that are received for route next-hops > > > that are only reachable via a port but can't be directly resolved > > > through route lookups. Such support was introduced by commit: > > > > > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.") > > > > > > Reported-at: https://bugzilla.redhat.com/1729846 > > > Reported-by: Haidong Li <haili@redhat.com> > > > CC: Han Zhou <zhouhan@gmail.com> > > > CC: Guru Shetty <guru@ovn.org> > > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP request.") > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > > Hi Dumitru, > > Hi Han, > > Thanks for reviewing this. > > > > > Sorry for my slow response, and thanks a lot for revising the patch for a bigger scope of validations. However, the exception of /32 address makes me thinking more about this patch. If ARP replies is allowed from any nexthop for a LR port with /32, at least ARP request for GARP purpose should also be allowed. But before asking for a v3, I'd hold on to rethink the purpose of this patch. > > Right, we should allow GARP requests too. If we decide to go ahead > with this patch I'll add a function in v3 to handle all types of ARPs > and call it both for unreachable static route next-hops and attached > subnets. > > > > > The nexthop specific flows are now from static routes. What if OVN support dynamical routing protocols in the future? Of course we can then take those dynamic nexthops into allowed peers. But then I am thinking what is the real benefit of all these restrictions? Why can't we just have simpler logic to handle all these situations without validation? I think the major benefit of the validation is to avoid handling the noise ARP/NDs when multiple subnets shares same L2, but most cases it is really not a big deal, right? For the CPU problem caused by ARP flooding as mentioned by the bug report, it is a real problem, but this patch seems not really helpful for that, because the attacker can just trigger the same CPU problem with *valid* packets. So I am not sure if the benefit of the change is worth the complexity it introduced. Please share your thought and correct me if I missed something. > > > > Thanks, > > Han > > I assume the simpler logic to handle all these situations without > validation is to add rate limiting for ARP packets that get punted to > the controller. I agree that this should be implemented too. > > But I think rate limiting all ARP packets regardless of IP addresses > is not enough. In the following scenario and if we would have a way to > rate limit ARP packets: > - Subnet 42.42.42.0/24 configured on the OVN > - "Invalid" ARP packets are injected at high rate in the network for 41.41.41.41 > - Host 42.42.42.42 sends GARP. > - Rate limiting of ARP packets towards controller at 100pps > > With the current code, ARP packets for 41.41.41.41 will be punted to > controller at a rate of at most 100 per second. But the chances that > the valid 42.42.42.42 GARP is dropped is really high. > > If we instead use the following approach: > a. Punt only useful ARPs to controller. > b. Rate limit ARPs that are sent to controller. > > Then ARP packets outside 42.42.42./24 are never punted to the > controller and don't consume any rate limiting tokens. For the second > case, when an attacker would flood with valid ARP packets we would > have the rate limit in place to protect the controller CPU. > > My commit addresses point "a" above as I think point "b" should be > done in a generic way for all protocol packets that need to reach the > controller. > > For dynamic routing protocols on the other hand I think we should not > install routes with next-hops that are unreachable through other > direct or indirect routes. > > Thanks, > Dumitru I agree that blindly ARP rate limit is not helpful, but a) is not really helpful in this case either. In your example, the attacker can just use any valid IP in 42.42.42.0/24 to send GARP flooding, which would result in exactly same result that a useful GARP from 42.42.42.42 is dropped because of blindly rate limiting all ARPs. To solve the problem properly, the ARP rate limiting must be done per IP.
On Mon, Aug 5, 2019 at 5:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Mon, Aug 5, 2019 at 1:06 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > On Mon, Aug 5, 2019 at 1:41 AM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > > > > > Add a restriction on the target protocol addresses to match the > > > > configured subnets. All other ARP/ND packets are invalid in this > > > > context. > > > > > > > > One exception is for ARP replies that are received for route next-hops > > > > that are only reachable via a port but can't be directly resolved > > > > through route lookups. Such support was introduced by commit: > > > > > > > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.") > > > > > > > > Reported-at: https://bugzilla.redhat.com/1729846 > > > > Reported-by: Haidong Li <haili@redhat.com> > > > > CC: Han Zhou <zhouhan@gmail.com> > > > > CC: Guru Shetty <guru@ovn.org> > > > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP request.") > > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > > > > Hi Dumitru, > > > > Hi Han, > > > > Thanks for reviewing this. > > > > > > > > Sorry for my slow response, and thanks a lot for revising the patch for a bigger scope of validations. However, the exception of /32 address makes me thinking more about this patch. If ARP replies is allowed from any nexthop for a LR port with /32, at least ARP request for GARP purpose should also be allowed. But before asking for a v3, I'd hold on to rethink the purpose of this patch. > > > > Right, we should allow GARP requests too. If we decide to go ahead > > with this patch I'll add a function in v3 to handle all types of ARPs > > and call it both for unreachable static route next-hops and attached > > subnets. > > > > > > > > The nexthop specific flows are now from static routes. What if OVN support dynamical routing protocols in the future? Of course we can then take those dynamic nexthops into allowed peers. But then I am thinking what is the real benefit of all these restrictions? Why can't we just have simpler logic to handle all these situations without validation? I think the major benefit of the validation is to avoid handling the noise ARP/NDs when multiple subnets shares same L2, but most cases it is really not a big deal, right? For the CPU problem caused by ARP flooding as mentioned by the bug report, it is a real problem, but this patch seems not really helpful for that, because the attacker can just trigger the same CPU problem with *valid* packets. So I am not sure if the benefit of the change is worth the complexity it introduced. Please share your thought and correct me if I missed something. > > > > > > Thanks, > > > Han > > > > I assume the simpler logic to handle all these situations without > > validation is to add rate limiting for ARP packets that get punted to > > the controller. I agree that this should be implemented too. > > > > But I think rate limiting all ARP packets regardless of IP addresses > > is not enough. In the following scenario and if we would have a way to > > rate limit ARP packets: > > - Subnet 42.42.42.0/24 configured on the OVN > > - "Invalid" ARP packets are injected at high rate in the network for 41.41.41.41 > > - Host 42.42.42.42 sends GARP. > > - Rate limiting of ARP packets towards controller at 100pps > > > > With the current code, ARP packets for 41.41.41.41 will be punted to > > controller at a rate of at most 100 per second. But the chances that > > the valid 42.42.42.42 GARP is dropped is really high. > > > > If we instead use the following approach: > > a. Punt only useful ARPs to controller. > > b. Rate limit ARPs that are sent to controller. > > > > Then ARP packets outside 42.42.42./24 are never punted to the > > controller and don't consume any rate limiting tokens. For the second > > case, when an attacker would flood with valid ARP packets we would > > have the rate limit in place to protect the controller CPU. > > > > My commit addresses point "a" above as I think point "b" should be > > done in a generic way for all protocol packets that need to reach the > > controller. > > > > For dynamic routing protocols on the other hand I think we should not > > install routes with next-hops that are unreachable through other > > direct or indirect routes. > > > > Thanks, > > Dumitru > > I agree that blindly ARP rate limit is not helpful, but a) is not really helpful in this case either. In your example, the attacker can just use any valid IP in 42.42.42.0/24 to send GARP flooding, which would result in exactly same result that a useful GARP from 42.42.42.42 is dropped because of blindly rate limiting all ARPs. To solve the problem properly, the ARP rate limiting must be done per IP. Ok, ideally ARP rate limiting should be done per IP but it would take quite a lot of resources to keep that information per host. Any idea how to implement that in an efficient way? There are scenarios when we don't know beforehand the IPs of the hosts running in the network so that we can whitelist them. Also, from what I've seen physical routers usually have a single queue for control plane protection for ARPs. Thanks, Dumitru
On Wed, Aug 7, 2019 at 8:12 AM Dumitru Ceara <dceara@redhat.com> wrote: > On Mon, Aug 5, 2019 at 5:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Mon, Aug 5, 2019 at 1:06 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > > > On Mon, Aug 5, 2019 at 1:41 AM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > > > > > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara <dceara@redhat.com> > wrote: > > > > > > > > > > Add a restriction on the target protocol addresses to match the > > > > > configured subnets. All other ARP/ND packets are invalid in this > > > > > context. > > > > > > > > > > One exception is for ARP replies that are received for route > next-hops > > > > > that are only reachable via a port but can't be directly resolved > > > > > through route lookups. Such support was introduced by commit: > > > > > > > > > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.") > > > > > > > > > > Reported-at: https://bugzilla.redhat.com/1729846 > > > > > Reported-by: Haidong Li <haili@redhat.com> > > > > > CC: Han Zhou <zhouhan@gmail.com> > > > > > CC: Guru Shetty <guru@ovn.org> > > > > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from > ARP request.") > > > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > > > > > > Hi Dumitru, > > > > > > Hi Han, > > > > > > Thanks for reviewing this. > > > > > > > > > > > Sorry for my slow response, and thanks a lot for revising the patch > for a bigger scope of validations. However, the exception of /32 address > makes me thinking more about this patch. If ARP replies is allowed from any > nexthop for a LR port with /32, at least ARP request for GARP purpose > should also be allowed. But before asking for a v3, I'd hold on to rethink > the purpose of this patch. > > > > > > Right, we should allow GARP requests too. If we decide to go ahead > > > with this patch I'll add a function in v3 to handle all types of ARPs > > > and call it both for unreachable static route next-hops and attached > > > subnets. > > > > > > > > > > > The nexthop specific flows are now from static routes. What if OVN > support dynamical routing protocols in the future? Of course we can then > take those dynamic nexthops into allowed peers. But then I am thinking what > is the real benefit of all these restrictions? Why can't we just have > simpler logic to handle all these situations without validation? I think > the major benefit of the validation is to avoid handling the noise ARP/NDs > when multiple subnets shares same L2, but most cases it is really not a big > deal, right? For the CPU problem caused by ARP flooding as mentioned by the > bug report, it is a real problem, but this patch seems not really helpful > for that, because the attacker can just trigger the same CPU problem with > *valid* packets. So I am not sure if the benefit of the change is worth the > complexity it introduced. Please share your thought and correct me if I > missed something. > > > > > > > > Thanks, > > > > Han > > > > > > I assume the simpler logic to handle all these situations without > > > validation is to add rate limiting for ARP packets that get punted to > > > the controller. I agree that this should be implemented too. > > > > > > But I think rate limiting all ARP packets regardless of IP addresses > > > is not enough. In the following scenario and if we would have a way to > > > rate limit ARP packets: > > > - Subnet 42.42.42.0/24 configured on the OVN > > > - "Invalid" ARP packets are injected at high rate in the network for > 41.41.41.41 > > > - Host 42.42.42.42 sends GARP. > > > - Rate limiting of ARP packets towards controller at 100pps > > > > > > With the current code, ARP packets for 41.41.41.41 will be punted to > > > controller at a rate of at most 100 per second. But the chances that > > > the valid 42.42.42.42 GARP is dropped is really high. > > > > > > If we instead use the following approach: > > > a. Punt only useful ARPs to controller. > > > b. Rate limit ARPs that are sent to controller. > > > > > > Then ARP packets outside 42.42.42./24 are never punted to the > > > controller and don't consume any rate limiting tokens. For the second > > > case, when an attacker would flood with valid ARP packets we would > > > have the rate limit in place to protect the controller CPU. > > > > > > My commit addresses point "a" above as I think point "b" should be > > > done in a generic way for all protocol packets that need to reach the > > > controller. > > > > > > For dynamic routing protocols on the other hand I think we should not > > > install routes with next-hops that are unreachable through other > > > direct or indirect routes. > > > > > > Thanks, > > > Dumitru > > > > I agree that blindly ARP rate limit is not helpful, but a) is not really > helpful in this case either. In your example, the attacker can just use any > valid IP in 42.42.42.0/24 to send GARP flooding, which would result in > exactly same result that a useful GARP from 42.42.42.42 is dropped because > of blindly rate limiting all ARPs. To solve the problem properly, the ARP > rate limiting must be done per IP. > > Ok, ideally ARP rate limiting should be done per IP but it would take > quite a lot of resources to keep that information per host. > > Any idea how to implement that in an efficient way? There are > scenarios when we don't know beforehand the IPs of the hosts running > in the network so that we can whitelist them. Also, from what I've > seen physical routers usually have a single queue for control plane > protection for ARPs. > > Thanks, > Dumitru > Yes, I agree that system resource is a challenge for per IP rate limiting. And yes there is no way to whitelist (because otherwise ARP is not needed). We may make some trade-off between the accuracy and efficiency. For example, we can have separate meter groups for each logical router port for ARP ratelimiting. Each meter group may have e.g. 64 meters, and then having a stage to do hash for the IPs, and in the next stage using the hash result (between 0 - 63) as index to use corresponding meter to do ratelimiting. This way, flooding to a specific IP will impact only the other IPs that fall into the same hash bucket on the same router port. But this is just a rough idea and I believe many more details still need to be figured out for the hashing part. As a first step maybe we can just do ratelimiting per router port. It is definitely better than nothing. What do you think?
On Thu, Aug 8, 2019 at 1:52 AM Han Zhou <zhouhan@gmail.com> wrote: > > > > On Wed, Aug 7, 2019 at 8:12 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On Mon, Aug 5, 2019 at 5:34 PM Han Zhou <zhouhan@gmail.com> wrote: >> > >> > >> > >> > On Mon, Aug 5, 2019 at 1:06 AM Dumitru Ceara <dceara@redhat.com> wrote: >> > > >> > > On Mon, Aug 5, 2019 at 1:41 AM Han Zhou <zhouhan@gmail.com> wrote: >> > > > >> > > > >> > > > >> > > > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara <dceara@redhat.com> wrote: >> > > > > >> > > > > Add a restriction on the target protocol addresses to match the >> > > > > configured subnets. All other ARP/ND packets are invalid in this >> > > > > context. >> > > > > >> > > > > One exception is for ARP replies that are received for route next-hops >> > > > > that are only reachable via a port but can't be directly resolved >> > > > > through route lookups. Such support was introduced by commit: >> > > > > >> > > > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.") >> > > > > >> > > > > Reported-at: https://bugzilla.redhat.com/1729846 >> > > > > Reported-by: Haidong Li <haili@redhat.com> >> > > > > CC: Han Zhou <zhouhan@gmail.com> >> > > > > CC: Guru Shetty <guru@ovn.org> >> > > > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP request.") >> > > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> > > > >> > > > Hi Dumitru, >> > > >> > > Hi Han, >> > > >> > > Thanks for reviewing this. >> > > >> > > > >> > > > Sorry for my slow response, and thanks a lot for revising the patch for a bigger scope of validations. However, the exception of /32 address makes me thinking more about this patch. If ARP replies is allowed from any nexthop for a LR port with /32, at least ARP request for GARP purpose should also be allowed. But before asking for a v3, I'd hold on to rethink the purpose of this patch. >> > > >> > > Right, we should allow GARP requests too. If we decide to go ahead >> > > with this patch I'll add a function in v3 to handle all types of ARPs >> > > and call it both for unreachable static route next-hops and attached >> > > subnets. >> > > >> > > > >> > > > The nexthop specific flows are now from static routes. What if OVN support dynamical routing protocols in the future? Of course we can then take those dynamic nexthops into allowed peers. But then I am thinking what is the real benefit of all these restrictions? Why can't we just have simpler logic to handle all these situations without validation? I think the major benefit of the validation is to avoid handling the noise ARP/NDs when multiple subnets shares same L2, but most cases it is really not a big deal, right? For the CPU problem caused by ARP flooding as mentioned by the bug report, it is a real problem, but this patch seems not really helpful for that, because the attacker can just trigger the same CPU problem with *valid* packets. So I am not sure if the benefit of the change is worth the complexity it introduced. Please share your thought and correct me if I missed something. >> > > > >> > > > Thanks, >> > > > Han >> > > >> > > I assume the simpler logic to handle all these situations without >> > > validation is to add rate limiting for ARP packets that get punted to >> > > the controller. I agree that this should be implemented too. >> > > >> > > But I think rate limiting all ARP packets regardless of IP addresses >> > > is not enough. In the following scenario and if we would have a way to >> > > rate limit ARP packets: >> > > - Subnet 42.42.42.0/24 configured on the OVN >> > > - "Invalid" ARP packets are injected at high rate in the network for 41.41.41.41 >> > > - Host 42.42.42.42 sends GARP. >> > > - Rate limiting of ARP packets towards controller at 100pps >> > > >> > > With the current code, ARP packets for 41.41.41.41 will be punted to >> > > controller at a rate of at most 100 per second. But the chances that >> > > the valid 42.42.42.42 GARP is dropped is really high. >> > > >> > > If we instead use the following approach: >> > > a. Punt only useful ARPs to controller. >> > > b. Rate limit ARPs that are sent to controller. >> > > >> > > Then ARP packets outside 42.42.42./24 are never punted to the >> > > controller and don't consume any rate limiting tokens. For the second >> > > case, when an attacker would flood with valid ARP packets we would >> > > have the rate limit in place to protect the controller CPU. >> > > >> > > My commit addresses point "a" above as I think point "b" should be >> > > done in a generic way for all protocol packets that need to reach the >> > > controller. >> > > >> > > For dynamic routing protocols on the other hand I think we should not >> > > install routes with next-hops that are unreachable through other >> > > direct or indirect routes. >> > > >> > > Thanks, >> > > Dumitru >> > >> > I agree that blindly ARP rate limit is not helpful, but a) is not really helpful in this case either. In your example, the attacker can just use any valid IP in 42.42.42.0/24 to send GARP flooding, which would result in exactly same result that a useful GARP from 42.42.42.42 is dropped because of blindly rate limiting all ARPs. To solve the problem properly, the ARP rate limiting must be done per IP. >> >> Ok, ideally ARP rate limiting should be done per IP but it would take >> quite a lot of resources to keep that information per host. >> >> Any idea how to implement that in an efficient way? There are >> scenarios when we don't know beforehand the IPs of the hosts running >> in the network so that we can whitelist them. Also, from what I've >> seen physical routers usually have a single queue for control plane >> protection for ARPs. >> >> Thanks, >> Dumitru > > > Yes, I agree that system resource is a challenge for per IP rate limiting. And yes there is no way to whitelist (because otherwise ARP is not needed). We may make some trade-off between the accuracy and efficiency. For example, we can have separate meter groups for each logical router port for ARP ratelimiting. Each meter group may have e.g. 64 meters, and then having a stage to do hash for the IPs, and in the next stage using the hash result (between 0 - 63) as index to use corresponding meter to do ratelimiting. This way, flooding to a specific IP will impact only the other IPs that fall into the same hash bucket on the same router port. But this is just a rough idea and I believe many more details still need to be figured out for the hashing part. As a first step maybe we can just do ratelimiting per router port. It is definitely better than nothing. What do you think? Hi Han, It sounds like a good idea to have a hashtable for rate limiting groups of ARPs. I will start like you suggested with a single bucket per router port. The question is if we still want to punt only ARPs for configured networks and nexthops like this patch is trying to do. I would add it and then implement rate limiting as a follow up patch/series. Thanks, Dumitru
On Wed, Aug 14, 2019 at 11:11 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On Thu, Aug 8, 2019 at 1:52 AM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > On Wed, Aug 7, 2019 at 8:12 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> On Mon, Aug 5, 2019 at 5:34 PM Han Zhou <zhouhan@gmail.com> wrote: > >> > > >> > > >> > > >> > On Mon, Aug 5, 2019 at 1:06 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > > > >> > > On Mon, Aug 5, 2019 at 1:41 AM Han Zhou <zhouhan@gmail.com> wrote: > >> > > > > >> > > > > >> > > > > >> > > > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara <dceara@redhat.com> wrote: > >> > > > > > >> > > > > Add a restriction on the target protocol addresses to match the > >> > > > > configured subnets. All other ARP/ND packets are invalid in this > >> > > > > context. > >> > > > > > >> > > > > One exception is for ARP replies that are received for route next-hops > >> > > > > that are only reachable via a port but can't be directly resolved > >> > > > > through route lookups. Such support was introduced by commit: > >> > > > > > >> > > > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.") > >> > > > > > >> > > > > Reported-at: https://bugzilla.redhat.com/1729846 > >> > > > > Reported-by: Haidong Li <haili@redhat.com> > >> > > > > CC: Han Zhou <zhouhan@gmail.com> > >> > > > > CC: Guru Shetty <guru@ovn.org> > >> > > > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP request.") > >> > > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > >> > > > > >> > > > Hi Dumitru, > >> > > > >> > > Hi Han, > >> > > > >> > > Thanks for reviewing this. > >> > > > >> > > > > >> > > > Sorry for my slow response, and thanks a lot for revising the patch for a bigger scope of validations. However, the exception of /32 address makes me thinking more about this patch. If ARP replies is allowed from any nexthop for a LR port with /32, at least ARP request for GARP purpose should also be allowed. But before asking for a v3, I'd hold on to rethink the purpose of this patch. > >> > > > >> > > Right, we should allow GARP requests too. If we decide to go ahead > >> > > with this patch I'll add a function in v3 to handle all types of ARPs > >> > > and call it both for unreachable static route next-hops and attached > >> > > subnets. > >> > > > >> > > > > >> > > > The nexthop specific flows are now from static routes. What if OVN support dynamical routing protocols in the future? Of course we can then take those dynamic nexthops into allowed peers. But then I am thinking what is the real benefit of all these restrictions? Why can't we just have simpler logic to handle all these situations without validation? I think the major benefit of the validation is to avoid handling the noise ARP/NDs when multiple subnets shares same L2, but most cases it is really not a big deal, right? For the CPU problem caused by ARP flooding as mentioned by the bug report, it is a real problem, but this patch seems not really helpful for that, because the attacker can just trigger the same CPU problem with *valid* packets. So I am not sure if the benefit of the change is worth the complexity it introduced. Please share your thought and correct me if I missed something. > >> > > > > >> > > > Thanks, > >> > > > Han > >> > > > >> > > I assume the simpler logic to handle all these situations without > >> > > validation is to add rate limiting for ARP packets that get punted to > >> > > the controller. I agree that this should be implemented too. > >> > > > >> > > But I think rate limiting all ARP packets regardless of IP addresses > >> > > is not enough. In the following scenario and if we would have a way to > >> > > rate limit ARP packets: > >> > > - Subnet 42.42.42.0/24 configured on the OVN > >> > > - "Invalid" ARP packets are injected at high rate in the network for 41.41.41.41 > >> > > - Host 42.42.42.42 sends GARP. > >> > > - Rate limiting of ARP packets towards controller at 100pps > >> > > > >> > > With the current code, ARP packets for 41.41.41.41 will be punted to > >> > > controller at a rate of at most 100 per second. But the chances that > >> > > the valid 42.42.42.42 GARP is dropped is really high. > >> > > > >> > > If we instead use the following approach: > >> > > a. Punt only useful ARPs to controller. > >> > > b. Rate limit ARPs that are sent to controller. > >> > > > >> > > Then ARP packets outside 42.42.42./24 are never punted to the > >> > > controller and don't consume any rate limiting tokens. For the second > >> > > case, when an attacker would flood with valid ARP packets we would > >> > > have the rate limit in place to protect the controller CPU. > >> > > > >> > > My commit addresses point "a" above as I think point "b" should be > >> > > done in a generic way for all protocol packets that need to reach the > >> > > controller. > >> > > > >> > > For dynamic routing protocols on the other hand I think we should not > >> > > install routes with next-hops that are unreachable through other > >> > > direct or indirect routes. > >> > > > >> > > Thanks, > >> > > Dumitru > >> > > >> > I agree that blindly ARP rate limit is not helpful, but a) is not really helpful in this case either. In your example, the attacker can just use any valid IP in 42.42.42.0/24 to send GARP flooding, which would result in exactly same result that a useful GARP from 42.42.42.42 is dropped because of blindly rate limiting all ARPs. To solve the problem properly, the ARP rate limiting must be done per IP. > >> > >> Ok, ideally ARP rate limiting should be done per IP but it would take > >> quite a lot of resources to keep that information per host. > >> > >> Any idea how to implement that in an efficient way? There are > >> scenarios when we don't know beforehand the IPs of the hosts running > >> in the network so that we can whitelist them. Also, from what I've > >> seen physical routers usually have a single queue for control plane > >> protection for ARPs. > >> > >> Thanks, > >> Dumitru > > > > > > Yes, I agree that system resource is a challenge for per IP rate limiting. And yes there is no way to whitelist (because otherwise ARP is not needed). We may make some trade-off between the accuracy and efficiency. For example, we can have separate meter groups for each logical router port for ARP ratelimiting. Each meter group may have e.g. 64 meters, and then having a stage to do hash for the IPs, and in the next stage using the hash result (between 0 - 63) as index to use corresponding meter to do ratelimiting. This way, flooding to a specific IP will impact only the other IPs that fall into the same hash bucket on the same router port. But this is just a rough idea and I believe many more details still need to be figured out for the hashing part. As a first step maybe we can just do ratelimiting per router port. It is definitely better than nothing. What do you think? > > Hi Han, > > It sounds like a good idea to have a hashtable for rate limiting > groups of ARPs. I will start like you suggested with a single bucket > per router port. The question is if we still want to punt only ARPs > for configured networks and nexthops like this patch is trying to do. > I would add it and then implement rate limiting as a follow up > patch/series. > > Thanks, > Dumitru That's great. Thanks for working on ratelimiting. Regarding the current patch, it seems no obvious benefit, while it increases complexity, so I'd rather pause it for now, unless we see real problems of not doing it.
On Thu, Aug 15, 2019 at 12:24 PM Han Zhou <zhouhan@gmail.com> wrote: > On Wed, Aug 14, 2019 at 11:11 PM Dumitru Ceara <dceara@redhat.com> wrote: > > > > On Thu, Aug 8, 2019 at 1:52 AM Han Zhou <zhouhan@gmail.com> wrote: > > > > > > > > > > > > On Wed, Aug 7, 2019 at 8:12 AM Dumitru Ceara <dceara@redhat.com> > wrote: > > >> > > >> On Mon, Aug 5, 2019 at 5:34 PM Han Zhou <zhouhan@gmail.com> wrote: > > >> > > > >> > > > >> > > > >> > On Mon, Aug 5, 2019 at 1:06 AM Dumitru Ceara <dceara@redhat.com> > wrote: > > >> > > > > >> > > On Mon, Aug 5, 2019 at 1:41 AM Han Zhou <zhouhan@gmail.com> > wrote: > > >> > > > > > >> > > > > > >> > > > > > >> > > > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara < > dceara@redhat.com> > wrote: > > >> > > > > > > >> > > > > Add a restriction on the target protocol addresses to match > the > > >> > > > > configured subnets. All other ARP/ND packets are invalid in > this > > >> > > > > context. > > >> > > > > > > >> > > > > One exception is for ARP replies that are received for route > next-hops > > >> > > > > that are only reachable via a port but can't be directly > resolved > > >> > > > > through route lookups. Such support was introduced by commit: > > >> > > > > > > >> > > > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router > ports.") > > >> > > > > > > >> > > > > Reported-at: https://bugzilla.redhat.com/1729846 > > >> > > > > Reported-by: Haidong Li <haili@redhat.com> > > >> > > > > CC: Han Zhou <zhouhan@gmail.com> > > >> > > > > CC: Guru Shetty <guru@ovn.org> > > >> > > > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor > from ARP request.") > > >> > > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > >> > > > > > >> > > > Hi Dumitru, > > >> > > > > >> > > Hi Han, > > >> > > > > >> > > Thanks for reviewing this. > > >> > > > > >> > > > > > >> > > > Sorry for my slow response, and thanks a lot for revising the > patch for a bigger scope of validations. However, the exception of /32 > address makes me thinking more about this patch. If ARP replies is allowed > from any nexthop for a LR port with /32, at least ARP request for GARP > purpose should also be allowed. But before asking for a v3, I'd hold on to > rethink the purpose of this patch. > > >> > > > > >> > > Right, we should allow GARP requests too. If we decide to go ahead > > >> > > with this patch I'll add a function in v3 to handle all types of > ARPs > > >> > > and call it both for unreachable static route next-hops and > attached > > >> > > subnets. > > >> > > > > >> > > > > > >> > > > The nexthop specific flows are now from static routes. What if > OVN support dynamical routing protocols in the future? Of course we can > then take those dynamic nexthops into allowed peers. But then I am thinking > what is the real benefit of all these restrictions? Why can't we just have > simpler logic to handle all these situations without validation? I think > the major benefit of the validation is to avoid handling the noise ARP/NDs > when multiple subnets shares same L2, but most cases it is really not a big > deal, right? For the CPU problem caused by ARP flooding as mentioned by the > bug report, it is a real problem, but this patch seems not really helpful > for that, because the attacker can just trigger the same CPU problem with > *valid* packets. So I am not sure if the benefit of the change is worth the > complexity it introduced. Please share your thought and correct me if I > missed something. > > >> > > > > > >> > > > Thanks, > > >> > > > Han > > >> > > > > >> > > I assume the simpler logic to handle all these situations without > > >> > > validation is to add rate limiting for ARP packets that get punted > to > > >> > > the controller. I agree that this should be implemented too. > > >> > > > > >> > > But I think rate limiting all ARP packets regardless of IP > addresses > > >> > > is not enough. In the following scenario and if we would have a > way to > > >> > > rate limit ARP packets: > > >> > > - Subnet 42.42.42.0/24 configured on the OVN > > >> > > - "Invalid" ARP packets are injected at high rate in the network > for 41.41.41.41 > > >> > > - Host 42.42.42.42 sends GARP. > > >> > > - Rate limiting of ARP packets towards controller at 100pps > > >> > > > > >> > > With the current code, ARP packets for 41.41.41.41 will be punted > to > > >> > > controller at a rate of at most 100 per second. But the chances > that > > >> > > the valid 42.42.42.42 GARP is dropped is really high. > > >> > > > > >> > > If we instead use the following approach: > > >> > > a. Punt only useful ARPs to controller. > > >> > > b. Rate limit ARPs that are sent to controller. > > >> > > > > >> > > Then ARP packets outside 42.42.42./24 are never punted to the > > >> > > controller and don't consume any rate limiting tokens. For the > second > > >> > > case, when an attacker would flood with valid ARP packets we would > > >> > > have the rate limit in place to protect the controller CPU. > > >> > > > > >> > > My commit addresses point "a" above as I think point "b" should be > > >> > > done in a generic way for all protocol packets that need to reach > the > > >> > > controller. > > >> > > > > >> > > For dynamic routing protocols on the other hand I think we should > not > > >> > > install routes with next-hops that are unreachable through other > > >> > > direct or indirect routes. > > >> > > > > >> > > Thanks, > > >> > > Dumitru > > >> > > > >> > I agree that blindly ARP rate limit is not helpful, but a) is not > really helpful in this case either. In your example, the attacker can just > use any valid IP in 42.42.42.0/24 to send GARP flooding, which would > result > in exactly same result that a useful GARP from 42.42.42.42 is dropped > because of blindly rate limiting all ARPs. To solve the problem properly, > the ARP rate limiting must be done per IP. > > >> > > >> Ok, ideally ARP rate limiting should be done per IP but it would take > > >> quite a lot of resources to keep that information per host. > > >> > > >> Any idea how to implement that in an efficient way? There are > > >> scenarios when we don't know beforehand the IPs of the hosts running > > >> in the network so that we can whitelist them. Also, from what I've > > >> seen physical routers usually have a single queue for control plane > > >> protection for ARPs. > > >> > > >> Thanks, > > >> Dumitru > > > > > > > > > Yes, I agree that system resource is a challenge for per IP rate > limiting. And yes there is no way to whitelist (because otherwise ARP is > not needed). We may make some trade-off between the accuracy and > efficiency. For example, we can have separate meter groups for each logical > router port for ARP ratelimiting. Each meter group may have e.g. 64 meters, > and then having a stage to do hash for the IPs, and in the next stage using > the hash result (between 0 - 63) as index to use corresponding meter to do > ratelimiting. This way, flooding to a specific IP will impact only the > other IPs that fall into the same hash bucket on the same router port. But > this is just a rough idea and I believe many more details still need to be > figured out for the hashing part. As a first step maybe we can just do > ratelimiting per router port. It is definitely better than nothing. What do > you think? > > > > Hi Han, > > > > It sounds like a good idea to have a hashtable for rate limiting > > groups of ARPs. I will start like you suggested with a single bucket > > per router port. The question is if we still want to punt only ARPs > > for configured networks and nexthops like this patch is trying to do. > > I would add it and then implement rate limiting as a follow up > > patch/series. > > > > Thanks, > > Dumitru > > That's great. Thanks for working on ratelimiting. Regarding the current > patch, it seems no obvious benefit, while it increases complexity, so I'd > rather pause it for now, unless we see real problems of not doing it. > Hi Han and Dumitru, I didn't follow up on the discussions happening here until now. I have submitted a patch here - https://patchwork.ozlabs.org/patch/1161273/ which should solve the problem which Dumitru's patch is trying to handle. I started working on this patch to resolve the high cpu usage issue which we noticed recently for GARP reply packets from the physical switch. Just to brief, in the proposed patch, a new action is added - lookup_arp/lookup_nd which is very similar to the present actions - get_arp/get_nd. lookup_arp(inport, ip, mac) sets eth.dst to the 'mac', if (ip,mac) is found in the mac_binding table else it sets eth.dst to 00:00:00:00:00. If eth.dst is zero, it means we need to learn the mac and put_arp/put_nd is called. Otherwise put_arp/put_nd is not called. Request to take a look into that patch. Thanks Numan
On Wed, Sep 11, 2019 at 10:15 PM Numan Siddique <nusiddiq@redhat.com> wrote: > > > > On Thu, Aug 15, 2019 at 12:24 PM Han Zhou <zhouhan@gmail.com> wrote: >> >> On Wed, Aug 14, 2019 at 11:11 PM Dumitru Ceara <dceara@redhat.com> wrote: >> > >> > On Thu, Aug 8, 2019 at 1:52 AM Han Zhou <zhouhan@gmail.com> wrote: >> > > >> > > >> > > >> > > On Wed, Aug 7, 2019 at 8:12 AM Dumitru Ceara <dceara@redhat.com> wrote: >> > >> >> > >> On Mon, Aug 5, 2019 at 5:34 PM Han Zhou <zhouhan@gmail.com> wrote: >> > >> > >> > >> > >> > >> > >> > >> > On Mon, Aug 5, 2019 at 1:06 AM Dumitru Ceara <dceara@redhat.com> >> wrote: >> > >> > > >> > >> > > On Mon, Aug 5, 2019 at 1:41 AM Han Zhou <zhouhan@gmail.com> wrote: >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > On Mon, Jul 29, 2019 at 2:30 AM Dumitru Ceara <dceara@redhat.com> >> wrote: >> > >> > > > > >> > >> > > > > Add a restriction on the target protocol addresses to match the >> > >> > > > > configured subnets. All other ARP/ND packets are invalid in >> this >> > >> > > > > context. >> > >> > > > > >> > >> > > > > One exception is for ARP replies that are received for route >> next-hops >> > >> > > > > that are only reachable via a port but can't be directly >> resolved >> > >> > > > > through route lookups. Such support was introduced by commit: >> > >> > > > > >> > >> > > > > 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router >> ports.") >> > >> > > > > >> > >> > > > > Reported-at: https://bugzilla.redhat.com/1729846 >> > >> > > > > Reported-by: Haidong Li <haili@redhat.com> >> > >> > > > > CC: Han Zhou <zhouhan@gmail.com> >> > >> > > > > CC: Guru Shetty <guru@ovn.org> >> > >> > > > > Fixes: b068454082f5 ("ovn-northd: Support learning neighbor >> from ARP request.") >> > >> > > > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> > >> > > > >> > >> > > > Hi Dumitru, >> > >> > > >> > >> > > Hi Han, >> > >> > > >> > >> > > Thanks for reviewing this. >> > >> > > >> > >> > > > >> > >> > > > Sorry for my slow response, and thanks a lot for revising the >> patch for a bigger scope of validations. However, the exception of /32 >> address makes me thinking more about this patch. If ARP replies is allowed >> from any nexthop for a LR port with /32, at least ARP request for GARP >> purpose should also be allowed. But before asking for a v3, I'd hold on to >> rethink the purpose of this patch. >> > >> > > >> > >> > > Right, we should allow GARP requests too. If we decide to go ahead >> > >> > > with this patch I'll add a function in v3 to handle all types of >> ARPs >> > >> > > and call it both for unreachable static route next-hops and >> attached >> > >> > > subnets. >> > >> > > >> > >> > > > >> > >> > > > The nexthop specific flows are now from static routes. What if >> OVN support dynamical routing protocols in the future? Of course we can >> then take those dynamic nexthops into allowed peers. But then I am thinking >> what is the real benefit of all these restrictions? Why can't we just have >> simpler logic to handle all these situations without validation? I think >> the major benefit of the validation is to avoid handling the noise ARP/NDs >> when multiple subnets shares same L2, but most cases it is really not a big >> deal, right? For the CPU problem caused by ARP flooding as mentioned by the >> bug report, it is a real problem, but this patch seems not really helpful >> for that, because the attacker can just trigger the same CPU problem with >> *valid* packets. So I am not sure if the benefit of the change is worth the >> complexity it introduced. Please share your thought and correct me if I >> missed something. >> > >> > > > >> > >> > > > Thanks, >> > >> > > > Han >> > >> > > >> > >> > > I assume the simpler logic to handle all these situations without >> > >> > > validation is to add rate limiting for ARP packets that get punted >> to >> > >> > > the controller. I agree that this should be implemented too. >> > >> > > >> > >> > > But I think rate limiting all ARP packets regardless of IP >> addresses >> > >> > > is not enough. In the following scenario and if we would have a >> way to >> > >> > > rate limit ARP packets: >> > >> > > - Subnet 42.42.42.0/24 configured on the OVN >> > >> > > - "Invalid" ARP packets are injected at high rate in the network >> for 41.41.41.41 >> > >> > > - Host 42.42.42.42 sends GARP. >> > >> > > - Rate limiting of ARP packets towards controller at 100pps >> > >> > > >> > >> > > With the current code, ARP packets for 41.41.41.41 will be punted >> to >> > >> > > controller at a rate of at most 100 per second. But the chances >> that >> > >> > > the valid 42.42.42.42 GARP is dropped is really high. >> > >> > > >> > >> > > If we instead use the following approach: >> > >> > > a. Punt only useful ARPs to controller. >> > >> > > b. Rate limit ARPs that are sent to controller. >> > >> > > >> > >> > > Then ARP packets outside 42.42.42./24 are never punted to the >> > >> > > controller and don't consume any rate limiting tokens. For the >> second >> > >> > > case, when an attacker would flood with valid ARP packets we would >> > >> > > have the rate limit in place to protect the controller CPU. >> > >> > > >> > >> > > My commit addresses point "a" above as I think point "b" should be >> > >> > > done in a generic way for all protocol packets that need to reach >> the >> > >> > > controller. >> > >> > > >> > >> > > For dynamic routing protocols on the other hand I think we should >> not >> > >> > > install routes with next-hops that are unreachable through other >> > >> > > direct or indirect routes. >> > >> > > >> > >> > > Thanks, >> > >> > > Dumitru >> > >> > >> > >> > I agree that blindly ARP rate limit is not helpful, but a) is not >> really helpful in this case either. In your example, the attacker can just >> use any valid IP in 42.42.42.0/24 to send GARP flooding, which would result >> in exactly same result that a useful GARP from 42.42.42.42 is dropped >> because of blindly rate limiting all ARPs. To solve the problem properly, >> the ARP rate limiting must be done per IP. >> > >> >> > >> Ok, ideally ARP rate limiting should be done per IP but it would take >> > >> quite a lot of resources to keep that information per host. >> > >> >> > >> Any idea how to implement that in an efficient way? There are >> > >> scenarios when we don't know beforehand the IPs of the hosts running >> > >> in the network so that we can whitelist them. Also, from what I've >> > >> seen physical routers usually have a single queue for control plane >> > >> protection for ARPs. >> > >> >> > >> Thanks, >> > >> Dumitru >> > > >> > > >> > > Yes, I agree that system resource is a challenge for per IP rate >> limiting. And yes there is no way to whitelist (because otherwise ARP is >> not needed). We may make some trade-off between the accuracy and >> efficiency. For example, we can have separate meter groups for each logical >> router port for ARP ratelimiting. Each meter group may have e.g. 64 meters, >> and then having a stage to do hash for the IPs, and in the next stage using >> the hash result (between 0 - 63) as index to use corresponding meter to do >> ratelimiting. This way, flooding to a specific IP will impact only the >> other IPs that fall into the same hash bucket on the same router port. But >> this is just a rough idea and I believe many more details still need to be >> figured out for the hashing part. As a first step maybe we can just do >> ratelimiting per router port. It is definitely better than nothing. What do >> you think? >> > >> > Hi Han, >> > >> > It sounds like a good idea to have a hashtable for rate limiting >> > groups of ARPs. I will start like you suggested with a single bucket >> > per router port. The question is if we still want to punt only ARPs >> > for configured networks and nexthops like this patch is trying to do. >> > I would add it and then implement rate limiting as a follow up >> > patch/series. >> > >> > Thanks, >> > Dumitru >> >> That's great. Thanks for working on ratelimiting. Regarding the current >> patch, it seems no obvious benefit, while it increases complexity, so I'd >> rather pause it for now, unless we see real problems of not doing it. > > > Hi Han and Dumitru, > > I didn't follow up on the discussions happening here until now. > I have submitted a patch here - https://patchwork.ozlabs.org/patch/1161273/ > which should solve the problem which Dumitru's patch is trying to handle. > > I started working on this patch to resolve the high cpu usage issue which we > noticed recently for GARP reply packets from the physical switch. > > Just to brief, in the proposed patch, a new action is added - lookup_arp/lookup_nd > which is very similar to the present actions - get_arp/get_nd. > lookup_arp(inport, ip, mac) sets eth.dst to the 'mac', if (ip,mac) is found in the mac_binding table > else it sets eth.dst to 00:00:00:00:00. > > If eth.dst is zero, it means we need to learn the mac and put_arp/put_nd is called. > Otherwise put_arp/put_nd is not called. > > Request to take a look into that patch. > > Thanks > Numan Hi Numan, I tried out your patch and it looks good to me. It does fix the specific problem I was initially trying to fix and covers some of the attack vectors. There's still one thing that needs to be addressed but that's (I think) out side of the scope of your change: we can still hog the CPU in ovn-controller if we're flooded with valid packets that change mac-addresses. But for such attacks we can't do anything right now except rate limiting. Thanks, Dumitru
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index bed2993..637e82c 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -5815,10 +5815,32 @@ build_static_route_flow(struct hmap *lflows, struct ovn_datapath *od, if (is_ipv4) { if (out_port->lrp_networks.n_ipv4_addrs) { lrp_addr_s = out_port->lrp_networks.ipv4_addrs[0].addr_s; + + /* Explicitly allow ARP replies for the next-hop. */ + struct ds match; + ds_init(&match); + ds_put_format(&match, "inport == %s && arp.op == 2 && " + "arp.spa == %s", out_port->json_key, + route->nexthop); + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, + ds_cstr(&match), + "put_arp(inport, arp.spa, arp.sha);"); + ds_destroy(&match); } } else { if (out_port->lrp_networks.n_ipv6_addrs) { lrp_addr_s = out_port->lrp_networks.ipv6_addrs[0].addr_s; + + /* Explicitly allow NA for the next-hop. */ + struct ds match; + ds_init(&match); + ds_put_format(&match, "inport == %s && nd_na && " + "ip6.src == %s", out_port->json_key, + route->nexthop); + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, + ds_cstr(&match), + "put_nd(inport, nd.target, nd.tll);"); + ds_destroy(&match); } } } @@ -6159,10 +6181,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, "ip4.dst == 0.0.0.0/8", "drop;"); - /* ARP reply handling. Use ARP replies to populate the logical - * router's ARP table. */ - ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, "arp.op == 2", - "put_arp(inport, arp.spa, arp.sha);"); /* Drop Ethernet local broadcast. By definition this traffic should * not be forwarded.*/ @@ -6175,16 +6193,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 30, ds_cstr(&match), "drop;"); - /* ND advertisement handling. Use advertisements to populate - * the logical router's ARP/ND table. */ - ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 90, "nd_na", - "put_nd(inport, nd.target, nd.tll);"); - /* Lean from neighbor solicitations that were not directed at - * us. (A priority-90 flow will respond to requests to us and - * learn the sender's mac address. */ - ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_INPUT, 80, "nd_ns", - "put_nd(inport, ip6.src, nd.sll);"); /* Pass other traffic not already handled to the next table for * routing. */ @@ -6320,15 +6329,34 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ds_cstr(&match), ds_cstr(&actions)); } + /* ARP reply handling. Use ARP replies to populate the logical + * router's ARP table. */ + for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { + ds_clear(&match); + ds_put_format(&match, "inport == %s && arp.spa == %s/%u && " + "arp.tpa == %s/%u && arp.op == 2", + op->json_key, + op->lrp_networks.ipv4_addrs[i].network_s, + op->lrp_networks.ipv4_addrs[i].plen, + op->lrp_networks.ipv4_addrs[i].network_s, + op->lrp_networks.ipv4_addrs[i].plen); + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90, + ds_cstr(&match), + "put_arp(inport, arp.spa, arp.sha);"); + } + /* Learn from ARP requests that were not directed at us. A typical * use case is GARP request handling. (A priority-90 flow will * respond to request to us and learn the sender's mac address.) */ for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { ds_clear(&match); ds_put_format(&match, - "inport == %s && arp.spa == %s/%u && arp.op == 1", + "inport == %s && arp.spa == %s/%u && " + "arp.tpa == %s/%u && arp.op == 1", op->json_key, op->lrp_networks.ipv4_addrs[i].network_s, + op->lrp_networks.ipv4_addrs[i].plen, + op->lrp_networks.ipv4_addrs[i].network_s, op->lrp_networks.ipv4_addrs[i].plen); if (op->od->l3dgw_port && op == op->od->l3dgw_port && op->od->l3redirect_port) { @@ -6669,6 +6697,45 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, ds_cstr(&match), ds_cstr(&actions)); } + /* NA reply handling. Use NA replies to populate the logical + * router's neighbor table. */ + for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { + ds_clear(&match); + ds_put_format(&match, "inport == %s && nd_na && " + "nd.target == %s/%u && ip6.src == %s/%u", + op->json_key, + op->lrp_networks.ipv6_addrs[i].network_s, + op->lrp_networks.ipv6_addrs[i].plen, + op->lrp_networks.ipv6_addrs[i].network_s, + op->lrp_networks.ipv6_addrs[i].plen); + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90, + ds_cstr(&match), + "put_nd(inport, nd.target, nd.tll);"); + } + + /* Learn from ND requests that were not directed at us. + * (A priority-90 flow will respond to request to us and learn the + * sender's mac address.) */ + for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { + ds_clear(&match); + ds_put_format(&match, + "inport == %s && nd_ns && ip6.src == %s/%u && " + "ip6.dst == %s/%u", + op->json_key, + op->lrp_networks.ipv6_addrs[i].network_s, + op->lrp_networks.ipv6_addrs[i].plen, + op->lrp_networks.ipv6_addrs[i].network_s, + op->lrp_networks.ipv6_addrs[i].plen); + if (op->od->l3dgw_port && op == op->od->l3dgw_port + && op->od->l3redirect_port) { + ds_put_format(&match, " && is_chassis_resident(%s)", + op->od->l3redirect_port->json_key); + } + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80, + ds_cstr(&match), + "put_nd(inport, ip6.src, nd.sll);"); + } + /* UDP/TCP port unreachable */ if (!smap_get(&op->od->nbr->options, "chassis") && !op->od->l3dgw_port) {
Add a restriction on the target protocol addresses to match the configured subnets. All other ARP/ND packets are invalid in this context. One exception is for ARP replies that are received for route next-hops that are only reachable via a port but can't be directly resolved through route lookups. Such support was introduced by commit: 6b785fd8fe29 ("ovn-util: Allow /32 IP addresses for router ports.") Reported-at: https://bugzilla.redhat.com/1729846 Reported-by: Haidong Li <haili@redhat.com> CC: Han Zhou <zhouhan@gmail.com> CC: Guru Shetty <guru@ovn.org> Fixes: b068454082f5 ("ovn-northd: Support learning neighbor from ARP request.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- v2: - Update commit message. - Implement the fix also for ARP replies and IPv6 ND. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.c | 95 +++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 81 insertions(+), 14 deletions(-)