lkml.org 
[lkml]   [2006]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [Devel] Re: [PATCH 1/2] iptables 32bit compat layer
    Date
    On Tuesday 21 February 2006 00:23, Andi Kleen wrote:
    > Mishin Dmitry <dim@openvz.org> writes:
    > > Hello,
    > >
    > > This patch set extends current iptables compatibility layer in order to
    > > get 32bit iptables to work on 64bit kernel. Current layer is insufficient
    > > due to alignment checks both in kernel and user space tools.
    > >
    > > This patch introduces base compatibility interface for other ip_tables
    > > modules
    >
    > Nice. But some issues with the implementation
    >
    >
    > +#if defined(CONFIG_X86_64)
    > +#define is_current_32bits() (current_thread_info()->flags & _TIF_IA32)
    >
    > This should be is_compat_task(). And we don't do such ifdefs
    > in generic code. And what you actually need here is a
    > is_compat_task_with_funny_u64_alignment() (better name sought)
    >
    > So I would suggest you add macros for that to the ia64 and x86-64
    > asm/compat.hs and perhaps a ARCH_HAS_FUNNY_U64_ALIGNMENT #define in there.
    agree.

    >
    > + ret = 0;
    > + switch (convert) {
    > + case COMPAT_TO_USER:
    > + pt = (struct ipt_entry_target *)target;
    >
    > etc. that looks ugly. why can't you just define different functions
    > for that? We don't really need in kernel ioctl
    3 functions and the requirement that if defined one, than defined all of them?

    >
    > +#ifdef CONFIG_COMPAT
    > + down(&compat_ipt_mutex);
    > +#endif
    >
    > Why does it need an own lock?
    Because it protects only compatibility translation. We spend a lot of time in
    these cycles and I don't think that it is a good way to hold ipt_mutex for
    this. The only reason of this lock is offset list - in the first iteration I
    fill it, in the second - use it. If you know how to implement this better,
    let me know.

    >
    > Overall the implementation looks very complicated. Are you sure
    > it wasn't possible to do this simpler?
    ughh...
    I don't like this code as well. But seems that it is due to iptables code
    itself, which was designed with no thoughts about compatibility in minds.

    So, I see following approaches:
    1) do translation before pass data to original do_replace or get_entries.
    Disadvantage of such approach is additional 2 cycles through data.
    2) do translation in compat_do_replace and compat_get_entries. Avoidance of
    additional cycles, but some code duplication.
    3) remove alignment checks in kernel - than we need only first time
    translation from kernel to user. But such code will not work with both 32bit
    and 64 bit iptables at the same time.

    Any suggestions?

    >
    >
    > -Andi
    >
    > _______________________________________________
    > Devel mailing list
    > Devel@openvz.org
    > https://openvz.org/mailman/listinfo/devel

    --
    Thanks,
    Dmitry.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-02-21 10:26    [W:0.026 / U:0.024 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site