Bug#780705: udev: floating net device number on devices with ro rootfs

[You-Sheng Yang]

On Wed, 18 Mar 2015 18:46:00 +0100 Michael Biebl <biebl at debian.org> wrote:
> Dear You-Sheng Yang,
> 
> first of all, the bug report was filed against the wrong package. The
> package dealing with the network renaming is udev. Therefore reassigning
> to udev.

Sorry, I thought it had to be filed under its source package.

> Second, the patch itself is not working. The temporary file is called
> /run/udev/tmp-rules--70-persistent-net.rules, not tmp-ruiles--..., so
> this patch can't have been tested, therefore removing the patch tag.

Yes, I noticed that but still re-attached the wrong one.

> Third, and this is the most important part, the patch/solution you are
> suggesting, don't work / make any sense. The persistent network interface
> naming feature as used by default in Debian udev requires a persistent
> storage, i.e. a writable /etc/udev/rules.d by design. If you deviate
> from a default Debian setup and make / or /etc ro, you can simply not
> use this particular feature. Simple as that.

https://wiki.debian.org/ReadonlyRoot
Looks like read-only rootfs is also permitted in Debian. Bugs blocking
ro rootfs are accepted in other packages, and there is also a small
section addressing udev under ro rootfs.

Anyway, here is another patch that solves the same problem. It actually
tries to avoid duplicated written entries in "70-persistent-net.rules".
If there is already a rule with the same match conditions, let
"write_net_rules" reuse the existing one and therefore it increases
device name no more. This patch is generated directly from the device
under test.

-- 
You-Sheng Yang (Vicamo)
-------------- next part --------------
From ca74f1efff06983160fe28bc3f931de73438de0a Mon Sep 17 00:00:00 2001
From: You-Sheng Yang <vicamo at gmail.com>
Date: Wed, 18 Mar 2015 11:33:32 +0800
Subject: [PATCH] Avoid duplicated udev match rules

On devices with read-only rootfs, e.g. mobile phones, nic device number
(wlan<N>) may increase every time disabled and re-enabled. To be more
precisely, this happens only on devices when disabling a nic removes the
corresponding driver.

"/lib/udev/rules.d/75-persistent-net-generator.rules" checks whether
NAME attribute has been assigned to wlan<N> device: if yes, skip all the
followed steps, or, call to "/lib/udev/write_net_rules" to generate a
persistent device name rule file. That persistent file should be created
under "/etc/udev/rules.d" and named "70-persistent-net.rules", so it
guarantees NAME attribute should be assigned if available before being
read. However, when rootfs was previously mounted as read-only, a file
"/run/udev/tmp-rules--70-persistent-net.rules" is created instead. This
temporary file is supposed to be moved back into "/etc/udev/rules.d" by
a systemd service udev-finish right after the system finishes start-up
chaos. Again, if rootfs is still mounted as read-only, this move will
certainly fail. One last important thing, /run/udev is _NOT_ included in
udev rules inclusion paths, so any rules written here will not be taken
into account when processing uevents.

So, when wlan0 is probed for the first time on a device with read-only
rootfs, udev creates "/run/udev/tmp-rules--70-persistent-net.rules" and
inserts one rule for it. When wlan0 is disabled and re-enabled, since
"/run/udev/tmp-rules--70-persistent-net.rules" is not taken into
account, its NAME attribute will not be set, and udev recognize it as a
new nic and tries to write another rule for it again. However, in this
time, "wlan0" has been taken in the previously written temporary rules
file, so "wlan1" is chosen instead, and an exactly the same matching
rule (except for NAME= part) is appended to
"/run/udev/tmp-rules--70-persistent-net.rules". When the device is
again disabled and re-enabled, "wlan2" will be assigned. And so on ....

This patch add an additional step to search if current match rule had
been previously written and reuse that interface name if available.

Signed-off-by: You-Sheng Yang <vicamo at gmail.com>

