bridge sioc[gs]drvspec operations incompatible with COMPAT_NETBSD32

[Matt Thomas]

The use of SIOC[GS]DRVSPEC to copyin or copyout other structures which have pointers/size_t/u_long makes them very hard to deal with in COMPAT_NETBSD32.

The simplest solution is to eliminate the use of the ifbifconf and ifbaconf structures in userland and have BRDGGIFS and BRDGRTS use the ifdrv struct members ifd_len and ifb_data directly for their needs. The netbsd32 compat code already deals with this so this just requires a small change to if_bridge{.c,var.h}. I also converted ifbareq to use fixed types in the diff.

Make brconfig to deal with the new method actually makes brconfig simplier.

[Roy Marples]

Hi Matt

Post by Matt Thomas
The use of SIOC[GS]DRVSPEC to copyin or copyout other structures which
have pointers/size_t/u_long makes them very hard to deal with in
COMPAT_NETBSD32.
The simplest solution is to eliminate the use of the ifbifconf and
ifbaconf structures in userland and have BRDGGIFS and BRDGRTS use the
ifdrv struct members ifd_len and ifb_data directly for their needs.
The netbsd32 compat code already deals with this so this just requires
a small change to if_bridge{.c,var.h}. I also converted ifbareq to
use fixed types in the diff.
Make brconfig to deal with the new method actually makes brconfig simplier.
There is the problem of missing compat code for the old ifbareq but
I’m not sure if it’s really required.
Comments?

Thanks for working on this!

I took your patch and adjusted it some more:
* Added a check in the kernel if we have a function in the command
table as the ipfilter stuff is optional and I think the table
will now always grow beyond it.
* added an extra parameter to do_cmd in brconfig.c so we can
get the returned length from the ioctl. This allows us to know
if we need a bigger buffer or not.

Seems to be working fine now, at least for setup.
Will be able to plug another interface in once PPPoE works to actually
test it with though.

While in brconfig, I notice that the kernel returns the length needed
if the buffer is too small, but brconfig just does old length *2 and
tries again. Is it worthwile fixing this to grow a buffer of what
the kernel actually wants?

Comments on this welcome!

Roy

[Ryota Ozaki]

Post by Roy Marples
Hi Matt

Post by Matt Thomas
The use of SIOC[GS]DRVSPEC to copyin or copyout other structures which
have pointers/size_t/u_long makes them very hard to deal with in
COMPAT_NETBSD32.
The simplest solution is to eliminate the use of the ifbifconf and
ifbaconf structures in userland and have BRDGGIFS and BRDGRTS use the
ifdrv struct members ifd_len and ifb_data directly for their needs.
The netbsd32 compat code already deals with this so this just requires
a small change to if_bridge{.c,var.h}. I also converted ifbareq to
use fixed types in the diff.
Make brconfig to deal with the new method actually makes brconfig simplier.
There is the problem of missing compat code for the old ifbareq but
I'm not sure if it's really required.
Comments?

Thanks for working on this!
* Added a check in the kernel if we have a function in the command
table as the ipfilter stuff is optional and I think the table
will now always grow beyond it.
* added an extra parameter to do_cmd in brconfig.c so we can
get the returned length from the ioctl. This allows us to know
if we need a bigger buffer or not.
Seems to be working fine now, at least for setup.
Will be able to plug another interface in once PPPoE works to actually
test it with though.
While in brconfig, I notice that the kernel returns the length needed
if the buffer is too small, but brconfig just does old length *2 and
tries again. Is it worthwile fixing this to grow a buffer of what
the kernel actually wants?

I think so and changing to len = olen just works for me :)

And bridge ATF tests passed (on amd64); I tried the tests with
intentional initial small buffers and the logic of growing buffers
looks working.

Post by Roy Marples
Comments on this welcome!

LGTM except a nitpick, trailing spaces at [OBRDGGIFS] = ... and
[OBRDGRTS] = ... (they were originally there though :-/).

Thanks,
ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Matt Thomas]

Post by Ryota Ozaki

Post by Roy Marples
Hi Matt

Post by Matt Thomas
The use of SIOC[GS]DRVSPEC to copyin or copyout other structures which
have pointers/size_t/u_long makes them very hard to deal with in
COMPAT_NETBSD32.
The simplest solution is to eliminate the use of the ifbifconf and
ifbaconf structures in userland and have BRDGGIFS and BRDGRTS use the
ifdrv struct members ifd_len and ifb_data directly for their needs.
The netbsd32 compat code already deals with this so this just requires
a small change to if_bridge{.c,var.h}. I also converted ifbareq to
use fixed types in the diff.
Make brconfig to deal with the new method actually makes brconfig simplier.
There is the problem of missing compat code for the old ifbareq but
I'm not sure if it's really required.
Comments?

Thanks for working on this!
* Added a check in the kernel if we have a function in the command
table as the ipfilter stuff is optional and I think the table
will now always grow beyond it.
* added an extra parameter to do_cmd in brconfig.c so we can
get the returned length from the ioctl. This allows us to know
if we need a bigger buffer or not.
Seems to be working fine now, at least for setup.
Will be able to plug another interface in once PPPoE works to actually
test it with though.
While in brconfig, I notice that the kernel returns the length needed
if the buffer is too small, but brconfig just does old length *2 and
tries again. Is it worthwile fixing this to grow a buffer of what
the kernel actually wants?

