[PATCH] newnat patch

[Harald Welte]

On Mon, Sep 03, 2001 at 02:09:43PM +0200, Jozsef Kadlecsik wrote:
> On Fri, 31 Aug 2001, Harald Welte wrote:

Hi Jozsef.  First of all sorry for replying to something more than 8 weeks
later - but this issue came up again after looking at your newnat5 patch.

> > > > Either
> > > > a) we call the nat helper function always (like the current code)
> > > > b) we make this decision part of struct ip_nat_helper
> > > >
> > > I implemented in the patch according to b:
> >
> > no, you didn't. You added a callback function. My solution b) has a constant
> > (0/1) as member of ip_nat_helper.
> 
> But the helper then must call a function/must have a condition
> to decide wether the packet really have to be mangled or not - and this
> function could be used as a callback function. I don't see the overhead.

The overhead is one additional function call at a place where we want to 
save as much time as possible, especially somewhat-expensive function calls.

I don't know if either you didn't fully understand my proposal or I miss
some requirement..

Why can't there be a simple constant (0/1) member of struct ip_nat_helper?

The designer of a NAT helper module has to know if his helper

a) needs to see each and every packet where the portnumber/... matches
b) needs to see only packets where the conntrack module (if any) has created
   an expectation for.

This static decision of the developer should be enough.  And even if you say it
is not enough, why can't the helper change this variable (member of
ip_nat_helper) during runtime?

In any case, we'd only have a single pointer deref, not a function call.

Please point me to the thing I'm missing :)

> There is a separated problem, which is highly related with H.323.
> After working on so long with properly dynamically de/registering
> helpers for H.323, I have rewritten my code shamelessly using Sampsa
> Ranta's excellent idea on relying an expectfn. That's simply the perfect
> solution for the extra helpers in the H.323 connection tracking.
> 
> However, there is a little problem when NAT is involved: in the NAT
> expect function one can assign the NAT helper for the expected
> connection as well, but later in the function path, alter_reply is called.
> Which then happily resets the conntrack helper to NULL, undoing what the
> conntrack expectfn did.

Ok. I understand your problem.

> So what would be the proper solution?
> 
> Would it be acceptable to call the expectfn in alter_reply as well
> like this:
> 
> --- ip_conntrack_core.c.orig    Mon Sep  3 13:59:46 2001
> +++ ip_conntrack_core.c Mon Sep  3 14:01:36 2001
> @@ -948,6 +948,11 @@
>                                       newreply);
>         WRITE_UNLOCK(&ip_conntrack_lock);
> 
> +       /* If it was an expected connection, re-run the expectfn
> +          just because of the possible helper alteration */
> +       if (conntrack->master && conntrack->master->expectfn)
> +               conntrack->master->expectfn(conntrack);
> +
>         return 1;
>  }

Mh. No. This looks too much like a dirty hack to me.  It is way too expensive
to call the re-run the expectfn all the time.  We have to be as fast as
possible for the easy cases (easy 'normal' protocols).  I don't think that we
should harm general performance just for the rare difficult cases like h323.

> so that one could rely easily on it?
> 
> Or should we revert back to the complicated dynamic helper registration
> (which itself requires about three additional functions in the core as
> well)?

I have to confess that I never looked at the code. Could you point me to 
some particular patch / piece of code / ... which implements the old dynamic
helper registration?

I guess dynamically registering and unregistering helper functions is 
(because of the locking involved) relatively expensive as well.  So we have
to look for a different solution [maybe at the devel-meeting]....  but even
if there is none I would pay this penalty rather than integrating the 
re-run expectfn() hack, sorry.

> Regards,
> Jozsef

-- 
Live long and prosper
- Harald Welte / laforge@gnumonks.org               http://www.gnumonks.org/
============================================================================
GCS/E/IT d- s-: a-- C+++ UL++++$ P+++ L++++$ E--- W- N++ o? K- w--- O- M- 
V-- PS+ PE-- Y+ PGP++ t++ 5-- !X !R tv-- b+++ DI? !D G+ e* h+ r% y+(*)