Bug 231441 - 12.0-ALPHA6 network does not start at boot
Summary: 12.0-ALPHA6 network does not start at boot
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Warner Losh
URL: https://reviews.freebsd.org/D17267
Keywords: regression
Depends on:
Blocks:
 
Reported: 2018-09-18 03:45 UTC by Patrick McMunn
Modified: 2018-10-10 00:59 UTC (History)
7 users (show)

See Also:


Attachments
skip shell_quote() call for regexes (2.65 KB, patch)
2018-09-21 05:08 UTC, Yuri Pankov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick McMunn 2018-09-18 03:45:05 UTC
I was running a fresh installation of 11.2-stable for the past couple of
weeks, and I decided to update from source to 12-current over the weekend.
After upgrading and rebooting, my network was no longer starting at boot.
It wouldn't work until I ran "service netif restart". Thinking perhaps I might have made a mistake in upgrading from source, I downloaded the 12-ALPHA6 snapshot, loaded it onto a USB stick, and did a fresh installaton. The problem with network not starting at boot persists.
Comment 1 Johannes Lundberg 2018-09-19 08:36:33 UTC
Here's some devd output:

https://gist.github.com/johalun/c9383ff03cb29e12c3f4978e06ca6413#file-gistfile1-txt-L761-L762

The highlighted lines show suspicious regexps.
Comment 2 Johannes Lundberg 2018-09-19 08:38:27 UTC
By the way, this affect many people so it's very important.
Comment 3 Sean Bruno freebsd_committer 2018-09-20 15:36:40 UTC
(In reply to Patrick from comment #0)
Can you please attach your rc.conf to this ticket?
Comment 4 Warner Losh freebsd_committer 2018-09-20 16:49:51 UTC
there's a chance this is related to a devd bug someone contacted me about.... it looked similar to Johannes' output...  I'll have to investigate.
Comment 5 Patrick McMunn 2018-09-21 01:10:51 UTC
Someone on the current mailing list was also saying that this is a devd issue. Here's my current rc.conf:

clear_tmp_enable="YES"
syslogd_flags="-ss"
sendmail_enable="NONE"

hostname="xps400"

dbus_enable="YES"
hald_enable="YES"
moused_enable="YES"
ntpdate_enable="YES"
ntpd_enable="YES"
powerd_enable="YES"
sddm_enable="YES"
sshd_enable="YES"
webcamd_enable="YES"

# Set dumpdev to "AUTO" to enable crash dumps, "NO" to disable
dumpdev="AUTO"

wlans_run0="wlan0"
ifconfig_wlan0="DHCP WPA"
Comment 6 amvandemore 2018-09-21 01:29:45 UTC
Try this instead:

ifconfig_wlan0="WPA SYNCDHCP"
Comment 7 Patrick McMunn 2018-09-21 02:19:56 UTC
(In reply to amvandemore from comment #6)

I made the change you recommended, but it made no difference. I still had to manually run "service netif start" to bring up the network after boot.
Comment 8 Yuri Pankov 2018-09-21 02:23:41 UTC
FWIW, I'm not seeing any issues with if_run loaded from /boot/loader.conf, it's correctly brought up on boot with the following (standard) entries in rc.conf:

wlans_run0="wlan0"
ifconfig_wlan0="WPA SYNCDHCP"
Comment 9 Patrick McMunn 2018-09-21 03:12:29 UTC
(In reply to Yuri Pankov from comment #8)

When I boot my system, kldstat shows that if_run.ko is loaded, and dmesg shows:

run0 on uhub2
uhid0 on uhub4
uhid0: <Logitech Logitech Dual Action, class 0/0, rev 1.10/3.00, addr 2> on usbus1
run0: <1.0> on usbus4
run0: MAC/BBP RT3070 (rev 0x0201), RF RT3020 (MIMO 1T1R), address 00:12:0e:ce:c5:04

After manually starting the network, dmesg shows:

wlan0: Ethernet address: 00:12:0e:ce:c5:04
run0: firmware RT2870 ver. 0.33 loaded
wlan0: link state changed to UP

So the driver, but not the firmware, is being loaded automatically at boot. I don't know if that's significant or if the firmware normally isn't loaded until the network is brought up.
Comment 10 Yuri Pankov 2018-09-21 03:47:34 UTC
Apparently, I'm being stupid, and the use case here is NOT to load driver via loader.conf and rather rely on devd to load it and bring up wlan interface.  To make up for my mistake, I've taken a look at the source of problem here.

Looks like devd::expand_one() treats "$(var)" as a subcommand to be passed down to shell -- this works correctly for the items in "action", but not so good for device matching.  In any case, removing parentheses from the wifi-driver-regex seems to work for me loading the if_run.ko, and creating wlan0:

diff --git a/sbin/devd/devd.conf b/sbin/devd/devd.conf
index 956a2970d436..c9deaf5dada9 100644
--- a/sbin/devd/devd.conf
+++ b/sbin/devd/devd.conf
@@ -19,13 +19,13 @@ options {
        # Setup some shorthand for regex that we use later in the file.
        #XXX Yes, these are gross -- imp
        set scsi-controller-regex
-               "(aac|adv|adw|aha|ahc|ahd|aic|amr|bt|ciss|dpt|\
+               "aac|adv|adw|aha|ahc|ahd|aic|amr|bt|ciss|dpt|\
                esp|ida|iir|ips|isp|mlx|mly|mpr|mps|mpt|ncr|ncv|nsp|stg|sym|\
-               trm)\
+               trm\
                [0-9]+";
        set wifi-driver-regex
-               "(ath|bwi|bwn|ipw|iwi|iwm|iwn|malo|mwl|ral|rsu|rtwn|rum|run|\
-               uath|upgt|ural|urtw|wi|wpi|wtap|zyd)[0-9]+";
+               "ath|bwi|bwn|ipw|iwi|iwm|iwn|malo|mwl|ral|rsu|rtwn|rum|run|\
+               uath|upgt|ural|urtw|wi|wpi|wtap|zyd[0-9]+";
 };

 # Note that the attach/detach with the highest value wins, so that one can
Comment 11 Warner Losh freebsd_committer 2018-09-21 04:48:20 UTC
The regex is correct. Changing it is bogus.
I need to fix the bug in devd that interprets it incorrectly since the code to protect expansion of variables is interfering with things like this.
Comment 12 Yuri Pankov 2018-09-21 05:08:19 UTC
Created attachment 197291 [details]
skip shell_quote() call for regexes

Indeed, I'm too fast sharing the incomplete analysis/fix as always.  Does the attached patch skipping the shell_quote() call for regexes make more sense?
Comment 13 Warner Losh freebsd_committer 2018-09-21 05:21:21 UTC
diff --git a/sbin/devd/devd.cc b/sbin/devd/devd.cc
index e81f718159b1..b2d08324511f 100644
--- a/sbin/devd/devd.cc
+++ b/sbin/devd/devd.cc
@@ -666,7 +666,7 @@ config::shell_quote(const string &s)
 }

 void
-config::expand_one(const char *&src, string &dst)
+config::expand_one(const char *&src, string &dst, bool is_shell)
 {
        int count;
        string buffer;
@@ -705,7 +705,7 @@ config::expand_one(const char *&src, string &dst)
        do {
                buffer += *src++;
        } while (is_id_char(*src));
-       dst.append(shell_quote(get_variable(buffer)));
+       dst.append(is_shell ? shell_quote(get_variable(buffer)) : get_variable(buffer));
 }

 const string
@@ -731,7 +731,7 @@ config::expand_string(const char *src, const char *prepend, const char *append)
                }
                dst.append(src, var_at - src);
                src = var_at;
