Re: Strange udev problem - (Found the problem and a temporary fix)

Re: Strange udev problem - (Found the problem and a temporary fix)

[Jim Gifford]

[-- Attachment #1: Type: text/plain, Size: 8497 bytes --]

If found the cause of a problem, and a temporary fix.

Cause:
http://linuxusb.bkbits.net:8080/udev/cset@1.759?nav=index.html|tags|ChangeSet@..1.760

Fix:
Submitted By: Jim Gifford (jim at linuxfromscratch dot org)
Date: 2004-06-21
Initial Package Version: 027
Origin: Jim Gifford
Upstream Status: N/A
Description: Reverts tweak node unlink handling
http://linuxusb.bkbits.net:8080/udev/cset@1.759?nav=index.html|tags|ChangeSet@..1.7601
Symptons of problem: When cron executes a bash script, /dev/null changes
from the
permissions lists in udev.rules to 600. This patch seems to correct the
issue,
I am submitting it as a temporary fix util a better fix can be found.

diff -Naur udev-027.orig/udev-add.c udev-027/udev-add.c
--- udev-027.orig/udev-add.c 2004-06-14 20:38:35.000000000 +0000
+++ udev-027/udev-add.c 2004-06-21 20:30:36.686112569 +0000
@@ -61,20 +61,21 @@
*/
static int get_major_minor(struct sysfs_class_device *class_dev, struct
udevice *udev)
{
+ int retval = -ENODEV;
struct sysfs_attribute *attr = NULL;
attr = sysfs_get_classdev_attr(class_dev, "dev");
if (attr == NULL)
- goto error;
+ goto exit;
dbg("dev='%s'", attr->value);
if (sscanf(attr->value, "%u:%u", &udev->major, &udev->minor) != 2)
- goto error;
+ goto exit;
dbg("found major=%d, minor=%d", udev->major, udev->minor);
- return 0;
-error:
- return -1;
+ retval = 0;
+exit:
+ return retval;
}
static int create_path(char *file)
@@ -105,52 +106,36 @@
return 0;
}
-static int make_node(char *file, int major, int minor, unsigned int mode,
uid_t uid, gid_t gid)
+static int make_node(char *filename, int major, int minor, unsigned int
mode, uid_t uid, gid_t gid)
{
- struct stat stats;
- int retval = 0;
-
- if (stat(file, &stats) != 0)
- goto create;
-
- /* preserve node with already correct numbers, to not change the inode
number */
- if (((stats.st_mode & S_IFMT) == S_IFBLK || (stats.st_mode & S_IFMT) ==
S_IFCHR) &&
- (stats.st_rdev == makedev(major, minor))) {
- dbg("preserve file '%s', cause it has correct dev_t", file);
- goto perms;
- }
-
- if (unlink(file) != 0)
- dbg("unlink(%s) failed with error '%s'", file, strerror(errno));
- else
- dbg("already present file '%s' unlinked", file);
+ int retval;
-create:
- retval = mknod(file, mode, makedev(major, minor));
+ retval = mknod(filename, mode, makedev(major, minor));
if (retval != 0) {
dbg("mknod(%s, %#o, %u, %u) failed with error '%s'",
- file, mode, major, minor, strerror(errno));
- goto exit;
+ filename, mode, major, minor, strerror(errno));
+ return retval;
}
-perms:
- dbg("chmod(%s, %#o)", file, mode);
- if (chmod(file, mode) != 0) {
- dbg("chmod(%s, %#o) failed with error '%s'", file, mode, strerror(errno));
- goto exit;
+ dbg("chmod(%s, %#o)", filename, mode);
+ retval = chmod(filename, mode);
+ if (retval != 0) {
+ dbg("chmod(%s, %#o) failed with error '%s'",
+ filename, mode, strerror(errno));
+ return retval;
}
if (uid != 0 || gid != 0) {
- dbg("chown(%s, %u, %u)", file, uid, gid);
- if (chown(file, uid, gid) != 0) {
+ dbg("chown(%s, %u, %u)", filename, uid, gid);
+ retval = chown(filename, uid, gid);
+ if (retval != 0) {
dbg("chown(%s, %u, %u) failed with error '%s'",
- file, uid, gid, strerror(errno));
- goto exit;
+ filename, uid, gid, strerror(errno));
+ return retval;
}
}
-exit:
- return retval;
+ return 0;
}
/* get the local logged in user */
@@ -184,12 +169,31 @@
endutent();
}
+/* Used to unlink existing files to ensure that our new file/symlink is
created */
+static int unlink_entry(char *filename)
+{
+ struct stat stats;
+ int retval = 0;
+
+ if (lstat(filename, &stats) == 0) {
+ if ((stats.st_mode & S_IFMT) != S_IFDIR) {
+ retval = unlink(filename);
+ if (retval) {
+ dbg("unlink(%s) failed with error '%s'",
+ filename, strerror(errno));
+ }
+ }
+ }
+ return retval;
+}
+
static int create_node(struct udevice *dev, int fake)
{
char filename[NAME_SIZE];
char linkname[NAME_SIZE];
char linktarget[NAME_SIZE];
char partitionname[NAME_SIZE];
+ int retval = 0;
uid_t uid = 0;
gid_t gid = 0;
int i;
@@ -253,29 +257,30 @@
}
if (!fake) {
+ unlink_entry(filename);
info("creating device node '%s'", filename);
- if (make_node(filename, dev->major, dev->minor, dev->mode, uid, gid) != 0)
- goto error;
+ make_node(filename, dev->major, dev->minor, dev->mode, uid, gid);
} else {
info("creating device node '%s', major = '%d', minor = '%d', "
"mode = '%#o', uid = '%d', gid = '%d'", filename,
dev->major, dev->minor, (mode_t)dev->mode, uid, gid);
}
- /* create all_partitions if requested */
+ /* create partitions if requested */
if (dev->partitions > 0) {
info("creating device partition nodes '%s[1-%i]'", filename,
dev->partitions);
if (!fake) {
for (i = 1; i <= dev->partitions; i++) {
strfieldcpy(partitionname, filename);
strintcat(partitionname, i);
+ unlink_entry(partitionname);
make_node(partitionname, dev->major,
dev->minor + i, dev->mode, uid, gid);
}
}
}
- /* create symlink(s) if requested */
+ /* create symlink if requested */
foreach_strpart(dev->symlink, " ", pos, len) {
strfieldcpymax(linkname, pos, len+1);
strfieldcpy(filename, udev_root);
@@ -302,18 +307,19 @@
strfieldcat(linktarget, &dev->name[tail]);
+ if (!fake)
+ unlink_entry(filename);
+
dbg("symlink(%s, %s)", linktarget, filename);
if (!fake) {
- unlink(filename);
- if (symlink(linktarget, filename) != 0)
+ retval = symlink(linktarget, filename);
+ if (retval != 0)
dbg("symlink(%s, %s) failed with error '%s'",
linktarget, filename, strerror(errno));
}
}
- return 0;
-error:
- return -1;
+ return retval;
}
static struct sysfs_class_device *get_class_dev(char *device_name)
@@ -367,16 +373,12 @@
return retval;
}
-static int rename_net_if(struct udevice *dev, int fake)
+static int rename_net_if(struct udevice *dev)
{
int sk;
struct ifreq ifr;
int retval;
- dbg("changing net interface name from '%s' to '%s'", dev->kernel_name,
dev->name);
- if (fake)
- return 0;
-
sk = socket(PF_INET, SOCK_DGRAM, 0);
if (sk < 0) {
dbg("error opening socket");
@@ -387,25 +389,26 @@
strfieldcpy(ifr.ifr_name, dev->kernel_name);
strfieldcpy(ifr.ifr_newname, dev->name);
+ dbg("changing net interface name from '%s' to '%s'", dev->kernel_name,
dev->name);
retval = ioctl(sk, SIOCSIFNAME, &ifr);
if (retval != 0)
dbg("error changing net interface name");
- close(sk);
return retval;
}
int udev_add_device(char *path, char *subsystem, int fake)
{
- struct sysfs_class_device *class_dev;
+ struct sysfs_class_device *class_dev = NULL;
struct udevice dev;
- char devpath[DEVPATH_SIZE];
- char *pos;
- int retval;
+ int retval = -EINVAL;
memset(&dev, 0x00, sizeof(dev));
+ /* for now, the block layer is the only place where block devices are */
+
dev.type = get_device_type(path, subsystem);
+
switch (dev.type) {
case 'b':
case 'c':
@@ -418,12 +421,12 @@
default:
dbg("unknown device type '%c'", dev.type);
- return -1;
+ retval = -EINVAL;
}
class_dev = get_class_dev(path);
if (class_dev == NULL)
- return -1;
+ goto exit;
if (dev.type == 'b' || dev.type == 'c') {
retval = get_major_minor(class_dev, &dev);
@@ -433,49 +436,38 @@
}
}
- if (namedev_name_device(class_dev, &dev) != 0)
+ retval = namedev_name_device(class_dev, &dev);
+ if (retval != 0)
goto exit;
- dbg("name='%s'", dev.name);
-
- switch (dev.type) {
- case 'b':
- case 'c':
- retval = create_node(&dev, fake);
+ if (!fake && (dev.type == 'b' || dev.type == 'c')) {
+ retval = udevdb_add_dev(path, &dev);
if (retval != 0)
- goto exit;
- if ((!fake) && (udevdb_add_dev(path, &dev) != 0))
dbg("udevdb_add_dev failed, but we are going to try "
"to create the node anyway. But remove might not "
"work properly for this device.");
+ }
- dev_d_send(&dev, subsystem, path);
+ dbg("name='%s'", dev.name);
+ switch (dev.type) {
+ case 'b':
+ case 'c':
+ retval = create_node(&dev, fake);
break;
case 'n':
- strfieldcpy(devpath, path);
- if (strcmp(dev.name, dev.kernel_name) != 0) {
- retval = rename_net_if(&dev, fake);
- if (retval != 0)
- goto exit;
- /* netif's are keyed with the configured name, cause
- * the original kernel name sleeps with the fishes
- */
- pos = strrchr(devpath, '/');
- if (pos != NULL) {
- pos[1] = '\0';
- strfieldcat(devpath, dev.name);
- }
- }
- if ((!fake) && (udevdb_add_dev(devpath, &dev) != 0))
- dbg("udevdb_add_dev failed");
-
- dev_d_send(&dev, subsystem, devpath);
+ retval = rename_net_if(&dev);
+ if (retval != 0)
+ dbg("net device naming failed");
break;
}
+ if ((retval == 0) && (!fake))
+ dev_d_send(&dev, subsystem, path);
+
exit:
- sysfs_close_class_device(class_dev);
+ if (class_dev)
+ sysfs_close_class_device(class_dev);
return retval;
}

[-- Attachment #2: udev-027-permissions-1[1].patch --]
[-- Type: application/octet-stream, Size: 9182 bytes --]

Submitted By: Jim Gifford (jim at linuxfromscratch dot org)
Date: 2004-06-21
Initial Package Version: 027
Origin: Jim Gifford
Upstream Status: N/A
Description: Reverts tweak node unlink handling
 	http://linuxusb.bkbits.net:8080/udev/cset@1.759?nav=index.html|tags|ChangeSet@..1.7601

	Symptons of problem: When cron executes a bash script, /dev/null changes from the
	permissions lists in udev.rules to 600. This patch seems to correct the issue,
	I am submitting it as a temporary fix util a better fix can be found.


diff -Naur udev-027.orig/udev-add.c udev-027/udev-add.c
--- udev-027.orig/udev-add.c	2004-06-14 20:38:35.000000000 +0000
+++ udev-027/udev-add.c	2004-06-21 20:30:36.686112569 +0000
@@ -61,20 +61,21 @@
  */
 static int get_major_minor(struct sysfs_class_device *class_dev, struct udevice *udev)
 {
+	int retval = -ENODEV;
 	struct sysfs_attribute *attr = NULL;
 
 	attr = sysfs_get_classdev_attr(class_dev, "dev");
 	if (attr == NULL)
-		goto error;
+		goto exit;
 	dbg("dev='%s'", attr->value);
 
 	if (sscanf(attr->value, "%u:%u", &udev->major, &udev->minor) != 2)
-		goto error;
+		goto exit;
 	dbg("found major=%d, minor=%d", udev->major, udev->minor);
 
-	return 0;
-error:
-	return -1;
+	retval = 0;
+exit:
+	return retval;
 }
 
 static int create_path(char *file)
@@ -105,52 +106,36 @@
 	return 0;
 }
 
-static int make_node(char *file, int major, int minor, unsigned int mode, uid_t uid, gid_t gid)
+static int make_node(char *filename, int major, int minor, unsigned int mode, uid_t uid, gid_t gid)
 {
-	struct stat stats;
-	int retval = 0;
-
-	if (stat(file, &stats) != 0)
-		goto create;
-
-	/* preserve node with already correct numbers, to not change the inode number */
-	if (((stats.st_mode & S_IFMT) == S_IFBLK || (stats.st_mode & S_IFMT) == S_IFCHR) &&
-	    (stats.st_rdev == makedev(major, minor))) {
-		dbg("preserve file '%s', cause it has correct dev_t", file);
-		goto perms;
-	}
-
-	if (unlink(file) != 0)
-		dbg("unlink(%s) failed with error '%s'", file, strerror(errno));
-	else
-		dbg("already present file '%s' unlinked", file);
+	int retval;
 
-create:
-	retval = mknod(file, mode, makedev(major, minor));
+	retval = mknod(filename, mode, makedev(major, minor));
 	if (retval != 0) {
 		dbg("mknod(%s, %#o, %u, %u) failed with error '%s'",
-		    file, mode, major, minor, strerror(errno));
-		goto exit;
+		    filename, mode, major, minor, strerror(errno));
+		return retval;
 	}
 
-perms:
-	dbg("chmod(%s, %#o)", file, mode);
-	if (chmod(file, mode) != 0) {
-		dbg("chmod(%s, %#o) failed with error '%s'", file, mode, strerror(errno));
-		goto exit;
+	dbg("chmod(%s, %#o)", filename, mode);
+	retval = chmod(filename, mode);
+	if (retval != 0) {
+		dbg("chmod(%s, %#o) failed with error '%s'",
+		    filename, mode, strerror(errno));
+		return retval;
 	}
 
 	if (uid != 0 || gid != 0) {
-		dbg("chown(%s, %u, %u)", file, uid, gid);
-		if (chown(file, uid, gid) != 0) {
+		dbg("chown(%s, %u, %u)", filename, uid, gid);
+		retval = chown(filename, uid, gid);
+		if (retval != 0) {
 			dbg("chown(%s, %u, %u) failed with error '%s'",
-			    file, uid, gid, strerror(errno));
-			goto exit;
+			    filename, uid, gid, strerror(errno));
+			return retval;
 		}
 	}
 
-exit:
-	return retval;
+	return 0;
 }
 
 /* get the local logged in user */
@@ -184,12 +169,31 @@
 	endutent();
 }
 
+/* Used to unlink existing files to ensure that our new file/symlink is created */
+static int unlink_entry(char *filename)
+{
+	struct stat stats;
+	int retval = 0;
+	
+	if (lstat(filename, &stats) == 0) {
+		if ((stats.st_mode & S_IFMT) != S_IFDIR) {
+			retval = unlink(filename);
+			if (retval) {
+				dbg("unlink(%s) failed with error '%s'",
+				    filename, strerror(errno));
+			}
+		}
+	}
+	return retval;
+}
+
 static int create_node(struct udevice *dev, int fake)
 {
 	char filename[NAME_SIZE];
 	char linkname[NAME_SIZE];
 	char linktarget[NAME_SIZE];
 	char partitionname[NAME_SIZE];
+	int retval = 0;
 	uid_t uid = 0;
 	gid_t gid = 0;
 	int i;
@@ -253,29 +257,30 @@
 	}
 
 	if (!fake) {
+		unlink_entry(filename);
 		info("creating device node '%s'", filename);
-		if (make_node(filename, dev->major, dev->minor, dev->mode, uid, gid) != 0)
-			goto error;
+		make_node(filename, dev->major, dev->minor, dev->mode, uid, gid);
 	} else {
 		info("creating device node '%s', major = '%d', minor = '%d', "
 		     "mode = '%#o', uid = '%d', gid = '%d'", filename,
 		     dev->major, dev->minor, (mode_t)dev->mode, uid, gid);
 	}
 
