On Wed, Feb 1, 2012 at 3:47 PM, Jan Engelhardt <jengelh@xxxxxxxxxx> 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 netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
