Re: ioctl32_unregister_conversion & modules

Re: ioctl32_unregister_conversion & modules

[Arnd Bergmann]

Pavel Machek wrote:

> ...what is the problem?
> 
> It seems that function pointers into modules do not need any special
> treatmeant [I *know* there was talk about this on l-k; but I can't
> find anything in Documentation/]:
> 
>                 if (!capable(CAP_SYS_ADMIN))
>                         return -EACCES;
>                 if (disk->fops->ioctl) {
>                         ret = disk->fops->ioctl(inode, file, cmd, arg);
>                         if (ret != -EINVAL)
>                                 return ret;
>                 }

This is protected against unload by the reference counting done in 
open()/release(). ->ioctl() can be called only for open devices,
so you know the ioctl handler is not getting unloaded while it
is running.
 
> So... what's the problem with {un}register_ioctl32_conversion being
> called from module_init/module_exit? Drivers in the tree do it
> already...

The problem is that when the conversion handler is called, the reference
counting is only done for the module listed as ->owner in the
file operations. For example in the patch you submitted to add 
register_ioctl32_conversion() to drivers/serial/core.c I see nothing
stopping you from unloading core.ko while the handler is running
on a device owned by drivers/char/cyclades.c or any other serial driver.
It does not even have to be run on a serial driver, a user might try
to do ioctl(TIOCGSERIAL, ...) on a regular file...

        Arnd <><

Re: ioctl32_unregister_conversion & modules

[Pavel Machek]

Hi!

> > ...what is the problem?
> > 
> > It seems that function pointers into modules do not need any special
> > treatmeant [I *know* there was talk about this on l-k; but I can't
> > find anything in Documentation/]:
> > 
> >                 if (!capable(CAP_SYS_ADMIN))
> >                         return -EACCES;
> >                 if (disk->fops->ioctl) {
> >                         ret = disk->fops->ioctl(inode, file, cmd, arg);
> >                         if (ret != -EINVAL)
> >                                 return ret;
> >                 }
> 
> This is protected against unload by the reference counting done in 
> open()/release(). ->ioctl() can be called only for open devices,
> so you know the ioctl handler is not getting unloaded while it
> is running.
>  
> > So... what's the problem with {un}register_ioctl32_conversion being
> > called from module_init/module_exit? Drivers in the tree do it
> > already...
> 
> The problem is that when the conversion handler is called, the reference
> counting is only done for the module listed as ->owner in the
> file operations. For example in the patch you submitted to add 
> register_ioctl32_conversion() to drivers/serial/core.c I see nothing
> stopping you from unloading core.ko while the handler is running
> on a device owned by drivers/char/cyclades.c or any other serial driver.
> It does not even have to be run on a serial driver, a user might try
> to do ioctl(TIOCGSERIAL, ...) on a regular file...

Oh.... Yep, that's pretty clear.

So what you are saying is that all existing
register_ioctl32_conversion-s that are in unloadable module are
broken. Ouch.

Fixing that would require resgister_ioctl32_conversion() to have 3-rd
parameter "this module" and some magic inside fs/compat_ioctl.c,
right?

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: ioctl32_unregister_conversion & modules

[David S. Miller]

On Fri, 2003-05-09 at 08:24, Pavel Machek wrote:
> Fixing that would require resgister_ioctl32_conversion() to have 3-rd
> parameter "this module" and some magic inside fs/compat_ioctl.c,
> right?

That would do it.  I would suggest seperating out the static translation
handlers and the dynamically registered ones.  Now that you're adding
in module owner info, the translation tables are going to start bloating
up like crazy.

I'm still upset about going from 32-bit --> 64-bit entries on
sparc64 :-(

-- 
David S. Miller <davem@redhat.com>

Re: ioctl32_unregister_conversion & modules

[Pavel Machek]

Hi!

> > Fixing that would require resgister_ioctl32_conversion() to have 3-rd
> > parameter "this module" and some magic inside fs/compat_ioctl.c,
> > right?
> 
> That would do it.  I would suggest seperating out the static translation
> handlers and the dynamically registered ones.  Now that you're adding
> in module owner info, the translation tables are going to start bloating
> up like crazy.
> 
> I'm still upset about going from 32-bit --> 64-bit entries on
> sparc64 :-(

So... Do you think moving common handlers from arch/*/ioctl32.c into
fs/compat_ioctl.c would do the trick?
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: ioctl32_unregister_conversion & modules

[David S. Miller]

   From: Pavel Machek <pavel@ucw.cz>
   Date: Fri, 9 May 2003 19:11:55 +0200
   
   So... Do you think moving common handlers from arch/*/ioctl32.c into
   fs/compat_ioctl.c would do the trick?

Yes.

Re: ioctl32_unregister_conversion & modules

[Arnd Bergmann]

On Friday 09 May 2003 17:24, Pavel Machek wrote:

> Fixing that would require resgister_ioctl32_conversion() to have 3-rd
> parameter "this module" and some magic inside fs/compat_ioctl.c,
> right?

The code that is currently using register_ioctl32_conversion() does
not have to be changed if we use 

extern int 
__register_ioctl_conversion(int, ioctl_trans_handler_t, struct module*);

static inline int 
register_ioctl_conversion(int cmd, ioctl_trans_handler_t h)
{
	return __register_ioctl_conversion(cmd, h, THIS_MODULE);
}

/* maybe also: */
static inline int 
register_compatible_ioctl(int cmd)
{
	return __register_ioctl_conversion(cmd, NULL, NULL);
}

register_compatible_ioctl() is not strictly needed, but it will avoid
doing the unnecessary try_module_get() when there is no handler.

	Arnd <><

ioctl32_unregister_conversion & modules

[Pavel Machek]

Hi!

...what is the problem?

It seems that function pointers into modules do not need any special
treatmeant [I *know* there was talk about this on l-k; but I can't
find anything in Documentation/]:

                if (!capable(CAP_SYS_ADMIN))
                        return -EACCES;
                if (disk->fops->ioctl) {
                        ret = disk->fops->ioctl(inode, file, cmd, arg);
                        if (ret != -EINVAL)
                                return ret;
                }

So... what's the problem with {un}register_ioctl32_conversion being
called from module_init/module_exit? Drivers in the tree do it
already...
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]