On Mon, Feb 17, 2014 at 12:15:19PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > I think we should always do the module autoloading for nf-nat and
> > > > nf-nat-ipvX modules depending on nf_ct_l3num(ct), then return EAGAIN
> > > > to give another retry. Now, this needs to happen in any case, not only
> > > > when calling ctnetlink_parse_nat_setup().
> > >
> > > Not sure what you mean.  If we enter nf_nat_setup_info without ctnetlink
> > > involvement the nf-nat protocol should already be there (else, how can
> > > we end up in nf_nat_setup_info? NAT/MASQUERADE depends on nf-nat).
> > >
> > > What use-case did you have in mind? (or, to put it differently, where
> > > do you think the module probing logic should be)?
> > 
> > If __nf_nat_l3proto_find returns NULL before trying to attach the null
> > binding, I think you should call the routine to autoload the modules
> > before returning EAGAIN.
> > 
> >         proto = __nf_nat_l3proto_find(nf_ct_l3num(ct));
> >         if (proto == NULL) {
> >                 ... release locks
> >                 request_module(...);
> >                 ... acquire locks again
> >                 return -EAGAIN;
> >         }
> 
> The patch alters ctnetlink to call ctnetlink_parse_nat_setup even when
> NAT attr == NULL.
> 
> nfnetlink_attach_null_binding() returns EAGAIN; this return value is
> propagated back to ctnetlink_parse_nat_setup.
> 
> That will then request_module(), nfnetlink will replay the message.
> 
> running
> conntrack -I -p tcp -s 1.1.1.1 -d 2.2.2.2 --timeout 100 --state \
> 	ESTABLISHED --sport 10 --dport 20
> on newly booted machine works, lsmod pre/post
> shows:
> +nf_conntrack_ipv4      14808  1 
> +nf_defrag_ipv4         12702  1 nf_conntrack_ipv4
> +nf_nat_ipv4            13199  0 
> +nf_nat                 20926  1 nf_nat_ipv4
> 
> Which is the desired behaviour afaiu.

Right, your patch looks good.

> [ If you think calling ctnetlink_parse_nat_setup with NULL attr
>   is abuse, please let me know and I will try to come up with something
>   different ]

I think we can simplify this, the nfnetlink_parse_nat_setup() hook
function is always called under rcu_read_lock. See patch attached.