[RFC] should we care of COMPAT mode in bridge ?

[RFC] should we care of COMPAT mode in bridge ?

[Eric Dumazet]

While trying Alexander Holler patch, I found a 32bit brctl program was
not able to work on a 64bit kernel. So I had to switch to another
machine for my tests.

brctl addbr mybridge
->
socket(PF_FILE, SOCK_STREAM, 0)         = 3
ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)

Should we care or not ?

Re: [RFC] should we care of COMPAT mode in bridge ?

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 06 Jun 2011 21:45:40 +0200

> While trying Alexander Holler patch, I found a 32bit brctl program was
> not able to work on a 64bit kernel. So I had to switch to another
> machine for my tests.
> 
> brctl addbr mybridge
> ->
> socket(PF_FILE, SOCK_STREAM, 0)         = 3
> ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)
> 
> Should we care or not ?

I think we should make an effort to fix this.

Re: [RFC] should we care of COMPAT mode in bridge ?

[Arnd Bergmann]

On Monday 06 June 2011 21:45:40 Eric Dumazet wrote:
> While trying Alexander Holler patch, I found a 32bit brctl program was
> not able to work on a 64bit kernel. So I had to switch to another
> machine for my tests.
> 
> brctl addbr mybridge
> ->
> socket(PF_FILE, SOCK_STREAM, 0)         = 3
> ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)
> 
> Should we care or not ?
> 

Generally, we try to make all ioctls work in compat mode. For the old
bridge ioctls, this is currently impossible because it uses SIOCPRIVATE
ioctls, but it would be easy to add an ndo_do_compat_ioctl. 

The solution that is documented in net/socket.c is to return 
an invalid version number for BRCTL_VERSION, in the hope that
brctl would switch to the newer interfaces. The new style bridge
ioctls (SIOCBRADDBR, SIOCBRADDIF, ...) are actually supposed to work,
but I've never tried that.

	Arnd

Re: [RFC] should we care of COMPAT mode in bridge ?

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 6 Jun 2011 22:06:28 +0200

> On Monday 06 June 2011 21:45:40 Eric Dumazet wrote:
>> While trying Alexander Holler patch, I found a 32bit brctl program was
>> not able to work on a 64bit kernel. So I had to switch to another
>> machine for my tests.
>> 
>> brctl addbr mybridge
>> ->
>> socket(PF_FILE, SOCK_STREAM, 0)         = 3
>> ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)
>> 
>> Should we care or not ?
>> 
> 
> Generally, we try to make all ioctls work in compat mode. For the old
> bridge ioctls, this is currently impossible because it uses SIOCPRIVATE
> ioctls, but it would be easy to add an ndo_do_compat_ioctl. 

Right, we need to funnel compat ioctls down to the device and add a
->ndo_compat_ioctl() or similar, if that is indeed feasible.

Re: [RFC] should we care of COMPAT mode in bridge ?

[Eric Dumazet]

Le lundi 06 juin 2011 à 22:06 +0200, Arnd Bergmann a écrit :
> On Monday 06 June 2011 21:45:40 Eric Dumazet wrote:
> > While trying Alexander Holler patch, I found a 32bit brctl program was
> > not able to work on a 64bit kernel. So I had to switch to another
> > machine for my tests.
> > 
> > brctl addbr mybridge
> > ->
> > socket(PF_FILE, SOCK_STREAM, 0)         = 3
> > ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)
> > 
> > Should we care or not ?
> > 
> 
> Generally, we try to make all ioctls work in compat mode. For the old
> bridge ioctls, this is currently impossible because it uses SIOCPRIVATE
> ioctls, but it would be easy to add an ndo_do_compat_ioctl. 
> 
> The solution that is documented in net/socket.c is to return 
> an invalid version number for BRCTL_VERSION, in the hope that
> brctl would switch to the newer interfaces. The new style bridge
> ioctls (SIOCBRADDBR, SIOCBRADDIF, ...) are actually supposed to work,
> but I've never tried that.
> 


I see... problem is brctl does :

int br_add_bridge(const char *brname)
{
        int ret;

#ifdef SIOCBRADDBR
        ret = ioctl(br_socket_fd, SIOCBRADDBR, brname);
        if (ret < 0)
#endif
        {
                char _br[IFNAMSIZ];
                unsigned long arg[3]
                        = { BRCTL_ADD_BRIDGE, (unsigned long) _br };

                strncpy(_br, brname, IFNAMSIZ);
                ret = ioctl(br_socket_fd, SIOCSIFBR, arg);
        }

        return ret < 0 ? errno : 0;
}


