Hi,
I have attached a patch to check if there are any vrf-base bfd
protocols. And if there are none of them, then protocol non-vrf bfd
protocols takes all sessions. Otherwise it takes only non-vrf
sessions. I added a global option into config structure, but not sure
if it is an approved way of doing things like that.
It still leaves open a case when one have vrfs but want bfd only in
the default and some protocols in vrf have bfd enabled, so it will
send packets trying to establish them. But I think it is a rare and
weird case to enable bfd sessions in protocols, when they are not
supposed to work anyway.
On Wed, Jul 17, 2019 at 6:21 PM Alexander Zubkov <[email protected]> wrote:
>
> I have also prepared some changes to the documentation. But it
> probably should wait unitl the questions with VRF and non VRF are
> finalized. For example in the current state, that catch-all behaviour
> better to be described too.
>
> On Wed, Jul 17, 2019 at 6:03 PM Alexander Zubkov <[email protected]> wrote:
> >
> > On Wed, Jul 17, 2019 at 4:46 PM Ondrej Zajicek <[email protected]>
> > wrote:
> > >
> > > On Wed, Jul 17, 2019 at 03:08:45PM +0200, Alexander Zubkov wrote:
> > > > On Wed, Jul 17, 2019 at 2:47 PM Ondrej Zajicek <[email protected]>
> > > > wrote:
> > > > > Hello
> > > > >
> > > > > This would work, it is necessary to also set sk->vrf for
> > > > > bfd_open_tx_sk()
> > > > > foir multihop BFD. It is not necessary to handle it in
> > > > > bfd_reconfigure(),
> > > > > as VRF change is handled in generic code in proto_reconfigure().
> > > > >
> > > > > I also just implemented BFD request dispatch based on VRFs to handle
> > > > > multiple
> > > > > VRFs and multiple BFD instances.
> > > >
> > > > Oh! I was just trying to figure out it myself today too. See the
> > > > attached patch. :)
> > >
> > > Your patch is correct and mostly the same as mine [*]. There are just
> > > two differences:
> >
> > I'm glad to hear that I have figured out it correctly. :)
> >
> > >
> > > - There was check in BFD code that forbade multiple BFD instances, that
> > > has to be removed.
> >
> > Yes. I was thinking if I could replace it with some check that only
> > one bfd per vrf exists. But delete way is easier here. :)
> >
> > >
> > > - I allowed request in VRF to be handled by BFD protocol in default/NULL
> > > VRF.
> > > That would make it compatible with one BFD and
> > > net.ipv4.udp_l3mdev_accept=1.
> >
> > I see. But in that case if one had a mixed "environment" with vrfs and
> > non-vrf protocols, than the default bfd would be able to catch other
> > vrf's sessions first. May be it would make sence to introduce some
> > check that config has non-default vrf options or some configuration
> > option for bfd protocol.
> >
> > >
> > > Note that it was true that wildcard socket bind in default VRF blocked
> > > bind in other VRFs, so it would not be possible to run BFD in both
> > > default VRF and regular VRFs, but that was fixed in Linux kernel 5.0,
> > > so perhaps it would make sense to change it that BFD in default VRF
> > > only accept requests from default BFD, like in your patch.
> > >
> > >
> > > [*]
> > > https://gitlab.labs.nic.cz/labs/bird/commit/cf7ff99513728e4e405508e5ccd7522289d4ec82
> > >
> > > --
> > > Elen sila lumenn' omentielvo
> > >
> > > Ondrej 'Santiago' Zajicek (email: [email protected])
> > > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
> > > "To err is human -- to blame it on a computer is even more so."
--- a/conf/conf.c
+++ b/conf/conf.c
@@ -106,6 +106,7 @@ config_alloc(const char *name)
c->tf_route = c->tf_proto = TM_ISO_SHORT_MS;
c->tf_base = c->tf_log = TM_ISO_LONG_MS;
c->gr_wait = DEFAULT_GR_WAIT;
+ c->bfd_vrf = 0;
return c;
}
--- a/conf/conf.h
+++ b/conf/conf.h
@@ -38,6 +38,7 @@ struct config {
struct timeformat tf_log; /* Time format for the logfile */
struct timeformat tf_base; /* Time format for other purposes */
u32 gr_wait; /* Graceful restart wait timeout (sec) */
+ int bfd_vrf; /* There are BFD protocols in VRF */
int cli_debug; /* Tracing of CLI connections and commands */
int latency_debug; /* I/O loop tracks duration of each event */
--- a/proto/bfd/bfd.c
+++ b/proto/bfd/bfd.c
@@ -624,7 +624,10 @@ bfd_request_notify(struct bfd_request *req, u8 state, u8 diag)
static int
bfd_add_request(struct bfd_proto *p, struct bfd_request *req)
{
- if (p->p.vrf && (p->p.vrf != req->vrf))
+ struct config *gc = p->p.cf->global;
+
+ /* Compare VRFs if protocol VRF is defined or there any BFDs in VRF */
+ if ((p->p.vrf || gc->bfd_vrf) && (p->p.vrf != req->vrf))
return 0;
struct bfd_session *s = bfd_find_session_by_addr(p, req->addr);
@@ -1033,10 +1036,14 @@ static int
bfd_reconfigure(struct proto *P, struct proto_config *c)
{
struct bfd_proto *p = (struct bfd_proto *) P;
- // struct bfd_config *old = (struct bfd_config *) (P->cf);
+ struct bfd_config *old = (struct bfd_config *) (P->cf);
struct bfd_config *new = (struct bfd_config *) c;
struct bfd_iface *ifa;
+ /* Restart protocols if default catch all status changed */
+ if (old->c.global->bfd_vrf != new->c.global->bfd_vrf)
+ return 0;
+
birdloop_mask_wakeups(p->loop);
WALK_LIST(ifa, p->iface_list)
@@ -1056,6 +1063,14 @@ bfd_reconfigure(struct proto *P, struct proto_config *c)
return 1;
}
+/* Remember if there are BFD protocols in VRF */
+static void
+bfd_postconfig(struct proto_config *c)
+{
+ if (c->vrf)
+ c->global->bfd_vrf = 1;
+}
+
static void
bfd_copy_config(struct proto_config *dest, struct proto_config *src UNUSED)
{
@@ -1119,5 +1134,6 @@ struct protocol proto_bfd = {
.start = bfd_start,
.shutdown = bfd_shutdown,
.reconfigure = bfd_reconfigure,
+ .postconfig = bfd_postconfig,
.copy_config = bfd_copy_config,
};
