lkml.org 
[lkml]   [2012]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 1/6] Netfilter: Merge ipt_LOG and ip6_LOG into xt_LOG
    From
    On Wed, Feb 1, 2012 at 3:47 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
    >
    > On Wednesday 2012-02-01 10:14, richard -rw- weinberger wrote:
    >>>
    >>> Do you have the patchset as a retrievable git tree, btw?
    >>
    >>Only for kernel stuff:
    >>git://git.kernel.org/pub/scm/linux/kernel/git/rw/misc.git xt_LOG
    >>
    >>If you want it for the user space part too, I can setup something...
    >
    > That would be welcome.

    Okay, will do!

    >>>>[PATCH 6/6] iptables: xt_LOG: Add ring buffer support
    >>>>
    >>>>in libxt_LOG.c:
    >>>>+.size = XT_ALIGN(sizeof(struct xt_log_info_v1) + sizeof(void *)),
    >>>
    >>> Why the extra void*?
    >>
    >>The kernel space part of  xt_log_info_v1 differs from the user space struct,
    >>it contains an extra pointer to the ring buffer context.
    >>Iptables must not see this extra pointer because the kernel writes to it.
    >
    > I don't see that extra pointer (outside xt_log_info_v1) that you talk about.
    >
    > +struct xt_log_info_v1 {
    > +       unsigned char level;
    > +       unsigned char logflags;
    > +       char prefix[30];
    > +
    > +       char ring_name[30];
    > +       unsigned long ring_size;
    > +       struct xt_LOG_ring_ctx *rctx;
    > +};
    >
    > This is the struct as you defined it. First, you must not use
    > unsigned long (due to its flexible size, which is an ABI breaker; cf.
    > "Writing Netfilter Modules" PDF), and the pointer must be tagged for
    > 8-alignment as well for the same reason.

    See:
    include/linux/netfilter/xt_LOG.h (user space part).

    +struct xt_log_info_v1 {
    + unsigned char level;
    + unsigned char logflags;
    + char prefix[30];
    +
    + char ring_name[30];
    + unsigned long ring_siVs.ze;
    +};

    Here is no context pointer.

    I'll fix the unsigned long issue.

    > Second, the kernel will reject a ruleset if (kernel) .targetsize != (user)
    > .size. This is likely to occur, since you essentially have
    > .targetsize=72 in xt_LOG.c and .size=72+sizeof(void*)
    > in libxt_LOG.
    >
    >
    >
    > Check commit 8a252e8941cd50e4390e6fedf280ba1c7ff9ac43 for
    > trailing whitespace, git log -p shows them.
    >
    > +       if (par->family == NFPROTO_IPV4)
    > +               m = ipt_log_packet(NFPROTO_IPV4, par->hooknum, skb, par->in,
    > +                                  par->out, &li, loginfo->prefix);
    > +#if IS_ENABLED(CONFIG_IPV6)
    > +       else if (par->family == NFPROTO_IPV6)
    > +               m = ip6t_log_packet(NFPROTO_IPV6, par->hooknum, skb, par->in,
    > +                                   par->out, &li, loginfo->prefix);
    > +#endif
    > +       else
    > +               BUG();
    >
    > Please avoid BUG. Either do nothing in the 'else' case, or print
    > a courtesy message with some unkn_log_packet() or
    > WARN_ON_ONCE(). The same goes for xt_LOG_ring_exit, which I vote
    > for being reduced to WARN_ON as well. If the condition triggers,
    > one will have a little memory leak, but at least the machine
    > still runs.

    Will fix!

    --
    Thanks,
    //richard
    --
    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: 2012-02-01 16:11    [W:4.465 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site