So for an old binary, compiled at the time SIOCBRADDBR wasnt there (in
include file I mean), we ended doing :

        {
                char _br[IFNAMSIZ];
                unsigned long arg[3]
                        = { BRCTL_ADD_BRIDGE, (unsigned long) _br };

                strncpy(_br, brname, IFNAMSIZ);
                ret = ioctl(br_socket_fd, SIOCSIFBR, arg);
        }

And this breaks on 64bit kernel

I guess we shall add some logic in kernel to support SIOCSIFBR
afterall ;)

Thanks !

BTW: I confirm that compiling an up2date 32bit brctl with an up2date
include files is OK on 64bit kernel.

Re: [RFC] should we care of COMPAT mode in bridge ?

[Arnd Bergmann]

On Monday 06 June 2011 22:09:58 David Miller wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Mon, 6 Jun 2011 22:06:28 +0200
> 
> > On Monday 06 June 2011 21:45:40 Eric Dumazet wrote:
> >> While trying Alexander Holler patch, I found a 32bit brctl program was
> >> not able to work on a 64bit kernel. So I had to switch to another
> >> machine for my tests.
> >> 
> >> brctl addbr mybridge
> >> ->
> >> socket(PF_FILE, SOCK_STREAM, 0)         = 3
> >> ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)
> >> 
> >> Should we care or not ?
> >> 
> > 
> > Generally, we try to make all ioctls work in compat mode. For the old
> > bridge ioctls, this is currently impossible because it uses SIOCPRIVATE
> > ioctls, but it would be easy to add an ndo_do_compat_ioctl. 
> 
> Right, we need to funnel compat ioctls down to the device and add a
> ->ndo_compat_ioctl() or similar, if that is indeed feasible.

I think it would be good to first understand why it doesn't work with the
new style ioctls that are in the kernel. The source code of brctl that
I'm looking at here contains:

int br_add_bridge(const char *brname)
{
        int ret;

#ifdef SIOCBRADDBR
        ret = ioctl(br_socket_fd, SIOCBRADDBR, brname);
        if (ret < 0)
#endif
        {
                char _br[IFNAMSIZ];
                unsigned long arg[3]
                        = { BRCTL_ADD_BRIDGE, (unsigned long) _br };

                strncpy(_br, brname, IFNAMSIZ);
                ret = ioctl(br_socket_fd, SIOCSIFBR, arg);
        }

        return ret < 0 ? errno : 0;
}

This means it should first attempt to call SIOCBRADDBR, which has
32 bit compat support in the kernel. The only reasons I can see why
this would not work are:

* the brctl tool in question is built from really old sources that
  don't have the SIOCBRADDBR option.
* it is built against really old kernel headers that do not export
  the SIOCBRADDBR definition.

If neither of the two is true, there is probably a bug somewhere that
wants to get fixed.

	Arnd

Re: [RFC] should we care of COMPAT mode in bridge ?

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 6 Jun 2011 22:19:33 +0200

> I think it would be good to first understand why it doesn't work with the
> new style ioctls that are in the kernel. The source code of brctl that
> I'm looking at here contains:

Eric's 32-bit binary was built against older headers that didn't
define the new ioctls.

That makes it pretty clear to me that we have to support those
older ioctls in compat mode even after all these years.

Re: [RFC] should we care of COMPAT mode in bridge ?

[Chris Friesen]

On 06/06/2011 02:03 PM, David Miller wrote:
> From: Eric Dumazet<eric.dumazet@gmail.com>
> Date: Mon, 06 Jun 2011 21:45:40 +0200
>
>> While trying Alexander Holler patch, I found a 32bit brctl program was
>> not able to work on a 64bit kernel. So I had to switch to another
>> machine for my tests.
>>
>> brctl addbr mybridge
>> ->
>> socket(PF_FILE, SOCK_STREAM, 0)         = 3
>> ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)
>>
>> Should we care or not ?
>
> I think we should make an effort to fix this.

For current status of affairs, this makes sense.

It raises an interesting question though...do we plan on supporting 
compat indefinitely?

As I see it x86 generally makes more sense to run as 64-bit when 
possible due to the extra register availability. Compat is primarily 
useful for the embedded space and for backwards compatibility, and 
ripping it out would cause a lot of grief for legacy 32-bit apps.  It 
would simplify the userspace/kernel interface though.

Chris

-- 
Chris Friesen
Software Developer
GENBAND
chris.friesen@genband.com
www.genband.com

Re: [RFC] should we care of COMPAT mode in bridge ?

[Arnd Bergmann]

