On Wed, Feb 1, 2012 at 2:24 AM, Jan Engelhardt <[email protected]> wrote:
> On Monday 2012-01-30 10:01, richard -rw- weinberger wrote:
>
>>On Sun, Jan 22, 2012 at 10:56 PM, Richard Weinberger <[email protected]> wrote:
>>> ipt_LOG and ip6_LOG have a lot of common code, merge them
>>> to reduce duplicate code.
>>>
>>> Signed-off-by: Richard Weinberger <[email protected]>
>>
>>No comments?
>
> Adding Pablo to Cc.
>
>
> 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...
>
>>[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.
--
Thanks,
//richard
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.
>>>[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.
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.
On Wed, Feb 1, 2012 at 3:47 PM, Jan Engelhardt <[email protected]> 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
On Wednesday 2012-02-01 16:08, richard -rw- weinberger wrote:
>> 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;
>> +};
>
>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.
Aw don't do that. That is not maintainer-friendly (since headers
are more or less regularly updated by copying from kernel sources).
Keep the structs the same on each side.
That's why we have .userspacesize in the first place.
See libxt_quota.c on how it's done.
.size = XT_ALIGN(sizeof(struct xt_log_info_v1)),
.userspacesize = offsetof(struct xt_log_info_v1, firstkernelmember),