[PATCH v2 0/2] Delete Net Routes for Deleted Interfaces

[PATCH v2 0/2] Delete Net Routes for Deleted Interfaces

[Andrew Hamilton]

Correct incorrect handling of routes being maintained when an
associated interface is deleted. Previously the route(s) for an interface
being removed were not deleted. This resulted in displaying corrupted
output to the console in the following sequence:
net_add_addr
if0 emu0 192.168.100.2
net_ls_routes if0:local 192.168.100.0/24 if0
net_del_addr if0
net_ls_routes if0:local 192.168.100.0/24 ???
...
net_ls_routes
if0:local 192.168.100.0/24 ?7?
The fields including the question marks above will contain pseudo-random
data from the heap which may change over time. In some cases this may
have resulted in crashes as well after a route was deleted and attempted
to be used in actual network routing operations.
With this update, routes mapped to a deleted interface will be deleted.

In addition, a test was added for testing the network commands. This
test will still run without the net.c change but it will fail.
With the net.c change, this test will pass on grub-emu and be
skipped for other platforms.

Changes since v1:
 (Ref:
https://lists.gnu.org/archive/html/grub-devel/2024-08/msg00138.html)
- Add net_test
- Resubmit net.c change due to incorrect patch submission before.

Andrew Hamilton (2):
  Add a test for the grub-emu environment to test various network
    commands such as adding addresses, updating routes, etc.
  Correct incorrect handling of routes being maintained when an
    associated interface is deleted. Previously the route(s) for an
    interface being removed were not deleted. This resulted in
    displaying corrupted output to the console in the following
    sequence: net_add_addr if0 emu0 192.168.100.2 net_ls_routes
    if0:local 192.168.100.0/24 if0 net_del_addr if0 net_ls_routes
    if0:local 192.168.100.0/24 ??? ... net_ls_routes if0:local
    192.168.100.0/24 ?7? The fields including the question marks above
    will contain pseudo-random data from the heap which may change over
    time. In some cases this may have resulted in crashes as well after
    a route was deleted and attempted to be used in actual network
    routing operations. With this update, routes mapped to a deleted
    interface will be deleted.

 Makefile.util.def   |   6 ++
 grub-core/net/net.c |  17 ++++
 tests/net_test.in   | 187 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 210 insertions(+)
 create mode 100644 tests/net_test.in

-- 
2.39.2


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 1/2] tests: Add net_test for network commands

[Andrew Hamilton]

diff --git a/Makefile.util.def b/Makefile.util.def
index 0f74a1680..9ed211bc5 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -1131,6 +1131,12 @@ script = {
   common = tests/cdboot_test.in;
 };
 