-               expand_one(src, dst);
+               expand_one(src, dst, prepend == NULL);
        }

        if (append != NULL)
diff --git a/sbin/devd/devd.hh b/sbin/devd/devd.hh
index 534c4d4c5a9d..d770da22226a 100644
--- a/sbin/devd/devd.hh
+++ b/sbin/devd/devd.hh
@@ -172,7 +172,7 @@ protected:
        void sort_vector(std::vector<event_proc *> &);
        void parse_one_file(const char *fn);
        void parse_files_in_dir(const char *dirname);
-       void expand_one(const char *&src, std::string &dst);
+       void expand_one(const char *&src, std::string &dst, bool is_shell);
        std::string shell_quote(const std::string &s);
        bool is_id_char(char) const;
        bool chop_var(char *&buffer, char *&lhs, char *&rhs) const;
Comment 14 Warner Losh freebsd_committer 2018-09-21 05:22:22 UTC
I think the following is a shorter change that does the right thing...

diff --git a/sbin/devd/devd.cc b/sbin/devd/devd.cc
index e81f718159b1..b2d08324511f 100644
--- a/sbin/devd/devd.cc
+++ b/sbin/devd/devd.cc
@@ -666,7 +666,7 @@ config::shell_quote(const string &s)
 }

 void
-config::expand_one(const char *&src, string &dst)
+config::expand_one(const char *&src, string &dst, bool is_shell)
 {
        int count;
        string buffer;
@@ -705,7 +705,7 @@ config::expand_one(const char *&src, string &dst)
        do {
                buffer += *src++;
        } while (is_id_char(*src));
-       dst.append(shell_quote(get_variable(buffer)));
+       dst.append(is_shell ? shell_quote(get_variable(buffer)) : get_variable(buffer));
 }

 const string