On Monday 06 June 2011 22:12:54 Eric Dumazet wrote:
> So for an old binary, compiled at the time SIOCBRADDBR wasnt there (in
> include file I mean), we ended doing :
> 
>         {
>                 char _br[IFNAMSIZ];
>                 unsigned long arg[3]
>                         = { BRCTL_ADD_BRIDGE, (unsigned long) _br };
> 
>                 strncpy(_br, brname, IFNAMSIZ);
>                 ret = ioctl(br_socket_fd, SIOCSIFBR, arg);
>         }
> 
> And this breaks on 64bit kernel

Ah, you were quicker than me at posting this ;-)

> I guess we shall add some logic in kernel to support SIOCSIFBR
> afterall ;)
> 
> Thanks !
> 
> BTW: I confirm that compiling an up2date 32bit brctl with an up2date
> include files is OK on 64bit kernel.

I did a little more digging in the history and found that the code in
brctl was added in this commit:

commit 328f4711bbc369dcccf8f8cfba2adf5dd0f74479
Author: shemminger <shemminger>
Date:   Fri May 21 17:41:48 2004 +0000

    New version of command and library that use sysfs.
    Update make system to build with or without sysfs.

It's not completely clear to me if it's worth supporting older
user space than this, but it's certainly possible.

	Arnd

Re: [RFC] should we care of COMPAT mode in bridge ?

[Arnd Bergmann]

On Monday 06 June 2011 22:23:53 Chris Friesen wrote:
> It raises an interesting question though...do we plan on supporting 
> compat indefinitely?
> 
> As I see it x86 generally makes more sense to run as 64-bit when 
> possible due to the extra register availability. Compat is primarily 
> useful for the embedded space and for backwards compatibility, and 
> ripping it out would cause a lot of grief for legacy 32-bit apps.  It 
> would simplify the userspace/kernel interface though.

There are a lot of applications that require being run as 32 bit,
and some of them are much happier in a chroot environment. Removing
the compat support entirely won't be an option for the next 10 years
at least, AFAICT. Even if we get all x86 folks to migrate to 64 bits
at some point in the distant future, there will always be other
architectures that make the move to 64 bits.

Note that there is currently work going on to add a new x32 ABI
to arch/x86 which makes it possible to use 32 bit pointers with the
full 64 bit register set. If this becomes a success, we will never
be able to build x86 kernels without compat support.

	Arnd

Re: [RFC] should we care of COMPAT mode in bridge ?

[Andreas Schwab]

David Miller <davem@davemloft.net> writes:

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 06 Jun 2011 21:45:40 +0200
>
>> While trying Alexander Holler patch, I found a 32bit brctl program was
>> not able to work on a 64bit kernel. So I had to switch to another
>> machine for my tests.
>> 
>> brctl addbr mybridge
>> ->
>> socket(PF_FILE, SOCK_STREAM, 0)         = 3
>> ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)
>> 
>> Should we care or not ?
>
> I think we should make an effort to fix this.

OTOH, the SIOCSIFBR and SIOCGIFBR ioctls are obsoleted by the
SIOCBRADDBR, SIOCBRDELBR, SIOCBRADDIF, SIOCBRDELIF ioctls and bridge
sysfs files (which have no compat issues).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC] should we care of COMPAT mode in bridge ?

[Eric Dumazet]

Le lundi 06 juin 2011 à 22:26 +0200, Arnd Bergmann a écrit :
> On Monday 06 June 2011 22:12:54 Eric Dumazet wrote:
> > So for an old binary, compiled at the time SIOCBRADDBR wasnt there (in
> > include file I mean), we ended doing :
> > 
> >         {
> >                 char _br[IFNAMSIZ];
> >                 unsigned long arg[3]
> >                         = { BRCTL_ADD_BRIDGE, (unsigned long) _br };
> > 
> >                 strncpy(_br, brname, IFNAMSIZ);
> >                 ret = ioctl(br_socket_fd, SIOCSIFBR, arg);
> >         }
> > 
> > And this breaks on 64bit kernel
> 
> Ah, you were quicker than me at posting this ;-)
> 
> > I guess we shall add some logic in kernel to support SIOCSIFBR
> > afterall ;)
> > 
> > Thanks !
> > 
> > BTW: I confirm that compiling an up2date 32bit brctl with an up2date
> > include files is OK on 64bit kernel.
> 
> I did a little more digging in the history and found that the code in
> brctl was added in this commit:
> 
> commit 328f4711bbc369dcccf8f8cfba2adf5dd0f74479
> Author: shemminger <shemminger>
> Date:   Fri May 21 17:41:48 2004 +0000
> 
>     New version of command and library that use sysfs.
>     Update make system to build with or without sysfs.
> 
> It's not completely clear to me if it's worth supporting older
> user space than this, but it's certainly possible.
> 
> 	Arnd

