On 23/06/18(Sat) 22:52, Denis Fondras wrote:
> Here is a diff to allow deletion of lo(4) created by rdomain.
This is very sensible area. I'd suggest writing some regression tests.
> Of course, lo(4) cannot be deleted while the rdomain is used on another
> interface. rtable is still available after all the interfaces are out of the
> rdomain though.
And it needs to be created automatically as soon as the rdomain is used
again. See netinet/ip_output.c:215 for the reason 8)
> Because the kernel panics when manipulating rdomain on enc(4), I added a check
> to disallow that (the kernel panic is not linked to this change, it
> preexisted).
That's the wrong way to fix the problem. What is the panic? How can it
be fixed? Such layers of over engineering tend to create more complexity
than they solve problems over time.
Comments below.
> Index: if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.556
> diff -u -p -r1.556 if.c
> --- if.c 21 Jun 2018 07:40:43 -0000 1.556
> +++ if.c 23 Jun 2018 19:41:05 -0000
> @@ -1729,43 +1729,53 @@ if_setlladdr(struct ifnet *ifp, const ui
> return (0);
> }
>
> +struct ifnet *
> +rdomain_isused(struct ifnet *ifp, int rdomain)
> +{
> + struct ifnet *ifp_;
We generally name the variables `ifp0' and `ifp'. You won't see
underscores in the kernel 8)
> +
> + TAILQ_FOREACH(ifp_, &ifnet, if_list) {
> + if (ifp_ == ifp)
> + continue;
> + if (ifp_->if_rdomain == rdomain)
> + return (ifp_);
> + }
> + return (NULL);
> +}
> +
> int
> if_setrdomain(struct ifnet *ifp, int rdomain)
> {
> struct ifreq ifr;
> int error, up = 0, s;
> + struct ifnet *loifp;
> + char loifname[IFNAMSIZ];
> + unsigned int unit = rdomain;
>
> if (rdomain < 0 || rdomain > RT_TABLEID_MAX)
> return (EINVAL);
>
> + if (strncmp(ifp->if_xname, "enc", 3) == 0)
> + return (EPERM);
> +
> /*
> * Create the routing table if it does not exist, including its
> * loopback interface with unit == rdomain.
> */
> - if (!rtable_exists(rdomain)) {
> - struct ifnet *loifp;
> - char loifname[IFNAMSIZ];
> - unsigned int unit = rdomain;
> -
> - snprintf(loifname, sizeof(loifname), "lo%u", unit);
> - error = if_clone_create(loifname, 0);
> -
> - if ((loifp = ifunit(loifname)) == NULL)
> - return (ENXIO);
> -
> - /* Do not error out if creating the default lo(4) interface */
> - if (error && (ifp != loifp || error != EEXIST))
> + if (!rtable_exists(rdomain))
> + if ((error = rtable_add(rdomain)))
> return (error);
The order of operation matters. If something bad happens afterward
you'll have a rdomain without default lo(4)... Is that ok?
>
> - if ((error = rtable_add(rdomain)) == 0)
> - rtable_l2set(rdomain, rdomain, loifp->if_index);
> - if (error) {
> - if_clone_destroy(loifname);
> - return (error);
> - }
> + snprintf(loifname, sizeof(loifname), "lo%u", unit);
> + error = if_clone_create(loifname, 0);
> + if (error && error != EEXIST)
> + return (error);
What if I create "lo33 rdomain 3"? I'll now end up with two lo(4) for
that rdomain. You want to check rtable_loindex() for this rdomain.
> - loifp->if_rdomain = rdomain;
> - }
> + if ((loifp = ifunit(loifname)) == NULL)
> + return (ENXIO);
> +
> + rtable_l2set(rdomain, rdomain, loifp->if_index);
> + loifp->if_rdomain = rdomain;
>
> /* make sure that the routing table is a real rdomain */
> if (rdomain != rtable_l2(rdomain))
> @@ -1773,7 +1783,8 @@ if_setrdomain(struct ifnet *ifp, int rdo
>
> if (rdomain != ifp->if_rdomain) {
> if ((ifp->if_flags & IFF_LOOPBACK) &&
> - (ifp->if_index == rtable_loindex(ifp->if_rdomain)))
> + (ifp->if_index == rtable_loindex(ifp->if_rdomain)) &&
> + (rdomain_isused(ifp, ifp->if_rdomain)))
> return (EPERM);
Can you use the same check in loop_clone_destroy()? I'd move everything
in a function and add a comment explaining when a lo(4) interface can be
destroyed 8)
