Skip to contentSkip to conversation

Fix type errors in inotify interfaces

PATCH In review Michael Kerrisk 4 participants 7 messages Nov 18, 2008 – Nov 19, 2008

Subsystems (2)
VFS ABI/API
lore
Share & tools
lore

Michael Kerrisk

author
#0 lore raw

PATCH 1/1 Fix type errors in inotify interfaces

Andrew,

Vegard reminded me of an issue with the inotify interface that I raised quite a while ago, offlist, with Robert; Robert acknowledged that things should be fixed, but then neither of us actually did anything :-{.

The problems lie in the types used for some inotify interfaces, both at the kernel level and at the glibc level. This mail addresses the kernel problem. I will follow up with some suggestions for glibc changes.

For the sys_inotify_rm_watch() interface, the type of the 'wd' argument is currently 'u32', it should be '__s32' . That is Robert's suggestion, and is consistent with the other declarations of watch descriptors in the kernel source, in particular, the inotify_event structure in include/linux/inotify.h:

struct inotify_event {

        __s32           wd;             /* watch descriptor */
        __u32           mask;           /* watch mask */
        __u32           cookie;         /* cookie to synchronize two events */
        __u32           len;            /* length (including nulls) of name */
        char            name[0];        /* stub for possible name */

};

The patch below makes the changes needed for inotify_rm_watch().

Thanks

Michael

fs/inotify_user.c +1 -1
index d367e9b..a71f764 100644
@@ -704,7 +704,7 @@ fput_and_out:
 	return ret;
 }
 
-asmlinkage long sys_inotify_rm_watch(int fd, u32 wd)
+asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd)
 {
 	struct file *filp;
 	struct inotify_device *dev;
include/linux/syscalls.h +1 -1
index d6ff145..36983a5 100644
@@ -550,7 +550,7 @@ asmlinkage long sys_inotify_init(void);
 asmlinkage long sys_inotify_init1(int flags);
 asmlinkage long sys_inotify_add_watch(int fd, const char __user *path,
 					u32 mask);
-asmlinkage long sys_inotify_rm_watch(int fd, u32 wd);
+asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd);
 
 asmlinkage long sys_spu_run(int fd, __u32 __user *unpc,
 				 __u32 __user *ustatus);
author
#1 lore raw

I hope Robert doesn't mind if I drop a piece from our old offlist mail conversation into this thread, for some background.

-------- Original Message --------
Subject: Re: Use of uint32_t in inotify APIs
Date: Tue, 20 Jun 2006 09:07:17 -0400
From: Robert Love <[email protected]>

On 6/20/06, Michael Kerrisk wrote:

Hi,

Quoted 5 lines

The use of uint32_t seems a little strange in at least some of the above. In particular, the watch descriptor returned by inotify_add_watch() is an "int", as is the "wd" field of the "inotify_event" structure, but in the prototype for inotify_rm_watch(), "wd" is uint32_t. Is there a reason for this, or is it just cruft?

The unsigned parameter in inotify_rm_watch() is wrong. I cannot think we that should be the case. But the problem lies not with glibc -- the function definition in the kernel is wrong, too. It should be fixed; everywhere else a watch descriptor is an __s32.

Quoted 3 lines

Following on from this, is it really necessary that "mask" is uint32_t in the prototype for inotify_rm_watch()? In most other glibc interfaces, bit masks are simply "int".

I think masks should be unsigned. This definitely makes sense.

In the kernel, most of the inotify data types are __u32.

[...]

Also the 'wd' in the event structure should be 'int32_t' -- the kernel uses the type '__s32' -- but perhaps glibc developers prefer to use a straight 'int' since the types are identical.

In that case, inotify_rm_watch()'s 'wd' should be changed to '__s32' in the kernel and 'int' in glibc. Otherwise, '__s32' in the kernel and 'int32_t' in glibc. Since this change will affect ABI, it needs to be made cautiously, but I don't see any issues.

Robert Love

author
#2 lore raw

Ulrich,

Below are the changes that I believe are required in glibc.

Thanks,

Michael

sysdeps/unix/sysv/linux/alpha/sys/inotify.h +2 -2
diff -ruN glibc-2.9/sysdeps/unix/sysv/linux/alpha/sys/inotify.h glibc-2.9-mod/sysdeps/unix/sysv/linux/alpha/sys/inotify.h
@@ -35,7 +35,7 @@
 /* Structure describing an inotify event.  */
 struct inotify_event
 {
-  int wd;		/* Watch descriptor.  */
+  int32_t wd;		/* Watch descriptor.  */
   uint32_t mask;	/* Watch mask.  */
   uint32_t cookie;	/* Cookie to synchronize two events.  */
   uint32_t len;		/* Length (including NULs) of name.  */

@@ -98,7 +98,7 @@
   __THROW;
 
 /* Remove the watch specified by WD from the inotify instance FD.  */
-extern int inotify_rm_watch (int __fd, uint32_t __wd) __THROW;
+extern int inotify_rm_watch (int __fd, int __wd) __THROW;
 
 __END_DECLS
sysdeps/unix/sysv/linux/sparc/sys/inotify.h +2 -2
diff -ruN glibc-2.9/sysdeps/unix/sysv/linux/sparc/sys/inotify.h glibc-2.9-mod/sysdeps/unix/sysv/linux/sparc/sys/inotify.h
@@ -35,7 +35,7 @@
 /* Structure describing an inotify event.  */
 struct inotify_event
 {
-  int wd;		/* Watch descriptor.  */
+  int32_t wd;		/* Watch descriptor.  */
   uint32_t mask;	/* Watch mask.  */
   uint32_t cookie;	/* Cookie to synchronize two events.  */
   uint32_t len;		/* Length (including NULs) of name.  */

@@ -98,7 +98,7 @@
   __THROW;
 
 /* Remove the watch specified by WD from the inotify instance FD.  */
-extern int inotify_rm_watch (int __fd, uint32_t __wd) __THROW;
+extern int inotify_rm_watch (int __fd, int __wd) __THROW;
 
 __END_DECLS
sysdeps/unix/sysv/linux/sys/inotify.h +2 -2
diff -ruN glibc-2.9/sysdeps/unix/sysv/linux/sys/inotify.h glibc-2.9-mod/sysdeps/unix/sysv/linux/sys/inotify.h
@@ -35,7 +35,7 @@
 /* Structure describing an inotify event.  */
 struct inotify_event
 {
-  int wd;		/* Watch descriptor.  */
+  int32_t wd;		/* Watch descriptor.  */
   uint32_t mask;	/* Watch mask.  */
   uint32_t cookie;	/* Cookie to synchronize two events.  */
   uint32_t len;		/* Length (including NULs) of name.  */

@@ -98,7 +98,7 @@
   __THROW;
 
 /* Remove the watch specified by WD from the inotify instance FD.  */
-extern int inotify_rm_watch (int __fd, uint32_t __wd) __THROW;
+extern int inotify_rm_watch (int __fd, int __wd) __THROW;
 
 __END_DECLS
#3 lore raw
In reply to Michael Kerrisk
Quoted 51 lines · On Tue, 18 Nov 2008 15:18:21 -0500 Michael Kerrisk…

Andrew,

Vegard reminded me of an issue with the inotify interface that I raised quite a while ago, offlist, with Robert; Robert acknowledged that things should be fixed, but then neither of us actually did anything :-{.

The problems lie in the types used for some inotify interfaces, both at the kernel level and at the glibc level. This mail addresses the kernel problem. I will follow up with some suggestions for glibc changes.

For the sys_inotify_rm_watch() interface, the type of the 'wd' argument is currently 'u32', it should be '__s32' . That is Robert's suggestion, and is consistent with the other declarations of watch descriptors in the kernel source, in particular, the inotify_event structure in include/linux/inotify.h:

struct inotify_event {

        __s32           wd;             /* watch descriptor */
        __u32           mask;           /* watch mask */
        __u32           cookie;         /* cookie to synchronize two events */
        __u32           len;            /* length (including nulls) of name */
        char            name[0];        /* stub for possible name */

};

The patch below makes the changes needed for inotify_rm_watch().

Thanks

Michael

fs/inotify_user.c +1 -1
index d367e9b..a71f764 100644
@@ -704,7 +704,7 @@ fput_and_out:
 	return ret;
 }
 
-asmlinkage long sys_inotify_rm_watch(int fd, u32 wd)
+asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd)
 {
 	struct file *filp;
 	struct inotify_device *dev;
include/linux/syscalls.h +0 -0
index d6ff145..36983a5 100644

include/linux/syscalls.h asmlinkage long sys_inotify_init(void); · line 556

Quoted 8 lines
include/linux/syscalls.h +1 -1
@@ -550,7 +550,7 @@ asmlinkage long sys_inotify_init(void);
 asmlinkage long sys_inotify_init1(int flags);
 asmlinkage long sys_inotify_add_watch(int fd, const char __user *path,
 					u32 mask);
-asmlinkage long sys_inotify_rm_watch(int fd, u32 wd);
+asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd);
 
 asmlinkage long sys_spu_run(int fd, __u32 __user *unpc,
 				 __u32 __user *ustatus);

hrm. I see no sane reason why a watch descriptor should take on negative values, so in some ways the u32 was logical. (Ditto file descriptors, but I don't think I want to change that ;))

otoh, the system call via which one _obtains_ watch descriptors most certainly wants to return -ve nunmbers, to signify errors.

All too hard. I think I'll stop thinking about it and merge the patch ;)

author
#4 lore raw

(Would be nice to see an Aacked-by from Robert or John on this patch.)

Quoted 59 lines · Andrew Morton
Quoted 57 lines · On Tue, 18 Nov 2008 15:18:21 -0500 Michael Kerrisk…

Andrew,

Vegard reminded me of an issue with the inotify interface that I raised quite a while ago, offlist, with Robert; Robert acknowledged that things should be fixed, but then neither of us actually did anything :-{.

The problems lie in the types used for some inotify interfaces, both at the kernel level and at the glibc level. This mail addresses the kernel problem. I will follow up with some suggestions for glibc changes.

For the sys_inotify_rm_watch() interface, the type of the 'wd' argument is currently 'u32', it should be '__s32' . That is Robert's suggestion, and is consistent with the other declarations of watch descriptors in the kernel source, in particular, the inotify_event structure in include/linux/inotify.h:

struct inotify_event {

        __s32           wd;             /* watch descriptor */
        __u32           mask;           /* watch mask */
        __u32           cookie;         /* cookie to synchronize two events */
        __u32           len;            /* length (including nulls) of name */
        char            name[0];        /* stub for possible name */

};

The patch below makes the changes needed for inotify_rm_watch().

Thanks

Michael

fs/inotify_user.c +1 -1
index d367e9b..a71f764 100644
@@ -704,7 +704,7 @@ fput_and_out:
      return ret;
 }
 
-asmlinkage long sys_inotify_rm_watch(int fd, u32 wd)
+asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd)
 {
      struct file *filp;
      struct inotify_device *dev;
include/linux/syscalls.h +1 -1
index d6ff145..36983a5 100644
@@ -550,7 +550,7 @@ asmlinkage long sys_inotify_init(void);
 asmlinkage long sys_inotify_init1(int flags);
 asmlinkage long sys_inotify_add_watch(int fd, const char __user *path,
                                      u32 mask);
-asmlinkage long sys_inotify_rm_watch(int fd, u32 wd);
+asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd);
Quoted 3 lines

include/linux/syscalls.h asmlinkage long sys_inotify_init(void); · line 556

Quoted 2 lines
include/linux/syscalls.h +0 -0
 asmlinkage long sys_spu_run(int fd, __u32 __user *unpc,
                               __u32 __user *ustatus);

Yes -- there is no sane reason for a negative watch descriptor to inotify_rm_watch(); this change is mainly about consistency. (The glibc change is probably more important.)

Quoted 4 lines

otoh, the system call via which one _obtains_ watch descriptors most certainly wants to return -ve nunmbers, to signify errors.

All too hard. I think I'll stop thinking about it and merge the patch ;)

Thanks.

Michael

Signature · 5 lines
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html
Acked-by
#5 lore raw
Quoted 1 line · Michael Kerrisk

(Would be nice to see an Aacked-by from Robert or John on this patch.)

I hesitate because I haven't looked at glibc in ages to see what it and other userland consumers are doing with inotify.

But my original analysis still seems correct, and whatever we do we currently have inconsistencies, so this looks good to me.

Acked-by Robert Love <[email protected]>

Robert

Acked-by
#6 lore raw
In reply to Michael Kerrisk
Quoted 63 lines · Michael Kerrisk

(Would be nice to see an Aacked-by from Robert or John on this patch.)

Quoted 59 lines · Andrew Morton
Quoted 57 lines · On Tue, 18 Nov 2008 15:18:21 -0500 Michael Kerrisk…

Andrew,

Vegard reminded me of an issue with the inotify interface that I raised quite a while ago, offlist, with Robert; Robert acknowledged that things should be fixed, but then neither of us actually did anything :-{.

The problems lie in the types used for some inotify interfaces, both at the kernel level and at the glibc level. This mail addresses the kernel problem. I will follow up with some suggestions for glibc changes.

For the sys_inotify_rm_watch() interface, the type of the 'wd' argument is currently 'u32', it should be '__s32' . That is Robert's suggestion, and is consistent with the other declarations of watch descriptors in the kernel source, in particular, the inotify_event structure in include/linux/inotify.h:

struct inotify_event {

        __s32           wd;             /* watch descriptor */
        __u32           mask;           /* watch mask */
        __u32           cookie;         /* cookie to synchronize two events */
        __u32           len;            /* length (including nulls) of name */
        char            name[0];        /* stub for possible name */

};

The patch below makes the changes needed for inotify_rm_watch().

Thanks

Michael

fs/inotify_user.c +1 -1
index d367e9b..a71f764 100644
@@ -704,7 +704,7 @@ fput_and_out:
      return ret;
 }
 
-asmlinkage long sys_inotify_rm_watch(int fd, u32 wd)
+asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd)
 {
      struct file *filp;
      struct inotify_device *dev;
include/linux/syscalls.h +1 -1
index d6ff145..36983a5 100644
@@ -550,7 +550,7 @@ asmlinkage long sys_inotify_init(void);
 asmlinkage long sys_inotify_init1(int flags);
 asmlinkage long sys_inotify_add_watch(int fd, const char __user *path,
                                      u32 mask);
-asmlinkage long sys_inotify_rm_watch(int fd, u32 wd);
+asmlinkage long sys_inotify_rm_watch(int fd, __s32 wd);
Quoted 13 lines
Quoted 3 lines

include/linux/syscalls.h asmlinkage long sys_inotify_init(void); · line 556

Quoted 2 lines
include/linux/syscalls.h +0 -0
 asmlinkage long sys_spu_run(int fd, __u32 __user *unpc,
                               __u32 __user *ustatus);

Yes -- there is no sane reason for a negative watch descriptor to inotify_rm_watch(); this change is mainly about consistency. (The glibc change is probably more important.)

Quoted 4 lines

otoh, the system call via which one _obtains_ watch descriptors most certainly wants to return -ve nunmbers, to signify errors.

All too hard. I think I'll stop thinking about it and merge the patch ;)

Seems sane enough.

Acked-by John McCutchan <[email protected]>
Signature · 1 line
John McCutchan <[email protected]>