Re: [diff] deleting lo(4) with rdomain
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 - 1.556
> +++ if.c 23 Jun 2018 19:41:05 -
> @@ -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_, , 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)
Re: [diff] deleting lo(4) with rdomain
On Sat, Jun 23, 2018 at 10:52:13PM +0200, Denis Fondras wrote:
> Here is a diff to allow deletion of lo(4) created by rdomain.
I think this is a good idea. Interface and route configuration in
the kernel should be revertabel without reboot.
> rtable is still available after all the interfaces are out of the
> rdomain though.
But then we have rtable without loopback again. I think removing
the final lo interface from the rdomain should also remove the
rtable.
Can we do that?
> +struct ifnet *
> +rdomain_isused(struct ifnet *ifp, int rdomain)
> +{
> + struct ifnet *ifp_;
Do we have local variable names ending with _ anywhere else? In
general the name conflict should be avoided by making the parameter
name more specific.
In this case rdomain_isused() should not be called with any interface.
The rdomain is enough if you ignore its loopback interface.
> + TAILQ_FOREACH(ifp_, , if_list) {
> + if (ifp_ == ifp)
This should be if (ifp->if_index == rtable_loindex(rdomain))
> + continue;
> + if (ifp_->if_rdomain == rdomain)
> + return (ifp_);
> + }
> + return (NULL);
> +}
And a ..._isused() function should be boolean, return 1 or 0.
> 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;
The variable unit is not needed, just use rdomain.
> 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);
I think EBUSY would be better now. It is not a permission problem
anymore.
bluhm
[diff] deleting lo(4) with rdomain
Here is a diff to allow deletion of lo(4) created by rdomain.
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.
[denis@visigoth:~] doas ifconfig em0 rdomain 3
[denis@visigoth:~] ifconfig
lo0: flags=8049 mtu 32768
index 4 priority 0 llprio 3
groups: lo
inet6 ::1 prefixlen 128
inet6 fe80::1%lo0 prefixlen 64 scopeid 0x4
inet 127.0.0.1 netmask 0xff00
em0: flags=8843 rdomain 3 mtu 1500
lladdr 8c:16:45:57:e4:91
index 2 priority 0 llprio 3
media: Ethernet autoselect (none)
status: no carrier
lo3: flags=8008 rdomain 3 mtu 32768
index 7 priority 0 llprio 3
groups: lo
[denis@visigoth:~] doas ifconfig lo3 -rdomain
ifconfig: SIOCSIFRDOMAIN: Operation not permitted
[denis@visigoth:~] doas ifconfig em0 -rdomain
[denis@visigoth:~] doas ifconfig lo3 -rdomain
[denis@visigoth:~] ifconfig
lo0: flags=8049 mtu 32768
index 4 priority 0 llprio 3
groups: lo
inet6 ::1 prefixlen 128
inet6 fe80::1%lo0 prefixlen 64 scopeid 0x4
inet 127.0.0.1 netmask 0xff00
em0: flags=8843 mtu 1500
lladdr 8c:16:45:57:e4:91
index 2 priority 0 llprio 3
media: Ethernet autoselect (none)
status: no carrier
lo3: flags=8008 mtu 32768
index 7 priority 0 llprio 3
groups: lo
[denis@visigoth:~] doas ifconfig lo3 destroy
[denis@visigoth:~] ifconfig
lo0: flags=8049 mtu 32768
index 4 priority 0 llprio 3
groups: lo
inet6 ::1 prefixlen 128
inet6 fe80::1%lo0 prefixlen 64 scopeid 0x4
inet 127.0.0.1 netmask 0xff00
em0: flags=8843 mtu 1500
lladdr 8c:16:45:57:e4:91
index 2 priority 0 llprio 3
media: Ethernet autoselect (none)
status: no carrier
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).
Index: if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.556
diff -u -p -r1.556 if.c
--- if.c21 Jun 2018 07:40:43 - 1.556
+++ if.c23 Jun 2018 19:41:05 -
@@ -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_;
+
+ TAILQ_FOREACH(ifp_, , 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);
- 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);
- 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 ==
