(forgot to add lkml to Cc)
On Mon, Jun 14, 2010 at 01:02:52PM +0200, Johannes Stezenbach wrote:
> Hi,
>
> in commit 19ba0559 the FIGETBSZ (and FS_IOC_FIEMAP) ioctl was moved
> from file_ioctl() to do_vfs_ioctl(), so it also works for directories.
> The problem I'm seeing is that FIGETBSZ is defined as _IO(0x00,2)
> which is simply 2. so there is some potential for conflicts
> with character devices which do not use the _IO macros for numbering
> their ioctls.
> Just doing a web search for "FIGETBSZ ioctl conflict" shows
> that a few people already ran into this problem.
>
> Would you mind adding a check for S_ISDIR | S_ISREG,
> or maybe !S_ISCHR?
>
>
> Thanks,
> Johannes
On Mon, Jun 14, 2010 at 02:36:52PM +0200, Johannes Stezenbach wrote:
> (forgot to add lkml to Cc)
>
> On Mon, Jun 14, 2010 at 01:02:52PM +0200, Johannes Stezenbach wrote:
> > Hi,
> >
> > in commit 19ba0559 the FIGETBSZ (and FS_IOC_FIEMAP) ioctl was moved
> > from file_ioctl() to do_vfs_ioctl(), so it also works for directories.
> > The problem I'm seeing is that FIGETBSZ is defined as _IO(0x00,2)
> > which is simply 2. so there is some potential for conflicts
> > with character devices which do not use the _IO macros for numbering
> > their ioctls.
> > Just doing a web search for "FIGETBSZ ioctl conflict" shows
> > that a few people already ran into this problem.
> >
> > Would you mind adding a check for S_ISDIR | S_ISREG,
> > or maybe !S_ISCHR?
Yeah, I think the right thing to do here is a check for
S_ISDIR | S_ISREG | S_ISLNK.
- Ted
FIGETBSZ has an ioctl number of _IO(0x00,2) == 2, which can conflict
with device driver ioctls. Let's avoid the potential for problems by
only honoring the ioctl number for files where this ioctl is likely
going to be useful: regular files, directories, and symlinks.
Thanks to Johannes Stezenbach for pointing this consequence of commit
19ba0559.
Signed-off-by: "Theodore Ts'o" <[email protected]>
Cc: Al Viro <[email protected]>
Cc: "Aneesh Kumar K.V" <[email protected]>
---
fs/compat_ioctl.c | 7 ++++++-
fs/ioctl.c | 5 ++++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 641640d..81d646b 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1715,8 +1715,13 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
goto out_fput;
#endif
- case FIBMAP:
case FIGETBSZ:
+ if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode) ||
+ S_ISLNK(filp->f_path.dentry->d_inode->i_mode))
+ break;
+ /*FALL THROUGH */
+
+ case FIBMAP:
case FIONREAD:
if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
break;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 2d140a7..5c61d69 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -597,7 +597,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
{
struct inode *inode = filp->f_path.dentry->d_inode;
int __user *p = (int __user *)arg;
- return put_user(inode->i_sb->s_blocksize, p);
+
+ if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
+ S_ISLNK(inode->i_mode))
+ return put_user(inode->i_sb->s_blocksize, p);
}
default:
--
1.7.0.4
On Mon, Jun 14, 2010 at 10:05:10AM -0400, Theodore Ts'o wrote:
> FIGETBSZ has an ioctl number of _IO(0x00,2) == 2, which can conflict
> with device driver ioctls. Let's avoid the potential for problems by
> only honoring the ioctl number for files where this ioctl is likely
> going to be useful: regular files, directories, and symlinks.
>
> Thanks to Johannes Stezenbach for pointing this consequence of commit
> 19ba0559.
ioctl operate on a file descriptor, so you never call them on
symbolic links.
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 2d140a7..5c61d69 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -597,7 +597,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
> {
> struct inode *inode = filp->f_path.dentry->d_inode;
> int __user *p = (int __user *)arg;
> - return put_user(inode->i_sb->s_blocksize, p);
> +
> + if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> + S_ISLNK(inode->i_mode))
> + return put_user(inode->i_sb->s_blocksize, p);
> }
>
> default:
A comment explaining why we fall through here for special files is
almost required. Without that the chance of breaking it during the
next random cleanup are far too high.
On Mon, Jun 14, 2010 at 10:07:30AM -0400, Christoph Hellwig wrote:
> On Mon, Jun 14, 2010 at 10:05:10AM -0400, Theodore Ts'o wrote:
> > FIGETBSZ has an ioctl number of _IO(0x00,2) == 2, which can conflict
> > with device driver ioctls. Let's avoid the potential for problems by
> > only honoring the ioctl number for files where this ioctl is likely
> > going to be useful: regular files, directories, and symlinks.
> >
> > Thanks to Johannes Stezenbach for pointing this consequence of commit
> > 19ba0559.
>
> ioctl operate on a file descriptor, so you never call them on
> symbolic links.
Oops, good point.
> A comment explaining why we fall through here for special files is
> almost required. Without that the chance of breaking it during the
> next random cleanup are far too high.
Sigh. I had fixed that, but I failed to save emacs buffer before
creating commit. Will resend with both fixes.
- Ted
FIGETBSZ has an ioctl number of _IO(0x00,2) == 2, which can conflict
with device driver ioctls. Let's avoid the potential for problems by
only honoring the ioctl number for files where this ioctl is likely
going to be useful: regular files, directories, and symlinks.
Thanks to Johannes Stezenbach for pointing this consequence of commit
19ba0559.
Signed-off-by: "Theodore Ts'o" <[email protected]>
Cc: Al Viro <[email protected]>
Cc: "Aneesh Kumar K.V" <[email protected]>
Cc: Johannes Stezenbach <[email protected]>
---
fs/compat_ioctl.c | 6 +++++-
fs/ioctl.c | 5 ++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 641640d..b8607fe 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1715,8 +1715,12 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
goto out_fput;
#endif
- case FIBMAP:
case FIGETBSZ:
+ if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode))
+ break;
+ /*FALL THROUGH */
+
+ case FIBMAP:
case FIONREAD:
if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
break;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 2d140a7..e578dab 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -597,7 +597,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
{
struct inode *inode = filp->f_path.dentry->d_inode;
int __user *p = (int __user *)arg;
- return put_user(inode->i_sb->s_blocksize, p);
+
+ if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))
+ return put_user(inode->i_sb->s_blocksize, p);
+ /* FALL THROUGH */
}
default:
--
1.7.0.4
FIGETBSZ has an ioctl number of _IO(0x00,2) == 2, which can conflict
with device driver ioctls. Let's avoid the potential for problems by
only honoring the ioctl number for files where this ioctl is likely
going to be useful: for regular files and directories
Thanks to Johannes Stezenbach for pointing this consequence of commit
19ba0559.
Signed-off-by: "Theodore Ts'o" <[email protected]>
Cc: Al Viro <[email protected]>
Cc: "Aneesh Kumar K.V" <[email protected]>
Cc: Johannes Stezenbach <[email protected]>
---
Fixed up commit description
fs/compat_ioctl.c | 6 +++++-
fs/ioctl.c | 5 ++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 641640d..b8607fe 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1715,8 +1715,12 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
goto out_fput;
#endif
- case FIBMAP:
case FIGETBSZ:
+ if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode))
+ break;
+ /*FALL THROUGH */
+
+ case FIBMAP:
case FIONREAD:
if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
break;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 2d140a7..e578dab 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -597,7 +597,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
{
struct inode *inode = filp->f_path.dentry->d_inode;
int __user *p = (int __user *)arg;
- return put_user(inode->i_sb->s_blocksize, p);
+
+ if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))
+ return put_user(inode->i_sb->s_blocksize, p);
+ /* FALL THROUGH */
}
default:
--
1.7.0.4
Vad happened with the patch below. It was ''signed off'' more than half
a year ago, and it is not in any of the latest kernel yet.
I ask, because I spend a week debugging the 'dvbloopback' driver, which
uses '2' as a private ioctl for a read command. Not easy to find.
Can a driver not have private ioctls?
Should this be fixed in the driver?, I have now RESERVED the value 2 in
the driver. But this does not
guarantee that someone invents FIGETBSZ_V2 and assigned the value 3 to it.
Regards
/Anders
FIGETBSZ has an ioctl number of _IO(0x00,2) == 2, which can conflict
with device driver ioctls. Let's avoid the potential for problems by
only honoring the ioctl number for files where this ioctl is likely
going to be useful: for regular files and directories
Thanks to Johannes Stezenbach for pointing this consequence of commit
19ba0559.
Signed-off-by: "Theodore Ts'o" <tytso@xxxxxxx>
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
Cc: Johannes Stezenbach <js@xxxxxxxxx>
---
Fixed up commit description
fs/compat_ioctl.c | 6 +++++-
fs/ioctl.c | 5 ++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 641640d..b8607fe 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1715,8 +1715,12 @@ asmlinkage long compat_sys_ioctl(unsigned int fd,
unsigned int cmd,
goto out_fput;
#endif
- case FIBMAP:
case FIGETBSZ:
+ if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode))
+ break;
+ /*FALL THROUGH */
+
+ case FIBMAP:
case FIONREAD:
if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
break;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 2d140a7..e578dab 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -597,7 +597,10 @@ int do_vfs_ioctl(struct file *filp, unsigned int
fd, unsigned int cmd,
{
struct inode *inode = filp->f_path.dentry->d_inode;
int __user *p = (int __user *)arg;
- return put_user(inode->i_sb->s_blocksize, p);
+
+ if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))
+ return put_user(inode->i_sb->s_blocksize, p);
+ /* FALL THROUGH */
}
default:
--
1.7.0.4
(restore Cc:s)
On Wed, Feb 09, 2011 at 09:13:50AM +0100, anders franzen wrote:
>
> Vad happened with the patch below. It was ''signed off'' more than
> half a year ago, and it is not in any of the latest kernel yet.
>
> I ask, because I spend a week debugging the 'dvbloopback' driver,
> which uses '2' as a private ioctl for a read command. Not easy to
> find.
>
> Can a driver not have private ioctls?
> Should this be fixed in the driver?, I have now RESERVED the value 2
> in the driver. But this does not
> guarantee that someone invents FIGETBSZ_V2 and assigned the value 3 to it.
>
> Regards
> /Anders
>
>
>
>
> FIGETBSZ has an ioctl number of _IO(0x00,2) == 2, which can conflict
> with device driver ioctls. Let's avoid the potential for problems by
> only honoring the ioctl number for files where this ioctl is likely
> going to be useful: for regular files and directories
>
> Thanks to Johannes Stezenbach for pointing this consequence of commit
> 19ba0559.
>
> Signed-off-by: "Theodore Ts'o" <tytso@xxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> Cc: Johannes Stezenbach <js@xxxxxxxxx>
> ---
>
> Fixed up commit description
>
> fs/compat_ioctl.c | 6 +++++-
> fs/ioctl.c | 5 ++++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index 641640d..b8607fe 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -1715,8 +1715,12 @@ asmlinkage long compat_sys_ioctl(unsigned int
> fd, unsigned int cmd,
> goto out_fput;
> #endif
> - case FIBMAP:
> case FIGETBSZ:
> + if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode))
> + break;
> + /*FALL THROUGH */
> +
> + case FIBMAP:
> case FIONREAD:
> if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
> break;
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 2d140a7..e578dab 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -597,7 +597,10 @@ int do_vfs_ioctl(struct file *filp, unsigned
> int fd, unsigned int cmd,
> {
> struct inode *inode = filp->f_path.dentry->d_inode;
> int __user *p = (int __user *)arg;
> - return put_user(inode->i_sb->s_blocksize, p);
> +
> + if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))
> + return put_user(inode->i_sb->s_blocksize, p);
> + /* FALL THROUGH */
> }
>
> default:
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>