Re: udev change event

[Kay Sievers <kay.sievers@vrfy.org>]

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

On Thu, 2008-07-10 at 10:21 -0400, David Zeuthen wrote:
> On Thu, 2008-07-03 at 23:50 +0200, Kay Sievers wrote:
> > On Thu, Jul 3, 2008 at 21:48, Bill Nottingham <notting@redhat.com> wrote:
> > > Marco d'Itri (md@Linux.IT) said:
> > >> On Jul 03, Harald Hoyer <harald@redhat.com> wrote:
> > >>
> > >> > 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?
> 
> 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


[-- Attachment #2: change-event-perms-fix.patch --]
[-- Type: text/x-patch, Size: 3458 bytes --]

diff --git a/udev_node.c b/udev_node.c
index 0e59e2d..732cce4 100644
--- a/udev_node.c
+++ b/udev_node.c
@@ -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)