+script = {
+  testcase = nonnative;
+  name = net_test;
+  common = tests/net_test.in;
+};
+
 script = {
   testcase = nonnative;
   name = netboot_test;
diff --git a/tests/net_test.in b/tests/net_test.in
new file mode 100644
index 000000000..de5d8a821
--- /dev/null
+++ b/tests/net_test.in
@@ -0,0 +1,187 @@
+#! @BUILD_SHEBANG@
+# This test is intended to perform basic tests of the "net" commands
+# by providing command line inputs and checking command console output.
+
+set -e
+
+. "@builddir@/grub-core/modinfo.sh"
+
+set -e
+
+if [ "$EUID" = "" ] ; then
+  EUID=$(id -u)
+fi
+
+# Define some variables to be used if different platforms have different
+# interfaces / etc.
+net_dev=""
+net_dev_mac=""
+net_addr=""
+net_addr2=""
+net_addr_ip6=""
+net_addr_ip6_exp=""
+net_route_subnet6=""
+net_gw_subnet=""
+net_gw_addr=""
+net_route_subnet=""
+echo "${grub_modinfo_target_cpu}-${grub_modinfo_platfomr}"
+case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
+    # PLATFORM: emu works when run as root to create the tap device
+    *-emu)
+        if [ "$EUID" != 0 ] ; then
+           echo "not root; cannot test net_test"
+           exit 99
+        fi
+	net_dev="emu0"
+	net_dev_mac="00:01:02:03:04:05"
+	net_addr="169.254.45.2"
+	net_addr2="169.254.46.2"
+	net_addr_ip6="fe80::bca3:3bdf:ba9B:ba9a"
+	net_addr_ip6_exp="fe80:0:0:0:bca3:3bdf:ba9b:ba9a"
+	net_route_subnet6="fe80:0:0:0:0:0:0:0/32"
+	net_gw_subnet="169.254.60.1/24"
+	net_gw_addr="169.254.45.1"
+	net_route_subnet="169.254.0.0/16"
+        ;;
+    # Others are untested (wasn't able to figure out how to get a
+    # network card in Qemu with Grub)
+    # FIXME:
+    *)
+        exit 77;;
+esac
+
+# TC1: The default state should be one card exists
+expected="$net_dev $net_dev_mac"
+output="$(echo 'net_ls_cards' | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC2: The default state should be no address defined
+expected=""
+output="$(echo 'net_ls_addr' | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC3: The default state should be no DNS defined
+expected=""
+output="$(echo 'net_ls_dns' | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC4: The default state should be no routes defined
+expected=""
+output="$(echo 'net_ls_routes' | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC5: Add an addr to the card
+expected="if1 $net_dev_mac $net_addr 
+if1:local $net_route_subnet if1"
+command="net_add_addr if1 $net_dev $net_addr; net_ls_addr; net_ls_routes"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC6: Test removing the addr and associated route update
+expected=""
+command="net_add_addr if1 $net_dev $net_addr; net_del_addr if1; net_ls_routes; \
+net_ls_addr"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC7: Test adding two interfaces then removing one
+expected="if:2 $net_dev_mac 169.254.46.2 
+if:2:local $net_route_subnet if:2"
+command="net_add_addr if:1 $net_dev $net_addr; net_add_addr if:2 $net_dev \
+$net_addr2 ; net_del_addr if:1; net_ls_addr; net_ls_routes"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC8: Test adding a route with a gateway
+expected="INTERFACE_1 00:01:02:03:04:05 169.254.45.2 
+IFACE1_ROUTE $net_gw_subnet gw $net_gw_addr
+INTERFACE_1:local $net_route_subnet INTERFACE_1"
+command="net_add_addr INTERFACE_1 $net_dev $net_addr; net_add_route \
+IFACE1_ROUTE $net_gw_subnet gw $net_gw_addr; net_ls_addr; net_ls_routes"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC9: Test timeout case for IP6 autoconf
+expected="error: couldn't send network packet.
+error: couldn't send network packet.
+error: couldn't send network packet.
+error: couldn't send network packet.
+error: couldn't send network packet.
+error: couldn't send network packet.
+error: couldn't autoconfigure emu0."
+command="net_ipv6_autoconf"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC10: Test setting a VLAN
+expected="if1 $net_dev_mac $net_addr vlan1"
+command="net_add_addr if1 $net_dev $net_addr; net_set_vlan if1 1; \
+net_ls_addr"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC11: Test setting another VLAN
+expected="if1 $net_dev_mac $net_addr vlan4"
+command="net_add_addr if1 $net_dev $net_addr; net_set_vlan if1 4; \
+net_ls_addr"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC12: Test setting up an IPv6 address
+#  I'm not sure if manually adding a route for the link local should be
+#  required...
+expected="if1 $net_dev_mac $net_addr_ip6_exp 
+if1:local $net_route_subnet6 if1"
+command="net_add_addr if1 $net_dev $net_addr_ip6; net_add_route \
+if1:local $net_route_subnet6 if1; net_ls_addr; net_ls_routes"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
+
+# TC13: Test adding IPV6 address, route, then removing the interface
+#  I'm not sure if manually adding a route for the link local should be
+#  required...
+expected=""
+command="net_add_addr if1 $net_dev $net_addr_ip6; net_add_route \
+if1:local $net_route_subnet6 if1;  net_del_addr if1; net_ls_routes; \
+net_ls_addr"
+output="$(echo "$command" | @builddir@/grub-shell)"
+if [ "$output" != "$expected" ]; then
+  printf "Line: $LINENO, Actual: >%s<\nExpected: >%s<\n" "$output" "$expected"
+  exit 1;
+fi
-- 
2.39.2


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 2/2] net/net: Delete Routes for Deleted Interfaces

