Skip to contentSkip to conversation

udev change event

DISCUSSION Harald Hoyer 5 participants 14 messages Jul 3, 2008 – Jul 10, 2008

lore
Share & tools
lore

Harald Hoyer

author
#0 lore raw

With recent changes, udev processes "change" events for removable devices and resets the permissions, if it has changed since creation. Is this intended? Or do we just want a new vol_id symlink?

#1 lore raw
Quoted 3 lines · Harald Hoyer

With recent changes, udev processes "change" events for removable devices and resets the permissions, if it has changed since creation. Is this intended? Or do we just want a new vol_id symlink?

I think this is intended: manually changing the permissions of dynamic devices is not supposed to work.

Signature · 2 lines
ciao,
Marco
#2 lore raw
In reply to Harald Hoyer
Quoted 7 lines · Marco d'Itri
Quoted 3 lines · Harald Hoyer

With recent changes, udev processes "change" events for removable devices and resets the permissions, if it has changed since creation. Is this intended? Or do we just want a new vol_id symlink?

I think this is intended: manually changing the permissions of dynamic devices is not supposed to work.

....? How do you handle ACLs for local users, then?

Bill

#3 lore raw
In reply to Harald Hoyer
Quoted 10 lines · Bill Nottingham
Quoted 7 lines · Marco d'Itri
Quoted 3 lines · Harald Hoyer

With recent changes, udev processes "change" events for removable devices and resets the permissions, if it has changed since creation. Is this intended? Or do we just want a new vol_id symlink?

I think this is intended: manually changing the permissions of dynamic devices is not supposed to work.

....? How do you handle ACLs for local users, then?

Re-apply them, with the same logic that did it in the first place?

Kay

#4 lore raw
In reply to Harald Hoyer
Quoted 3 lines · Bill Nottingham
Quoted 2 lines

I think this is intended: manually changing the permissions of dynamic devices is not supposed to work.

....? How do you handle ACLs for local users, then?

I don't (not my package), but if I had to I would dynamically maintain rules in /dev/.udev/rules.d/ .

Signature · 2 lines
ciao,
Marco
#6 lore raw
In reply to Harald Hoyer
Quoted 7 lines · Marco d'Itri
Quoted 3 lines · Bill Nottingham
Quoted 2 lines

I think this is intended: manually changing the permissions of dynamic devices is not supposed to work.

....? How do you handle ACLs for local users, then?

I don't (not my package), but if I had to I would dynamically maintain rules in /dev/.udev/rules.d/ .

So, instead of just calling setfacl() when a user logs in, it's better to write udev syntax and kill the daemon (oh, and then generate an event to apply them?)

That's just nuts. Far better to change all the distributed rules to only have MODE= on 'add' events, not change ones.

Bill

#7 lore raw
In reply to Harald Hoyer
Quoted 3 lines · Bill Nottingham

So, instead of just calling setfacl() when a user logs in, it's better to write udev syntax and kill the daemon (oh, and then generate an event to apply them?)

You do not need to kill the daemon, and if no event is received to apply it then I suppose that there is no need to access the drive.

Signature · 2 lines
ciao,
Marco
#8 lore raw
In reply to Harald Hoyer
Quoted 6 lines · Marco d'Itri
Quoted 3 lines

So, instead of just calling setfacl() when a user logs in, it's better to write udev syntax and kill the daemon (oh, and then generate an event to apply them?)

You do not need to kill the daemon, and if no event is received to apply it then I suppose that there is no need to access the drive.

I'm not following you here. Here's the scenario you're proposing I implement:

- CD w/blank media exists, is 0640 root/disk (or whatever)
- I log in
- Rather than set the ACLs, the system daemon writes a udev rule
- ... how does this not work without an event, and what would theoretically provide it?

Bill

#9 lore raw
In reply to Harald Hoyer
Quoted 5 lines · Bill Nottingham

- CD w/blank media exists, is 0640 root/disk (or whatever)
- I log in
- Rather than set the ACLs, the system daemon writes a udev rule
- ... how does this not work without an event, and what would theoretically provide it?