-	/* create all_partitions if requested */
+	/* create partitions if requested */
 	if (dev->partitions > 0) {
 		info("creating device partition nodes '%s[1-%i]'", filename, dev->partitions);
 		if (!fake) {
 			for (i = 1; i <= dev->partitions; i++) {
 				strfieldcpy(partitionname, filename);
 				strintcat(partitionname, i);
+				unlink_entry(partitionname);
 				make_node(partitionname, dev->major,
 					  dev->minor + i, dev->mode, uid, gid);
 			}
 		}
 	}
 
-	/* create symlink(s) if requested */
+	/* create symlink if requested */
 	foreach_strpart(dev->symlink, " ", pos, len) {
 		strfieldcpymax(linkname, pos, len+1);
 		strfieldcpy(filename, udev_root);
@@ -302,18 +307,19 @@
 
 		strfieldcat(linktarget, &dev->name[tail]);
 
+		if (!fake)
+			unlink_entry(filename);
+
 		dbg("symlink(%s, %s)", linktarget, filename);
 		if (!fake) {
-			unlink(filename);
-			if (symlink(linktarget, filename) != 0)
+			retval = symlink(linktarget, filename);
+			if (retval != 0)
 				dbg("symlink(%s, %s) failed with error '%s'",
 				    linktarget, filename, strerror(errno));
 		}
 	}
 
-	return 0;
-error:
-	return -1;
+	return retval;
 }
 
 static struct sysfs_class_device *get_class_dev(char *device_name)
@@ -367,16 +373,12 @@
 	return retval;
 }
 
-static int rename_net_if(struct udevice *dev, int fake)
+static int rename_net_if(struct udevice *dev)
 {
 	int sk;
 	struct ifreq ifr;
 	int retval;
 
-	dbg("changing net interface name from '%s' to '%s'", dev->kernel_name, dev->name);
-	if (fake)
-		return 0;
-
 	sk = socket(PF_INET, SOCK_DGRAM, 0);
 	if (sk < 0) {
 		dbg("error opening socket");
@@ -387,25 +389,26 @@
 	strfieldcpy(ifr.ifr_name, dev->kernel_name);
 	strfieldcpy(ifr.ifr_newname, dev->name);
 
+	dbg("changing net interface name from '%s' to '%s'", dev->kernel_name, dev->name);
 	retval = ioctl(sk, SIOCSIFNAME, &ifr);
 	if (retval != 0)
 		dbg("error changing net interface name");
-	close(sk);
 
 	return retval;
 }
 
 int udev_add_device(char *path, char *subsystem, int fake)
 {
-	struct sysfs_class_device *class_dev;
+	struct sysfs_class_device *class_dev = NULL;
 	struct udevice dev;
-	char devpath[DEVPATH_SIZE];
-	char *pos;
-	int retval;
+	int retval = -EINVAL;
 
 	memset(&dev, 0x00, sizeof(dev));
 
+	/* for now, the block layer is the only place where block devices are */
+
 	dev.type = get_device_type(path, subsystem);
+
 	switch (dev.type) {
 	case 'b':
 	case 'c':
@@ -418,12 +421,12 @@
 
 	default:
 		dbg("unknown device type '%c'", dev.type);
-		return -1;
+		retval = -EINVAL;
 	}
 
 	class_dev = get_class_dev(path);
 	if (class_dev == NULL)
-		return -1;
+		goto exit;
 
 	if (dev.type == 'b' || dev.type == 'c') {
 		retval = get_major_minor(class_dev, &dev);
@@ -433,49 +436,38 @@
 		}
 	}
 
-	if (namedev_name_device(class_dev, &dev) != 0)
+	retval = namedev_name_device(class_dev, &dev);
+	if (retval != 0)
 		goto exit;
 
-	dbg("name='%s'", dev.name);
-
-	switch (dev.type) {
-	case 'b':
-	case 'c':
-		retval = create_node(&dev, fake);
+	if (!fake && (dev.type == 'b' || dev.type == 'c')) {
+		retval = udevdb_add_dev(path, &dev);
 		if (retval != 0)
-			goto exit;
-		if ((!fake) && (udevdb_add_dev(path, &dev) != 0))
 			dbg("udevdb_add_dev failed, but we are going to try "
 			    "to create the node anyway. But remove might not "
 			    "work properly for this device.");
+	}
 
-		dev_d_send(&dev, subsystem, path);
+	dbg("name='%s'", dev.name);
+	switch (dev.type) {
+	case 'b':
+	case 'c':
+		retval = create_node(&dev, fake);
 		break;
 
 	case 'n':
-		strfieldcpy(devpath, path);
-		if (strcmp(dev.name, dev.kernel_name) != 0) {
-			retval = rename_net_if(&dev, fake);
-			if (retval != 0)
-				goto exit;
-			/* netif's are keyed with the configured name, cause
-			 * the original kernel name sleeps with the fishes
-			 */
-			pos = strrchr(devpath, '/');
-			if (pos != NULL) {
-				pos[1] = '\0';
-				strfieldcat(devpath, dev.name);
-			}
-		}
-		if ((!fake) && (udevdb_add_dev(devpath, &dev) != 0))
-			dbg("udevdb_add_dev failed");
-
-		dev_d_send(&dev, subsystem, devpath);
+		retval = rename_net_if(&dev);
+		if (retval != 0)
+			dbg("net device naming failed");
 		break;
 	}
 
+	if ((retval == 0) && (!fake))
+		dev_d_send(&dev, subsystem, path);
+
 exit:
-	sysfs_close_class_device(class_dev);
+	if (class_dev)
+		sysfs_close_class_device(class_dev);
 
 	return retval;
 }

Re: Strange udev problem - (Found the problem and a temporary fix)

[Greg KH]

On Mon, Jun 21, 2004 at 01:43:49PM -0700, Jim Gifford wrote:
> If found the cause of a problem, and a temporary fix.
> 
> Cause:
> http://linuxusb.bkbits.net:8080/udev/cset@1.759?nav=index.html|tags|ChangeSet@..1.760

What?  How?

> Fix:
> Submitted By: Jim Gifford (jim at linuxfromscratch dot org)
> Date: 2004-06-21
> Initial Package Version: 027
> Origin: Jim Gifford
> Upstream Status: N/A
> Description: Reverts tweak node unlink handling

You're just reverting the code, that's not ok.

I still don't see how udev is at fault here, as you see no log messages
from it.

Didn't you also do a "echo /bin/true > /proc/sys/kernel/hotplug" to
disable udev and the hotplug events entirely and still had the problem?
That rules out udev.

thanks,

> http://linuxusb.bkbits.net:8080/udev/cset@1.759?nav=index.html|tags|ChangeSet@..1.7601
> Symptons of problem: When cron executes a bash script, /dev/null changes
> from the permissions lists in udev.rules to 600. This patch seems to
> correct the issue, I am submitting it as a temporary fix util a better
> fix can be found.

Do you have a small example script that shows this happening?  I thought
the other script you posted just watched what happened in /dev, right?

thanks,

greg k-h


-------------------------------------------------------
This SF.Net email sponsored by Black Hat Briefings & Training.
Attend Black Hat Briefings & Training, Las Vegas July 24-29 - 
digital self defense, top technical experts, no vendor pitches, 
unmatched networking opportunities. Visit www.blackhat.com
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel