Re: Comments about IPT_ALIGN

[Laszlo Valko <valko@linux.karinthy.hu>]

Hi Thomas!

On Sun, Jan 26, 2003 at 07:05:09PM +0100, Thomas Heinz wrote:
> Apart from breaking some existing targets and matches what about the
> following requirements for match/target info structs:
>         - the struct properties stated in my first posting must hold
>           (of course unions can also be used :)
>         - the end of the struct must be marked by:
>               struct ipt_entry next[0];
>           or
>               long next[0];   // I'm not sure about this!
>           or
>               padd_t next[0]; // typedef __u32/__u64 padd_t dep. on arch

I don't have anything con/pro either of these, but I think for
an entry structure (which is followed by other entry structures) it
is wise to use struct ipt_entry next[0].

I'll check if there's already a generic "strictest alignment on this platform"
define in some Linux arch header.

>         - pointers or arrays of pointers should be avoided but if they're
>           really needed (as in limit) a special pointer type should be
>           used: 32 bit arch: union nf_ptr_t {void *p;
>                                              u_int32_t padd;};
>                 64 bit arch: union nf_ptr_t {void *p;
>                                              u_int64_t padd;};

Pointers to be passed between kernel/user space will not work easily
anyway, so this can only be used for kernel private pointers.
For that purpose, this is OK until the netfilter framework provides
facilities to eliminate pointers from kernel-user interface structures.

>           Maybe there are better ways to achieve the same
>           (__attribute__ ((aligned (__alignof__(long)))) might work).

The problem is that you _cannot_ determine from an x-bit userspace compiler
what alignment restrictions do exist on a y-bit kernelspace compiler. Since
they are two distinct compilers.
That's the fundamental flaw in __alignof__/IPT_ALIGN-type "hacking".

>           Btw if only one pointer is needed it could be put at the end
>           before the next member and it should work fine.

As long as the sizeof() structure does not change... Otherwise you'll
get EINVAL from check_entry.

> Of course this would break some matches/targets but apart from that fact
> it should be ok, isn't it? I mean with this solution IPT_ALIGN is no

Otherwise it's ok...

But please realize that struct ipt_entry next[0] at the end effectively
substitutes the missing IPT_ALIGN from the entry, maybe a more correct way.

> longer necessary and no #ifdefs need to be used except for padd_t (which
> is just an option as there is at least one other way to make it work)
> and maybe for nf_ptr_t.

> > 2) introduce a new way of passing structures, with new structures, probably
> >    with new setsockopt/getsockopt numbers, essentially creating an API
> >    incompatible with the existing one (there are a few other bleeding wounds
> >    waiting to be solved in the existing interface as I saw).
> 
> What do you mean by "other bleeding wounds"? Could you explain a bit
> further?
> 
> 
> Thomas

These are the ones that I see currently (mostly because of relevancy to
wordlength issues):

1) Versioning: we are unable to introduce changes into the interface without
   either breaking existing behaviour, or naming the child (module/socket
   option) differently. And we would need it right now :)

2) Kernel private data: there is currently no way to make a kernel-private
   structure that is not visible in the user-kernel interface (see the pointer
   in ipt_limit). I consider it an ugly hack to make a union to hide/cover
   a kernel-side pointer just because it's part of the interface. It should
   not be there at all.

3) Passing arbirary-sized data: I have seen at least two guys in the last
   month here asking how to do it - putting a pointer into the entry structure
   and having the module do copy_from_User is not the right way to do that.


Regards,
Laszlo