[Andrew Hamilton]

Signed-off-by: Andrew Hamilton <adhamilt@gmail.com>
---
 grub-core/net/net.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 8cad4fb6d..51e0dd312 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -718,6 +718,23 @@ grub_cmd_deladdr (struct grub_command *cmd __attribute__ ((unused)),
     return grub_error (GRUB_ERR_IO,
 		       N_("you can't delete this address"));
 
+  struct grub_net_route *route;
+  struct grub_net_route **prev;
+
+  /* Remove any existing routes using this interface. */
+  for (prev = &grub_net_routes, route = *prev; route;
+       prev = &((*prev)->next), route = *prev)
+    {
+      if (grub_strcmp (route->interface->name, inter->name) == 0)
+        {
+          *prev = route->next;
+          grub_free (route->name);
+          grub_free (route);
+          if (*prev == NULL)
+            break;
+        }
+    }
+
   grub_net_network_level_interface_unregister (inter);
   grub_free (inter->name);
   grub_free (inter);
-- 
2.39.2


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 0/2] Delete Net Routes for Deleted Interfaces

[Andrew Hamilton]

[-- Attachment #1.1: Type: text/plain, Size: 2762 bytes --]

Any feedback on this? I can regenerate and resend the patch if there are
any conflicts with other updates.

Thanks,
Andrew

On Sat, Aug 31, 2024 at 10:31 PM Andrew Hamilton <adhamilt@gmail.com> wrote:

> Correct incorrect handling of routes being maintained when an
> associated interface is deleted. Previously the route(s) for an interface
> being removed were not deleted. This resulted in displaying corrupted
> output to the console in the following sequence:
> net_add_addr
> if0 emu0 192.168.100.2
> net_ls_routes if0:local 192.168.100.0/24 if0
> net_del_addr if0
> net_ls_routes if0:local 192.168.100.0/24 ???
> ...
> net_ls_routes
> if0:local 192.168.100.0/24 ?7?
> The fields including the question marks above will contain pseudo-random
> data from the heap which may change over time. In some cases this may
> have resulted in crashes as well after a route was deleted and attempted
> to be used in actual network routing operations.
> With this update, routes mapped to a deleted interface will be deleted.
>
> In addition, a test was added for testing the network commands. This
> test will still run without the net.c change but it will fail.
> With the net.c change, this test will pass on grub-emu and be
> skipped for other platforms.
>
> Changes since v1:
>  (Ref:
> https://lists.gnu.org/archive/html/grub-devel/2024-08/msg00138.html)
> - Add net_test
> - Resubmit net.c change due to incorrect patch submission before.
>
> Andrew Hamilton (2):
>   Add a test for the grub-emu environment to test various network
>     commands such as adding addresses, updating routes, etc.
>   Correct incorrect handling of routes being maintained when an
>     associated interface is deleted. Previously the route(s) for an
>     interface being removed were not deleted. This resulted in
>     displaying corrupted output to the console in the following
>     sequence: net_add_addr if0 emu0 192.168.100.2 net_ls_routes
>     if0:local 192.168.100.0/24 if0 net_del_addr if0 net_ls_routes
>     if0:local 192.168.100.0/24 ??? ... net_ls_routes if0:local
>     192.168.100.0/24 ?7? The fields including the question marks above
>     will contain pseudo-random data from the heap which may change over
>     time. In some cases this may have resulted in crashes as well after
>     a route was deleted and attempted to be used in actual network
>     routing operations. With this update, routes mapped to a deleted
>     interface will be deleted.
>
>  Makefile.util.def   |   6 ++
>  grub-core/net/net.c |  17 ++++
>  tests/net_test.in   | 187 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 210 insertions(+)
>  create mode 100644 tests/net_test.in
>
> --
> 2.39.2
>
>

[-- Attachment #1.2: Type: text/html, Size: 3920 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel