nf_nat_core just using l3proto_find_get

nf_nat_core just using l3proto_find_get

[Jan Engelhardt]

Hi,


I just noticed that nf_nat_core.c uses

        l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);

in its nf_nat_init() function. Why is not it using 
nf_ct_l3proto_module_try_get here?

Re: nf_nat_core just using l3proto_find_get

[Patrick McHardy]

Jan Engelhardt wrote:
> Hi,
> 
> 
> I just noticed that nf_nat_core.c uses
> 
>         l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);
> 
> in its nf_nat_init() function. Why is not it using 
> nf_ct_l3proto_module_try_get here?

There is no such function, nf_ct_l3proto_find_get() takes a
module reference.

Re: nf_nat_core just using l3proto_find_get

[Jan Engelhardt]

On Feb 19 2008 13:22, Patrick McHardy wrote:
> Jan Engelhardt wrote:
>> Hi,
>> 
>> 
>> I just noticed that nf_nat_core.c uses
>> 
>>         l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);
>> 
>> in its nf_nat_init() function. Why is not it using
>> nf_ct_l3proto_module_try_get here?
>
> There is no such function, nf_ct_l3proto_find_get() takes a
> module reference.
>
Hm?

=== nf_nat_core.c ===
        spin_unlock_bh(&nf_nat_lock);

        /* Initialize fake conntrack so that NAT will skip it */
        nf_conntrack_untracked.status |= IPS_NAT_DONE_MASK;

        l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);
        return 0;

 cleanup_extend:
        nf_ct_extend_unregister(&nat_extend);
        return ret;
}
===

Since nf_ct_l3proto_find_get is listed in System.map, it
cannot be a macro, but it is a function indeed.
And nf_ct_l3proto_try_module_get does take a numerical value:

=== xt_conntrack.c
conntrack_mt_check(const char *tablename, const void *ip,
                   const struct xt_match *match, void *matchinfo,
                   unsigned int hook_mask)
{
        if (nf_ct_l3proto_try_module_get(match->family) < 0) {
                printk(KERN_WARNING "can't load conntrack support for "
                                    "proto=%u\n", match->family);
===

leading to the original question why nf_nat_core does not use
nf_ct_l3proto_try_module_get.

Re: nf_nat_core just using l3proto_find_get

[Patrick McHardy]

Jan Engelhardt wrote:
> On Feb 19 2008 13:22, Patrick McHardy wrote:
>> Jan Engelhardt wrote:
>>> I just noticed that nf_nat_core.c uses
>>>
>>>         l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);
>>>
>>> in its nf_nat_init() function. Why is not it using
>>> nf_ct_l3proto_module_try_get here?
>> There is no such function, nf_ct_l3proto_find_get() takes a
>> module reference.
>>
> Hm?
> 
> [...]
> 
> leading to the original question why nf_nat_core does not use
> nf_ct_l3proto_try_module_get.


Why should it? There's no need for manual module loading since
there already is a symbol dependency.

Re: nf_nat_core just using l3proto_find_get

[Jan Engelhardt]

On Mar 11 2008 15:05, Patrick McHardy wrote:
>> > >
>> > >         l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);
>> > >
>> [...]
>> leading to the original question why nf_nat_core does not use
>> nf_ct_l3proto_try_module_get.
>
> Why should it? There's no need for manual module loading since
> there already is a symbol dependency.
>

According to `modinfo nf_nat`, nf_nat does not have a dependency
on nf_conntrack_ipv4.

So what happens when nf_nat is loaded before nf_conntrack_ipv4?
(Even if there are dependencies, this question is valid)

l3proto will be NULL, and is this ok?

Re: nf_nat_core just using l3proto_find_get

[Patrick McHardy]

Jan Engelhardt wrote:
> On Mar 11 2008 15:05, Patrick McHardy wrote:
>>>>>         l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);
>>>>>
>>> [...]
>>> leading to the original question why nf_nat_core does not use
>>> nf_ct_l3proto_try_module_get.
>> Why should it? There's no need for manual module loading since
>> there already is a symbol dependency.
>>
> 
> According to `modinfo nf_nat`, nf_nat does not have a dependency
> on nf_conntrack_ipv4.

Right, that was iptable_nat.

> So what happens when nf_nat is loaded before nf_conntrack_ipv4?
> (Even if there are dependencies, this question is valid)
> 
> l3proto will be NULL, and is this ok?

No, it will break when translating ICMP errors.

Re: nf_nat_core just using l3proto_find_get

[Jan Engelhardt]

On Mar 11 2008 15:20, Patrick McHardy wrote:
>
>> So what happens when nf_nat is loaded before nf_conntrack_ipv4?
>> (Even if there are dependencies, this question is valid)
>> 
>> l3proto will be NULL, and is this ok?
[well, not NULL, but it will point to the l3Proto_generic]
>
> No, it will break when translating ICMP errors.
>
I see — so this patch is needed, seems like it?

===
commit 0d3f177a94aadedf9fd26f230d473e636eb0553c
Author: Jan Engelhardt <jengelh@computergmbh.de>
Date:   Tue Mar 11 20:40:05 2008 +0100

    [NETFILTER]: nf_nat: autoload IPv4 connection tracking
    
    Without this patch, the generic L3 tracker would kick in
    if nf_conntrack_ipv4 was not loaded before nf_nat, which
    would lead to translation problems with ICMP errors.
    
    Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>

diff --git a/net/ipv4/netfilter/nf_nat_core.c b/net/ipv4/netfilter/nf_nat_core.c
index 9c8aa8d..a9de065 100644
--- a/net/ipv4/netfilter/nf_nat_core.c
+++ b/net/ipv4/netfilter/nf_nat_core.c
@@ -657,7 +657,12 @@ static int __init nf_nat_init(void)
 	/* Initialize fake conntrack so that NAT will skip it */
 	nf_conntrack_untracked.status |= IPS_NAT_DONE_MASK;
 
-	l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);
+	if (nf_ct_l3proto_try_module_get(AF_INET) < 0)
+		printk(KERN_INFO KBUILD_MODNAME ": Could not load connection "
+		       "tracking for l3proto %u, using generic L3 tracking "
+		       "only.\n", AF_INET);
+
+	l3proto = __nf_ct_l3proto_find(AF_INET);
 	return 0;
 
  cleanup_extend:
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: nf_nat_core just using l3proto_find_get

[Patrick McHardy]

Jan Engelhardt wrote:
> I see — so this patch is needed, seems like it?
> 
> ===
> commit 0d3f177a94aadedf9fd26f230d473e636eb0553c
> Author: Jan Engelhardt <jengelh@computergmbh.de>
> Date:   Tue Mar 11 20:40:05 2008 +0100
> 
>     [NETFILTER]: nf_nat: autoload IPv4 connection tracking
>     
>     Without this patch, the generic L3 tracker would kick in
>     if nf_conntrack_ipv4 was not loaded before nf_nat, which
>     would lead to translation problems with ICMP errors.
>     
>     Signed-off-by: Jan Engelhardt <jengelh@computergmbh.de>
> 
> diff --git a/net/ipv4/netfilter/nf_nat_core.c b/net/ipv4/netfilter/nf_nat_core.c
> index 9c8aa8d..a9de065 100644
> --- a/net/ipv4/netfilter/nf_nat_core.c
> +++ b/net/ipv4/netfilter/nf_nat_core.c
> @@ -657,7 +657,12 @@ static int __init nf_nat_init(void)
>  	/* Initialize fake conntrack so that NAT will skip it */
>  	nf_conntrack_untracked.status |= IPS_NAT_DONE_MASK;
>  
> -	l3proto = nf_ct_l3proto_find_get((u_int16_t)AF_INET);
> +	if (nf_ct_l3proto_try_module_get(AF_INET) < 0)
> +		printk(KERN_INFO KBUILD_MODNAME ": Could not load connection "
> +		       "tracking for l3proto %u, using generic L3 tracking "
> +		       "only.\n", AF_INET);
> +
> +	l3proto = __nf_ct_l3proto_find(AF_INET);


Actually I think this should be a hard error, NAT doesn't
work without IPv4 conntrack. The easiest fix seems to add
a need_ipv4_conntrack() "call" to the init function.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html