From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kay Sievers Date: Thu, 10 Jul 2008 18:44:51 +0000 Subject: Re: udev change event Message-Id: <1215715491.24293.14.camel@linux.site> MIME-Version: 1 Content-Type: multipart/mixed; boundary="=-mWryz35E4NKUFpf7oa2S" List-Id: References: <486CBF8C.1090406@redhat.com> In-Reply-To: <486CBF8C.1090406@redhat.com> To: linux-hotplug@vger.kernel.org --=-mWryz35E4NKUFpf7oa2S Content-Type: text/plain Content-Transfer-Encoding: 7bit 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 wrote: > > > Marco d'Itri (md@Linux.IT) said: > > >> On Jul 03, Harald Hoyer 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 --=-mWryz35E4NKUFpf7oa2S Content-Disposition: attachment; filename=change-event-perms-fix.patch Content-Type: text/x-patch; name=change-event-perms-fix.patch; charset=utf-8 Content-Transfer-Encoding: 7bit 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) --=-mWryz35E4NKUFpf7oa2S--