I think so and changing to len = olen just works for me :)
And bridge ATF tests passed (on amd64); I tried the tests with
intentional initial small buffers and the logic of growing buffers
looks working.

Post by Roy Marples
Comments on this welcome!

LGTM except a nitpick, trailing spaces at [OBRDGGIFS] = ... and
[OBRDGRTS] = ... (they were originally there though :-/).

new diff for if_bridge* is at http://www.netbsd.org/~matt/ifbridge-diff.txt
new diff for brconfig.c is at http://www.netbsd.org/~matt/brconfig-diff.txt

Just minor cleanups.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Ryota Ozaki]

Post by Matt Thomas

Post by Ryota Ozaki

Post by Roy Marples
Hi Matt

Post by Matt Thomas
The use of SIOC[GS]DRVSPEC to copyin or copyout other structures which
have pointers/size_t/u_long makes them very hard to deal with in
COMPAT_NETBSD32.
The simplest solution is to eliminate the use of the ifbifconf and
ifbaconf structures in userland and have BRDGGIFS and BRDGRTS use the
ifdrv struct members ifd_len and ifb_data directly for their needs.
The netbsd32 compat code already deals with this so this just requires
a small change to if_bridge{.c,var.h}. I also converted ifbareq to
use fixed types in the diff.
Make brconfig to deal with the new method actually makes brconfig simplier.
There is the problem of missing compat code for the old ifbareq but
I'm not sure if it's really required.
Comments?

Thanks for working on this!
* Added a check in the kernel if we have a function in the command
table as the ipfilter stuff is optional and I think the table
will now always grow beyond it.
* added an extra parameter to do_cmd in brconfig.c so we can
get the returned length from the ioctl. This allows us to know
if we need a bigger buffer or not.
Seems to be working fine now, at least for setup.
Will be able to plug another interface in once PPPoE works to actually
test it with though.
While in brconfig, I notice that the kernel returns the length needed
if the buffer is too small, but brconfig just does old length *2 and
tries again. Is it worthwile fixing this to grow a buffer of what
the kernel actually wants?

I think so and changing to len = olen just works for me :)
And bridge ATF tests passed (on amd64); I tried the tests with
intentional initial small buffers and the logic of growing buffers
looks working.

Post by Roy Marples
Comments on this welcome!

LGTM except a nitpick, trailing spaces at [OBRDGGIFS] = ... and
[OBRDGRTS] = ... (they were originally there though :-/).

new diff for if_bridge* is at http://www.netbsd.org/~matt/ifbridge-diff.txt
new diff for brconfig.c is at http://www.netbsd.org/~matt/brconfig-diff.txt

Need s/for (;;) {/do {/ in show_interfaces.

Post by Matt Thomas
Just minor cleanups.

With the fix above, the patch works for me.

ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Matt Thomas]

Post by Ryota Ozaki

Post by Matt Thomas

Post by Ryota Ozaki

Post by Roy Marples
Hi Matt

Post by Matt Thomas
The use of SIOC[GS]DRVSPEC to copyin or copyout other structures which
have pointers/size_t/u_long makes them very hard to deal with in
COMPAT_NETBSD32.
The simplest solution is to eliminate the use of the ifbifconf and
ifbaconf structures in userland and have BRDGGIFS and BRDGRTS use the
ifdrv struct members ifd_len and ifb_data directly for their needs.
The netbsd32 compat code already deals with this so this just requires
a small change to if_bridge{.c,var.h}. I also converted ifbareq to
use fixed types in the diff.
Make brconfig to deal with the new method actually makes brconfig simplier.
There is the problem of missing compat code for the old ifbareq but
I'm not sure if it's really required.
Comments?

Thanks for working on this!
* Added a check in the kernel if we have a function in the command
table as the ipfilter stuff is optional and I think the table
will now always grow beyond it.
* added an extra parameter to do_cmd in brconfig.c so we can
get the returned length from the ioctl. This allows us to know
if we need a bigger buffer or not.
Seems to be working fine now, at least for setup.
Will be able to plug another interface in once PPPoE works to actually
test it with though.
While in brconfig, I notice that the kernel returns the length needed
if the buffer is too small, but brconfig just does old length *2 and
tries again. Is it worthwile fixing this to grow a buffer of what
the kernel actually wants?

I think so and changing to len = olen just works for me :)
And bridge ATF tests passed (on amd64); I tried the tests with
intentional initial small buffers and the logic of growing buffers
looks working.

Post by Roy Marples
Comments on this welcome!

LGTM except a nitpick, trailing spaces at [OBRDGGIFS] = ... and
[OBRDGRTS] = ... (they were originally there though :-/).

new diff for if_bridge* is at http://www.netbsd.org/~matt/ifbridge-diff.txt
new diff for brconfig.c is at http://www.netbsd.org/~matt/brconfig-diff.txt

Need s/for (;;) {/do {/ in show_interfaces.

Post by Matt Thomas
Just minor cleanups.

With the fix above, the patch works for me.

Thanks. Fixed and committed.
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de