@@ -731,7 +731,7 @@ config::expand_string(const char *src, const char *prepend, const char *append)
                }
                dst.append(src, var_at - src);
                src = var_at;
-               expand_one(src, dst);
+               expand_one(src, dst, prepend == NULL);
        }

        if (append != NULL)
diff --git a/sbin/devd/devd.hh b/sbin/devd/devd.hh
index 534c4d4c5a9d..d770da22226a 100644
--- a/sbin/devd/devd.hh
+++ b/sbin/devd/devd.hh
@@ -172,7 +172,7 @@ protected:
        void sort_vector(std::vector<event_proc *> &);
        void parse_one_file(const char *fn);
        void parse_files_in_dir(const char *dirname);
-       void expand_one(const char *&src, std::string &dst);
+       void expand_one(const char *&src, std::string &dst, bool is_shell);
        std::string shell_quote(const std::string &s);
        bool is_id_char(char) const;
        bool chop_var(char *&buffer, char *&lhs, char *&rhs) const;
Comment 15 Warner Losh freebsd_committer 2018-09-21 05:23:42 UTC
looks like I missed the patch before doing my own :)
I think Yuri's would work, but mine is simpler.
Comment 16 Warner Losh freebsd_committer 2018-09-21 05:28:57 UTC
https://reviews.freebsd.org/D17267

has the review, and is where we should continue discussions about the code.
Comment 17 commit-hook freebsd_committer 2018-09-22 15:33:46 UTC
A commit references this bug:

Author: imp
Date: Sat Sep 22 15:32:54 UTC 2018
New revision: 338888
URL: https://svnweb.freebsd.org/changeset/base/338888

Log:
  We don't need shell protection for when we're expanding matches.
  Don't add it. This should fix when we do regepx matches against
  variables we've set and fix wifi bring up.

  PR: 231441
  Approved by: re@ (kib)
  Differential Revision: https://reviews.freebsd.org/D17267

Changes:
  head/sbin/devd/devd.cc
  head/sbin/devd/devd.hh
Comment 18 Warner Losh freebsd_committer 2018-10-10 00:59:13 UTC
fixed in current, not relevant to -stable. Patch went in and no further reports of breakage.