Inserting the CD. If it was inserted by a precedent user, do you really want to change the device owner?

Signature · 2 lines
ciao,
Marco
#10 lore raw
In reply to Harald Hoyer
Quoted 9 lines · Marco d'Itri
Quoted 5 lines · Bill Nottingham

- CD w/blank media exists, is 0640 root/disk (or whatever)
- I log in
- Rather than set the ACLs, the system daemon writes a udev rule
- ... how does this not work without an event, and what would theoretically provide it?

Inserting the CD. If it was inserted by a precedent user, do you really want to change the device owner?

Substitute 'precedent user' with 'no user', and yes, why not?

Doing this all as udev rules seems very wrong.

Bill

#11 lore raw
In reply to Harald Hoyer
Quoted 7 lines · Kay Sievers
Quoted 6 lines · Bill Nottingham
Quoted 5 lines · Marco d'Itri
Quoted 3 lines · Harald Hoyer

With recent changes, udev processes "change" events for removable devices and resets the permissions, if it has changed since creation. Is this intended? Or do we just want a new vol_id symlink?

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

I think this is intended: manually changing the permissions of dynamic devices is not supposed to work.

....? How do you handle ACLs for local users, then?

Re-apply them, with the same logic that did it in the first place?

Racy. The ACL's are applied long after udev stopped processing rules. Better to fix udev to only apply permissions on "add" events.

David

#12 lore raw
In reply to Harald Hoyer
Quoted 11 lines · David Zeuthen
Quoted 10 lines · Kay Sievers
Quoted 9 lines · Bill Nottingham
Quoted 7 lines · Marco d'Itri
Quoted 3 lines · Harald Hoyer

With recent changes, udev processes "change" events for removable devices and resets the permissions, if it has changed since creation. Is this intended? Or do we just want a new vol_id symlink?

I think this is intended: manually changing the permissions of dynamic devices is not supposed to work.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

....? How do you handle ACLs for local users, then?

Re-apply them, with the same logic that did it in the first place?

Racy. The ACL's are applied long after udev stopped processing rules. Better to fix udev to only apply permissions on "add" events.

We should just leave the perms and ownership as they are, if they already match the udev values, in the same way we preserve the inode.

That way all ACL's are preserved, and hooking into "change" to set permissions will still work.

Does the following work for you?

Thanks,
Kay

