Re: /proc/bus/pci IOCTL breakage

[Alexey Dobriyan]

On Thu, Jul 26, 2007 at 11:04:35PM -0700, David Miller wrote:
> From: David Miller <davem@xxxxxxxxxxxxx>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/