New version of newnat patch

[Jozsef Kadlecsik]

Hi Harald,

On Thu, 5 Jul 2001, Harald Welte wrote:

> > The problems I see about the new newnat patch:
>
> :) there is now a new 0.91 release, it contains a lot of changes :)

:-)

> > - expect_list is not protected everywhere by the new lock. For example
> >   in init_conntrack the searching in expect_list is read-protected,
> >   but from deleting the list is not protected at all. But there are
> >   other places as well.
>
> what do you mean. Could you be more precise, please?

Forget it. I simply did not interpret correctly the name and the
function of the lock.

> > I have problems with the newnat patches in general:
>
> >         /* remove it from the expectation list, as there may
> >          * expectations survive and we want to leave their list
> >          * in a consistent state -HW */
> >         list_del(&ct->sibling_list);
> >         /* Destroy our master expectation, if we've been expected */
> >         if (ct->master)
> >                 unexpect_related(ct->master);
> >
> >   list_del deletes one entry from a list: the whole sibling_list
> >   should be deleted.
>
> no. expected connections are allowed to persist after the master connection
> died. as described in the newnat-summary.txt, the ip_conntrack_expect's
> exist even after the expected connections are established. the expect will
> live as long as the ip_conntrack of the established slave connection.

I think we are both wrong :-): there is no need to try to delete anything
from the sibling_list. When a conntrack dies, clean_from_lists deletes it
from the hashes and destroys the un-established expectations as well. And
destroy_conntrack is called when the use count of the conntrack reaches
zero, i.e. it has no living established expectation.

> >   And this is a conntrack entry which is being
> >   destroyed. If it was expected, then its expectation entry has already
> >   been deleted from the global list.
>
> yes. It's expectation entry has been deleted (unlinked) from the global
> list, but the struct ip_conntrack_expect lives as long as the struct
> ip_conntrack. (see above)

Yes. But destroy_conntrack calls unexpect_related, which wants to delete
the expectation from the global list again.

> > - ip_nat_delete_sack should be called from do_bindings and not from
> >   the nat helpers.
>
> mh. No. too general. What if somebody wants to write a helper module
> which deals with SACK ?

No way to do otherwise: the condition to call ip_nat_delete_sack in a NAT
helper is never true. The helper is called when there is an expectation,
but SACK must be deleted from the initializing SYN packet.

> > - The code does not deal with resent packets.
>
> I have to find a solution for this, yes.  As well for limiting the number
> of outstanding expectations per master.  I appreciate your patches, but
> I was unable to integrate them now. Have to think more about this first..
> sorry ;) Right now it is up to the helper. The helper could check if
> there already is an expectation for this particular sequence number,
> (if there are not more than one by the protocol design) and skip creating
> a new expectation.

The helper could easily detect resent packets until the helper deals
with max one expectation. If there are multiple expectations, the helper
is forced to maintain a state about its expectations - or to search
sibling_list always. And it should be solved (repeated) in every helper,
when the core could do it as well.

> How would you want to solve this in the core (without saving the original
> tuples) ?  I guess there is no way :(

No, there is no other way. I'll try to send some patches next week.

> > A little bit subjective and would require more changes (and Rusty surely
> > wouldn't like it), but instead of all the new exp_matches_pkt functions
> > I would add a single new flag say NFC_NAT_HELPER_REQUIRED. It could be
> > set in ip_conntrack_expect_related (to pass down pskb requires the
> > mentioned changes) and could be tested in do_bindings. The advantage is
> > that it would work for all protocols, not only for TCP.
>
> Hm, interesting idea.  Some problems:
>
> - how do you handle retransmitted packages? (assuming the retransmit
>   does not cause a new expectation)

I think there is no problem: even when a retransmission, the flag must be
set for the packet as well, so that NAT can then mangle the payload. And
(when done by the core) ip_conntrack_expect_related handles the resent
packets not to raise new expectations.

Regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
WWW-Home: http://www.kfki.hu/~kadlec
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary