* Re: ioctl32_unregister_conversion & modules
[not found] <20030509100039$6904@gated-at.bofh.it>
@ 2003-05-09 12:10 ` Arnd Bergmann
2003-05-09 15:24 ` Pavel Machek
0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2003-05-09 12:10 UTC (permalink / raw)
To: Pavel Machek, linux-kernel
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 <><
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ioctl32_unregister_conversion & modules
2003-05-09 12:10 ` ioctl32_unregister_conversion & modules Arnd Bergmann
@ 2003-05-09 15:24 ` Pavel Machek
2003-05-09 16:13 ` David S. Miller
2003-05-09 19:30 ` Arnd Bergmann
0 siblings, 2 replies; 7+ messages in thread
From: Pavel Machek @ 2003-05-09 15:24 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-kernel
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?]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ioctl32_unregister_conversion & modules
2003-05-09 15:24 ` Pavel Machek
@ 2003-05-09 16:13 ` David S. Miller
2003-05-09 17:11 ` Pavel Machek
2003-05-09 19:30 ` Arnd Bergmann
1 sibling, 1 reply; 7+ messages in thread
From: David S. Miller @ 2003-05-09 16:13 UTC (permalink / raw)
To: Pavel Machek; +Cc: Arnd Bergmann, linux-kernel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ioctl32_unregister_conversion & modules
2003-05-09 16:13 ` David S. Miller
@ 2003-05-09 17:11 ` Pavel Machek
2003-05-09 17:16 ` David S. Miller
0 siblings, 1 reply; 7+ messages in thread
From: Pavel Machek @ 2003-05-09 17:11 UTC (permalink / raw)
To: David S. Miller; +Cc: Arnd Bergmann, linux-kernel
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?]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ioctl32_unregister_conversion & modules
2003-05-09 15:24 ` Pavel Machek
2003-05-09 16:13 ` David S. Miller
@ 2003-05-09 19:30 ` Arnd Bergmann
1 sibling, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2003-05-09 19:30 UTC (permalink / raw)
To: Pavel Machek; +Cc: linux-kernel
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 <><
^ permalink raw reply [flat|nested] 7+ messages in thread
* ioctl32_unregister_conversion & modules
@ 2003-05-09 9:32 Pavel Machek
0 siblings, 0 replies; 7+ messages in thread
From: Pavel Machek @ 2003-05-09 9:32 UTC (permalink / raw)
To: davem, kernel list, ak
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?]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2003-05-09 19:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20030509100039$6904@gated-at.bofh.it>
2003-05-09 12:10 ` ioctl32_unregister_conversion & modules Arnd Bergmann
2003-05-09 15:24 ` Pavel Machek
2003-05-09 16:13 ` David S. Miller
2003-05-09 17:11 ` Pavel Machek
2003-05-09 17:16 ` David S. Miller
2003-05-09 19:30 ` Arnd Bergmann
2003-05-09 9:32 Pavel Machek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).