In my case, I used latest bridge-utils tree, but compiled on a RHEL4
machine :)

# ls -l /usr/include/linux/sockios.h
-rw-r--r--  1 root root 5369 Mar 18  2002 /usr/include/linux/sockios.h
# rpm -qf /usr/include/linux/sockios.h
glibc-kernheaders-2.4-9.1.100.EL

Please note that pretty everything works on this 32bit distro machine,
but a 64bit kernel.

Re: [RFC] should we care of COMPAT mode in bridge ?

[Arnd Bergmann]

On Monday 06 June 2011 22:32:32 Eric Dumazet wrote:
> In my case, I used latest bridge-utils tree, but compiled on a RHEL4
> machine :)
> 
> # ls -l /usr/include/linux/sockios.h
> -rw-r--r--  1 root root 5369 Mar 18  2002 /usr/include/linux/sockios.h
> # rpm -qf /usr/include/linux/sockios.h
> glibc-kernheaders-2.4-9.1.100.EL
> 
> Please note that pretty everything works on this 32bit distro machine,
> but a 64bit kernel.
> 

Ok, that is indeed an excellent reason to make it work.

	Arnd

Re: [RFC] should we care of COMPAT mode in bridge ?

[Eric Dumazet]

Le lundi 06 juin 2011 à 22:31 +0200, Andreas Schwab a écrit :

> OTOH, the SIOCSIFBR and SIOCGIFBR ioctls are obsoleted by the
> SIOCBRADDBR, SIOCBRDELBR, SIOCBRADDIF, SIOCBRDELIF ioctls and bridge
> sysfs files (which have no compat issues).
> 

Yep, unless you are stuck with an old binary :)

Re: [RFC] should we care of COMPAT mode in bridge ?

[Eric Dumazet]

Le lundi 06 juin 2011 à 22:33 +0200, Arnd Bergmann a écrit :

> Ok, that is indeed an excellent reason to make it work.

Indeed.

Even "perf" (compiled as a 32bit binary) is working so nice on this
machine, its really awesome...

Re: [RFC] should we care of COMPAT mode in bridge ?

[Stephen Hemminger]

On Mon, 06 Jun 2011 13:03:09 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 06 Jun 2011 21:45:40 +0200
> 
> > While trying Alexander Holler patch, I found a 32bit brctl program was
> > not able to work on a 64bit kernel. So I had to switch to another
> > machine for my tests.
> > 
> > brctl addbr mybridge
> > ->
> > socket(PF_FILE, SOCK_STREAM, 0)         = 3
> > ioctl(3, SIOCSIFBR, 0xffd509c0)         = -1 EINVAL (Invalid argument)
> > 
> > Should we care or not ?
> 
> I think we should make an effort to fix this.

The problem is that most of the other ioctl's won't work because of use of
SIOCDEVPRIVATE.

Re: [RFC] should we care of COMPAT mode in bridge ?

[Stephen Hemminger]

On Mon, 06 Jun 2011 13:23:15 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> Date: Mon, 6 Jun 2011 22:19:33 +0200
> 
> > I think it would be good to first understand why it doesn't work with the
> > new style ioctls that are in the kernel. The source code of brctl that
> > I'm looking at here contains:
> 
> Eric's 32-bit binary was built against older headers that didn't
> define the new ioctls.
> 
> That makes it pretty clear to me that we have to support those
> older ioctls in compat mode even after all these years.

That is really old, even Debian Lenny isn't that out of date.

Re: [RFC] should we care of COMPAT mode in bridge ?

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 7 Jun 2011 16:51:34 -0700

> The problem is that most of the other ioctl's won't work because of use of
> SIOCDEVPRIVATE.

As we discussed we can pass SIOCDEVPRIVATE requests down to the driver
just like we do for all kinds of other compat ioctls.

Re: [RFC] should we care of COMPAT mode in bridge ?

[Stephen Hemminger]

On Tue, 07 Jun 2011 17:27:27 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Tue, 7 Jun 2011 16:51:34 -0700
> 
> > The problem is that most of the other ioctl's won't work because of use of
> > SIOCDEVPRIVATE.
> 
> As we discussed we can pass SIOCDEVPRIVATE requests down to the driver
> just like we do for all kinds of other compat ioctls.

Ok. patches accepted, but not spending my time on it.