udev_node.c +38 -37
index 0e59e2d..732cce4 100644
@@ -39,7 +39,8 @@ int udev_node_mknod(struct udevice *udev, const char *file, dev_t devt, mode_t m
 {
 	char file_tmp[PATH_SIZE + sizeof(TMP_FILE_EXT)];
 	struct stat stats;
-	int retval = 0;
+	int preserve = 0;
+	int err = 0;
 
 	if (major(devt) != 0 && strcmp(udev->dev->subsystem, "block") == 0)
 		mode |= S_IFBLK;

@@ -47,56 +48,56 @@ int udev_node_mknod(struct udevice *udev, const char *file, dev_t devt, mode_t m
 		mode |= S_IFCHR;
 
 	if (lstat(file, &stats) == 0) {
-		if ((stats.st_mode & S_IFMT) == (mode & S_IFMT) && (stats.st_rdev == devt)) {
+		if (((stats.st_mode & S_IFMT) == (mode & S_IFMT)) && (stats.st_rdev == devt)) {
 			info("preserve file '%s', because it has correct dev_t\n", file);
-			selinux_setfilecon(file, udev->dev->kernel, stats.st_mode);
-			goto perms;
+			preserve = 1;
+			selinux_setfilecon(file, udev->dev->kernel, mode);
+		} else {
+			info("atomically replace existing file '%s'\n", file);
+			strlcpy(file_tmp, file, sizeof(file_tmp));
+			strlcat(file_tmp, TMP_FILE_EXT, sizeof(file_tmp));
+			unlink(file_tmp);
+			selinux_setfscreatecon(file_tmp, udev->dev->kernel, mode);
+			err = mknod(file_tmp, mode, devt);
+			selinux_resetfscreatecon();
+			if (err != 0) {
+				err("mknod(%s, %#o, %u, %u) failed: %s\n",
+				    file_tmp, mode, major(devt), minor(devt), strerror(errno));
+				goto exit;
+			}
+			err = rename(file_tmp, file);
+			if (err != 0) {
+				err("rename(%s, %s) failed: %s\n",
+				    file_tmp, file, strerror(errno));
+				unlink(file_tmp);
+			}
 		}
 	} else {
+		info("mknod(%s, %#o, (%u,%u))\n", file, mode, major(devt), minor(devt));
 		selinux_setfscreatecon(file, udev->dev->kernel, mode);
-		retval = mknod(file, mode, devt);
+		err = mknod(file, mode, devt);
 		selinux_resetfscreatecon();
-		if (retval == 0)
-			goto perms;
-	}
-
-	info("atomically replace '%s'\n", file);
-	strlcpy(file_tmp, file, sizeof(file_tmp));
-	strlcat(file_tmp, TMP_FILE_EXT, sizeof(file_tmp));
-	unlink(file_tmp);
-	selinux_setfscreatecon(file_tmp, udev->dev->kernel, mode);
-	retval = mknod(file_tmp, mode, devt);
-	selinux_resetfscreatecon();
-	if (retval != 0) {
-		err("mknod(%s, %#o, %u, %u) failed: %s\n",
-		    file_tmp, mode, major(devt), minor(devt), strerror(errno));
-		goto exit;
-	}
-	retval = rename(file_tmp, file);
-	if (retval != 0) {
-		err("rename(%s, %s) failed: %s\n",
-		    file_tmp, file, strerror(errno));
-		unlink(file_tmp);
-		goto exit;
+		if (err != 0)
+			goto exit;
 	}
 
-perms:
-	dbg("chmod(%s, %#o)\n", file, mode);
-	if (chmod(file, mode) != 0) {
-		err("chmod(%s, %#o) failed: %s\n", file, mode, strerror(errno));
-		goto exit;
+	if (!preserve || stats.st_mode != mode) {
+		info("chmod(%s, %#o)\n", file, mode);
+		if (chmod(file, mode) != 0) {
+			err("chmod(%s, %#o) failed: %s\n", file, mode, strerror(errno));
+			goto exit;
+		}
 	}
 
-	if (uid != 0 || gid != 0) {
-		dbg("chown(%s, %u, %u)\n", file, uid, gid);
+	if (!preserve || stats.st_uid != uid || stats.st_gid != gid) {
+		info("chown(%s, %u, %u)\n", file, uid, gid);
 		if (chown(file, uid, gid) != 0) {
-			err("chown(%s, %u, %u) failed: %s\n",
-			    file, uid, gid, strerror(errno));
+			err("chown(%s, %u, %u) failed: %s\n", file, uid, gid, strerror(errno));
 			goto exit;
 		}
 	}
 exit:
-	return retval;
+	return err;
 }
 
 static int node_symlink(const char *node, const char *slink)
#13 lore raw
In reply to Harald Hoyer
Quoted 19 lines · Kay Sievers
Quoted 11 lines · David Zeuthen
Quoted 10 lines · Kay Sievers
Quoted 9 lines · Bill Nottingham
Quoted 7 lines · Marco d'Itri
Quoted 3 lines · Harald Hoyer

With recent changes, udev processes "change" events for removable devices and resets the permissions, if it has changed since creation. Is this intended? Or do we just want a new vol_id symlink?

I think this is intended: manually changing the permissions of dynamic devices is not supposed to work.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

....? How do you handle ACLs for local users, then?

Re-apply them, with the same logic that did it in the first place?

Racy. The ACL's are applied long after udev stopped processing rules. Better to fix udev to only apply permissions on "add" events.

Quoted 7 lines

We should just leave the perms and ownership as they are, if they already match the udev values, in the same way we preserve the inode.

That way all ACL's are preserved, and hooking into "change" to set permissions will still work.

Does the following work for you?

Care to check the changes in the udev git tree?

Thanks,
Kay