On Thu, Jul 26, 2007 at 11:04:35PM -0700, David Miller wrote:
> From: David Miller <[EMAIL PROTECTED]>
> Date: Thu, 26 Jul 2007 19:07:51 -0700 (PDT)
>
> > Two ways to fix:
> >
> > 1) Make the PROC wrapper call ->unlocked_ioctl() or ->ioctl()
> > as a fallback of ->compat_ioctl is NULL.
> >
> > 2) Make proc_bus_pci_operations provide a .compat_ioctl method,
> > but then we'll need to audit the entire tree for cases like
> > this and make the same fix.
> >
> > Because it's easier to validate that all cases are covered,
> > I think #1 is the preferred fix.
>
> Here is my suggested fix.
>
> It is important to only provide the compat_ioctl method
> if the downstream de->proc_fops does too, otherwise this
> utterly confuses the logic in fs/compat_ioctl.c and we
> end up doing the right thing.
Indeed, my patch broke cases where .compat_ioctl was not supplied and
ioctl was done with compat_sys_ioctl().
> To be honest, the other unlocked_ioctl fallback scheme in
> this file should be scrutinized for similar problems.
I checked on test module all (3 + 3 + 1) x 2 combinations of available
methods and ioctl/compat_ioctl accesses. Regression were only in
compat_sys_ioctl(2) part. And your patch fixes all of them.
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -386,6 +386,19 @@ static const struct file_operations proc_reg_file_ops = {
> .release = proc_reg_release,
> };
>
> +#ifdef CONFIG_COMPAT
> +static const struct file_operations proc_reg_file_ops_no_compat = {
> + .llseek = proc_reg_llseek,
> + .read = proc_reg_read,
> + .write = proc_reg_write,
> + .poll = proc_reg_poll,
> + .unlocked_ioctl = proc_reg_unlocked_ioctl,
> + .mmap = proc_reg_mmap,
> + .open = proc_reg_open,
> + .release = proc_reg_release,
> +};
> +#endif
> +
> struct inode *proc_get_inode(struct super_block *sb, unsigned int ino,
> struct proc_dir_entry *de)
> {
> @@ -413,8 +426,15 @@ struct inode *proc_get_inode(struct super_block *sb,
> unsigned int ino,
> if (de->proc_iops)
> inode->i_op = de->proc_iops;
> if (de->proc_fops) {
> - if (S_ISREG(inode->i_mode))
> - inode->i_fop = &proc_reg_file_ops;
> + if (S_ISREG(inode->i_mode)) {
> +#ifdef CONFIG_COMPAT
> + if (!de->proc_fops->compat_ioctl)
> + inode->i_fop =
> + &proc_reg_file_ops_no_compat;
> + else
> +#endif
> + inode->i_fop = &proc_reg_file_ops;
> + }
> else
> inode->i_fop = de->proc_fops;
> }
Looks good, thanks David.
-
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/
