TUNSETIFF: data not copied back to userspace

TUNSETIFF: data not copied back to userspace

[Michael Tautschnig]

Hello!

Thanks for the information regarding userland/kernelspace - I think I 
understood ...

The reason of my question were problems using the tun/tap interface - I've 
got an application (namely l2tpns) demanding the ifreq-structure being 
copied back to userspace - which AFAIK doesn't happen in case of the 
TUNSETIFF-ioctl. Is this a bug or are there any reasons why this shouldn't 
happen in dev_ifsioc ?

Thanks,
Michael

Re: TUNSETIFF: data not copied back to userspace

[Eric Brower]

Take a look at the dev_ifsioc() function in ioctl32.c-- it does not
copy the data back to userspace except for SIOCSIFMAP, and judging
from the strict definition of TUNSETIFF it should not be required to
do so.

I think if this is functionality that is required (I see no TUNGETIFF,
but I don't play with this stuff enough to know) the directionality of
the ioctl definition should be _IOWR rather than _IOR; in any case,
the "argument" field should  not be "int" but rather "struct ifreq*"
in if_tun.h.  Likely, neither will change for historical reasons....

E


On Mon, 14 Feb 2005 20:41:33 +0100 (CET), Michael Tautschnig
<michael.tautschnig@zt-consulting.com> wrote:
> Hello!
> 
> Thanks for the information regarding userland/kernelspace - I think I
> understood ...
> 
> The reason of my question were problems using the tun/tap interface - I've
> got an application (namely l2tpns) demanding the ifreq-structure being
> copied back to userspace - which AFAIK doesn't happen in case of the
> TUNSETIFF-ioctl. Is this a bug or are there any reasons why this shouldn't
> happen in dev_ifsioc ?
> 
> Thanks,
> Michael
> 
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
E

Re: TUNSETIFF: data not copied back to userspace

[Michael Tautschnig]

> Take a look at the dev_ifsioc() function in ioctl32.c-- it does not
> copy the data back to userspace except for SIOCSIFMAP, and judging
> from the strict definition of TUNSETIFF it should not be required to
> do so.
IMHO this isn't true -

    switch (cmd) {
     case SIOCGIFFLAGS:
     case SIOCGIFMETRIC:
     case SIOCGIFMTU:
     case SIOCGIFMEM:
     case SIOCGIFHWADDR:
     case SIOCGIFINDEX:
     case SIOCGIFADDR:
     case SIOCGIFBRDADDR:
     case SIOCGIFDSTADDR:
     case SIOCGIFNETMASK:
     case SIOCGIFTXQLEN:
       if (copy_to_user(uifr32, &ifr, sizeof(*uifr32)))
         return -EFAULT;
       break;
     case SIOCGIFMAP:

- the (modified) struct is copied in all cases listed above. But what do 
you mean by "the strict definition of TUNSETIFF"?


>
> I think if this is functionality that is required (I see no TUNGETIFF,
> but I don't play with this stuff enough to know) the directionality of
> the ioctl definition should be _IOWR rather than _IOR; in any case,
> the "argument" field should  not be "int" but rather "struct ifreq*"
> in if_tun.h.  Likely, neither will change for historical reasons....
>

The idea behind all that is, that the application needs (?) to know the 
name of the device (tunX) to do further ioctls on that device! Of course, 
it could be hardcoded, but that doesn't seem as nice ...

Why should be be _IOWR there?

Thanks,
Michael

Re: TUNSETIFF: data not copied back to userspace

[David S. Miller]

On Mon, 14 Feb 2005 23:59:32 +0100 (CET)
Michael Tautschnig <michael.tautschnig@zt-consulting.com> wrote:

>     switch (cmd) {
>      case SIOCGIFFLAGS:
>      case SIOCGIFMETRIC:
>      case SIOCGIFMTU:
>      case SIOCGIFMEM:
>      case SIOCGIFHWADDR:
>      case SIOCGIFINDEX:
>      case SIOCGIFADDR:
>      case SIOCGIFBRDADDR:
>      case SIOCGIFDSTADDR:
>      case SIOCGIFNETMASK:
>      case SIOCGIFTXQLEN:
>        if (copy_to_user(uifr32, &ifr, sizeof(*uifr32)))
>          return -EFAULT;
>        break;
>      case SIOCGIFMAP:
> 
> - the (modified) struct is copied in all cases listed above. But what do 
> you mean by "the strict definition of TUNSETIFF"?

So add "case TUNSETIFF" to the switch statement above.
You discovered the fix for your own bug, so why not implement
it and submit the resulting patch here? :-)

> Why should be be _IOWR there?

ioctl's are mostly defined in with a set of macros that describe
whether data goes in or out (or both) from/to userspace.  The
_IORW macros set the direction to indicate that data goes both
in and out.

These macros are defined in include/asm/ioctl.h

include/linux/if_tun.h defines TUNSETIFF using the _IOW() macro.
Which indicates that data only comes in from userspace.  This definition
is incorrect, it should use _IORW() as Eric stated, but it can't be changed
because  this ioctl value is hardcoded into many existing applications which
would break if we "fixed" this.

Re: TUNSETIFF: data not copied back to userspace

[Michael Tautschnig]

[...]
>
> So add "case TUNSETIFF" to the switch statement above.
> You discovered the fix for your own bug, so why not implement
> it and submit the resulting patch here? :-)
It might have been intentionally left out - but here we go:


--- linux-2.6.10/fs/compat_ioctl.c      2005-02-15 00:09:53.969591354 +0100
+++ linux-2.6.10.patched/fs/compat_ioctl.c      2005-02-15 00:08:26.078059666 +0100
@@ -696,6 +696,7 @@
                 case SIOCGIFDSTADDR:
                 case SIOCGIFNETMASK:
                 case SIOCGIFTXQLEN:
+    		case TUNSETIFF:
                         if (copy_to_user(uifr32, &ifr, sizeof(*uifr32)))
                                 return -EFAULT;
                         break;

:-))

Actually, it did the trick - l2tpns works fine now.

[...]

>
> include/linux/if_tun.h defines TUNSETIFF using the _IOW() macro.
> Which indicates that data only comes in from userspace.  This definition
> is incorrect, it should use _IORW() as Eric stated, but it can't be changed
> because  this ioctl value is hardcoded into many existing applications which
> would break if we "fixed" this.
>

So the preferred behaviour would be: Add an interface-specification to 
l2tpns' config - or will the above "patch" ever be included in the 
official kernel?

Thanks for your help,
Michael

Re: TUNSETIFF: data not copied back to userspace

[David S. Miller]

On Tue, 15 Feb 2005 00:18:08 +0100 (CET)
Michael Tautschnig <michael.tautschnig@zt-consulting.com> wrote:

> So the preferred behaviour would be: Add an interface-specification to 
> l2tpns' config - or will the above "patch" ever be included in the 
> official kernel?

I put the patch into my tree just now, I'll push it upstream.