diff --git a/debian/extra/rule_generator.functions b/debian/extra/rule_generator.functions
index 925193e..a676ff7 100644
--- a/debian/extra/rule_generator.functions
+++ b/debian/extra/rule_generator.functions
@@ -100,14 +100,14 @@ raw_find_next_available() {
 }
 
 # Find all rules matching a key (with action) and a pattern.
 find_all_rules() {
 	local key="$1"
 	local linkre="$2"
 	local match="$3"
 
-	local search='.*[[:space:],]'"$key"'"('"$linkre"')".*'
+	local search='.*'"$key"'"('"$linkre"')".*'
 	echo $(sed -n -r -e 's/^#.*//' -e "${match}s/${search}/\1/p" \
 		$RO_RULES_FILE \
 		$([ -e $RULES_FILE ] && echo $RULES_FILE) \
 		2>/dev/null)
 }
diff --git a/debian/extra/write_net_rules b/debian/extra/write_net_rules
index 4379792..b05ed43 100644
--- a/debian/extra/write_net_rules
+++ b/debian/extra/write_net_rules
@@ -38,26 +38,26 @@ if [ -n "$UDEV_LOG" ]; then
 	fi
 fi
 
 RULES_FILE='/etc/udev/rules.d/70-persistent-net.rules'
 
 . /lib/udev/rule_generator.functions
 
 interface_name_taken() {
-	local value="$(find_all_rules 'NAME=' $INTERFACE)"
+	local value="$(find_all_rules '[[:space:],]NAME=' $INTERFACE)"
 	if [ "$value" ]; then
 		return 0
 	else
 		return 1
 	fi
 }
 
 find_next_available() {
-	raw_find_next_available "$(find_all_rules 'NAME=' "$1")"
+	raw_find_next_available "$(find_all_rules '[[:space:],]NAME=' "$1")"
 }
 
 write_rule() {
 	local match="$1"
 	local name="$2"
 	local comment="$3"
 
 	{
@@ -122,20 +122,30 @@ if [ "$INTERFACE_NAME" ]; then
 	COMMENT="$COMMENT (custom name provided by external tool)"
 	if [ "$INTERFACE_NAME" != "$INTERFACE" ]; then
 		INTERFACE=$INTERFACE_NAME;
 		echo "INTERFACE_NEW=$INTERFACE"
 	fi
 else
 	# if a rule using the current name already exists, find a new name
 	if interface_name_taken; then
-		INTERFACE="$basename$(find_next_available "$basename[0-9]*")"
-		# prevent INTERFACE from being "eth" instead of "eth0"
-		[ "$INTERFACE" = "${INTERFACE%%[ \[\]0-9]*}" ] && INTERFACE=${INTERFACE}0
-		echo "INTERFACE_NEW=$INTERFACE"
+		match_exp="$(echo "$match" | sed -e s/{/\\\\{/g -e s/}/\\\\}/g -e s/*/\\\\*/g -e s/?/\\\\?/g)"
+		INTERFACE_NAME="$(find_all_rules 'SUBSYSTEM=="net", ACTION=="add"'"${match_exp}"', NAME=' "$basename[0-9]*")"
+		if [ -z "$INTERFACE_NAME" ]; then
+			INTERFACE="$basename$(find_next_available "$basename[0-9]*")"
+			# prevent INTERFACE from being "eth" instead of "eth0"
+			[ "$INTERFACE" = "${INTERFACE%%[ \[\]0-9]*}" ] && INTERFACE=${INTERFACE}0
+			echo "INTERFACE_NEW=$INTERFACE"
+		elif [ "$INTERFACE_NAME" != "$INTERFACE" ]; then
+			INTERFACE=$INTERFACE_NAME
+			echo "INTERFACE_NEW=$INTERFACE"
+		else
+			unlock_rules_file
+			exit 0
+		fi
 	fi
 fi
 
 write_rule "$match" "$INTERFACE" "$COMMENT"
 
 unlock_rules_file
 
 exit 0
-- 
2.1.4