New version of newnat patch
Fri, 6 Jul 2001 10:11:26 +0200 (CEST)
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.
E-mail : email@example.com, firstname.lastname@example.org
Address : KFKI Research Institute for Particle and Nuclear Physics
H-1525 Budapest 114, POB. 49, Hungary