udev change event

udev change event

[Harald Hoyer]

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

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?

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3636 bytes --]

Re: udev change event

[Marco d'Itri]

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

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.

-- 
ciao,
Marco

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: udev change event

[Bill Nottingham]

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?

Bill

Re: udev change event

[Kay Sievers]

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?

Kay

Re: udev change event

[Marco d'Itri]

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

On Jul 03, Bill Nottingham <notting@redhat.com> wrote:

> > 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/ .

-- 
ciao,
Marco

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: udev change event

[Harald Hoyer]

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

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?


https://bugzilla.redhat.com/show_bug.cgi?id=451320

for the reference

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/x-pkcs7-signature, Size: 3636 bytes --]

Re: udev change event

[Bill Nottingham]

Marco d'Itri (md@Linux.IT) said: 
> On Jul 03, Bill Nottingham <notting@redhat.com> wrote:
> 
> > > 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

Re: udev change event

[Marco d'Itri]

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

On Jul 07, Bill Nottingham <notting@redhat.com> wrote:

> 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.

-- 
ciao,
Marco

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: udev change event

[Bill Nottingham]

Marco d'Itri (md@Linux.IT) said: 
> > 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

Re: udev change event

[Marco d'Itri]

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

On Jul 07, Bill Nottingham <notting@redhat.com> wrote:

> - 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?

-- 
ciao,
Marco

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: udev change event

[Bill Nottingham]

Marco d'Itri (md@Linux.IT) said: 
> On Jul 07, Bill Nottingham <notting@redhat.com> wrote:
> 
> > - 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

Re: udev change event

[David Zeuthen]

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.

      David

Re: udev change event

[Kay Sievers]

[-- 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)

Re: udev change event

[Kay Sievers]

On Thu, Jul 10, 2008 at 20:44, Kay Sievers <kay.sievers@vrfy.org> wrote:
> 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?

Care to check the changes in the udev git tree?

Thanks,
Kay