Mailing List Archive

Hi,

> But I found some vestiges remaining in iptables-1.3.8 that look like this
> - *nfcache |= NFC_UNKNOWN;
> remaining in the init() functions of these extensions:
>
> libipt_policy.c libip6t_policy.c
> libipt_connmark.c libip6t_connmark.c


Oops, nevermind about connmark. I see that's already taken care of
recently in latest svn. But the policy modules still have it.

Best Regards,
Peter

Peter Riley wrote:
> Hi,
>
>> But I found some vestiges remaining in iptables-1.3.8 that look like this
>> - *nfcache |= NFC_UNKNOWN;
>> remaining in the init() functions of these extensions:
>>
>> libipt_policy.c libip6t_policy.c
>> libipt_connmark.c libip6t_connmark.c
>
>
> Oops, nevermind about connmark. I see that's already taken care of
> recently in latest svn. But the policy modules still have it.


I count 132 occurences of nfcache (a few are in headers that must stay
though). I'll happily apply a patch that kills them all.

Patrick McHardy wrote:
>
> I count 132 occurences of nfcache (a few are in headers that must stay
> though). I'll happily apply a patch that kills them all.
>

Patrick, yes I get 134 occurrences on 132 lines in current svn.
The breakdown appears to me to be:

51 init() function declarations in match and target extensions

52 parse() function declarations in match extensions only
(not counting connlimit and multiport which are more complicated than
one declaration per file)

2 parse related function declarations in connlimit
4 parse related function declarations in multiport

5+5 calls in iptables.c & ip6tables.c to ->init() or ->parse() members above

3 occurrences in xtables.h that prototype the above:

struct xtables_match
{...
void (*init)(struct xt_entry_match *m, unsigned int *nfcache);

int (*parse)(int c, char **argv, int invert, unsigned int *flags,
const void *entry, unsigned int *nfcache, struct xt_entry_match **match);

struct xtables_target
{...
void (*init)(struct xt_entry_target *t, unsigned int *nfcache);

3+3 occurrences in dump_entry() in libip4tc.c and libip4tc.c for debugging:

printf("Cache: %08X ", e->nfcache);
if (e->nfcache & NFC_ALTERED) printf("ALTERED ");
if (e->nfcache & NFC_UNKNOWN) printf("UNKNOWN ");

It seems that there is good reason for printing out nfcache contents as long as
those bits are still present in structs ipt_entry/ip6t_entry defined in headers
on the kernel side. After all, this is how I tracked down the problem I am
reporting to begin with!


What all this leaves remaining are the occurrences I mentioned in previous message
whose removal doesn't break anything:


1+1 in libipt_policy.c and libip6t_policy.c init() functions where NFC bits are
still being set:

*nfcache |= NFC_UNKNOWN;

These (among similar others that have already been removed) crept in
subsequent to Pablo Neira's NFC-killer patches that I mentioned in original
post.

2+2 occurrences in the libip4tc.c and libip4tc.c is_same() comparisons:

if (a->nfcache != b->nfcache
...) return NULL;

These are the occurrences causing problems. As mentioned, this prevents
iptables from being able to delete-by-match any rules created by old
userspace tools that still set nfcache bits in entries -- the entries are
not considered "same" because (only) the nfcache bits differ (modulo the
match mask of course).

-----
=134 Total


Please let me know if I can do anything more regarding this.


Best Regards,
Peter

On Aug 30 2007 08:13, Peter Riley wrote:
>Patrick McHardy wrote:
>>
>> I count 132 occurences of nfcache (a few are in headers that must stay
>> though). I'll happily apply a patch that kills them all.
>>
>
>Patrick, yes I get 134 occurrences on 132 lines in current svn.
>The breakdown appears to me to be:
[...]

> printf("Cache: %08X ", e->nfcache);
> if (e->nfcache & NFC_ALTERED) printf("ALTERED ");
> if (e->nfcache & NFC_UNKNOWN) printf("UNKNOWN ");
>
> It seems that there is good reason for printing out nfcache contents as long as
> those bits are still present in structs ipt_entry/ip6t_entry defined in headers
> on the kernel side. After all, this is how I tracked down the problem I am
> reporting to begin with!

Do we still need nfcache anyway?


Jan
--

On Thu, 30 Aug 2007, Peter Riley wrote:

> Patrick McHardy wrote:
>>
>> I count 132 occurences of nfcache (a few are in headers that must stay
>> though). I'll happily apply a patch that kills them all.
>>
>
> Patrick, yes I get 134 occurrences on 132 lines in current svn.
> The breakdown appears to me to be:
>
> 51 init() function declarations in match and target extensions
>
> 52 parse() function declarations in match extensions only
> (not counting connlimit and multiport which are more complicated than
> one declaration per file)
>
> 2 parse related function declarations in connlimit
> 4 parse related function declarations in multiport
>
> 5+5 calls in iptables.c & ip6tables.c to ->init() or ->parse() members above
>
> 3 occurrences in xtables.h that prototype the above:
>
> struct xtables_match
> {...
> void (*init)(struct xt_entry_match *m, unsigned int *nfcache);
>
> int (*parse)(int c, char **argv, int invert, unsigned int *flags,
> const void *entry, unsigned int *nfcache, struct xt_entry_match **match);
>
> struct xtables_target
> {...
> void (*init)(struct xt_entry_target *t, unsigned int *nfcache);
>
> 3+3 occurrences in dump_entry() in libip4tc.c and libip4tc.c for debugging:
>
> printf("Cache: %08X ", e->nfcache);
> if (e->nfcache & NFC_ALTERED) printf("ALTERED ");
> if (e->nfcache & NFC_UNKNOWN) printf("UNKNOWN ");
>
> It seems that there is good reason for printing out nfcache contents as long as
> those bits are still present in structs ipt_entry/ip6t_entry defined in headers
> on the kernel side. After all, this is how I tracked down the problem I am
> reporting to begin with!
>
>
> What all this leaves remaining are the occurrences I mentioned in previous message
> whose removal doesn't break anything:
>
>
> 1+1 in libipt_policy.c and libip6t_policy.c init() functions where NFC bits are
> still being set:
>
> *nfcache |= NFC_UNKNOWN;
>
> These (among similar others that have already been removed) crept in
> subsequent to Pablo Neira's NFC-killer patches that I mentioned in original
> post.
>
> 2+2 occurrences in the libip4tc.c and libip4tc.c is_same() comparisons:
>
> if (a->nfcache != b->nfcache
> ...) return NULL;
>
> These are the occurrences causing problems. As mentioned, this prevents
> iptables from being able to delete-by-match any rules created by old
> userspace tools that still set nfcache bits in entries -- the entries are
> not considered "same" because (only) the nfcache bits differ (modulo the
> match mask of course).
>
> -----
> =134 Total
>
>
> Please let me know if I can do anything more regarding this.


Basically all of them can go except those in include/linux/*.h files.

Jan Engelhardt wrote:
> On Aug 30 2007 08:13, Peter Riley wrote:
>> Patrick McHardy wrote:
>>>
>>> I count 132 occurences of nfcache (a few are in headers that must stay
>>> though). I'll happily apply a patch that kills them all.
>>>
>> Patrick, yes I get 134 occurrences on 132 lines in current svn.
>> The breakdown appears to me to be:
> [...]
>
> Do we still need nfcache anyway?
>

It seems to me there are three options....

Let's Make A Deal and say three "curtains":

Behind curtain #1 is... ** A Late-Summer Vacation Package in Las Vegas!! **

You've worked hard enough, it's hot and dry out, so just do the minimum,
kick back and relax...

Leave the kernel headers alone, leave the iptables headers alone.
Then

struct xtables_match keeps void (*init)(..., unsigned int *nfcache);
int (*parse)(..., unsigned int *nfcache, ...);

struct xtables_target keeps void (*init)(..., unsigned int *nfcache);

So the passing of *nfcache in the ->parse() and ->init() members of the
extensions stays, plus the occurrences in the calls to them, and the
debugging dump too. But this is the bulk of the occurrences Patrick
mentioned... Only the small vestiges that actually do something are
removed from is_same() and the two policy extensions.

I hear the Vegas greens (shall I say browns?) ain't much good for golfing
(nothing but sand traps in the desert), so the nfcache-golfing scores won't
improve very much: 134 - 6 = 128.

But the best part is: no one ever has to know! What happens in Vegas stays
in Vegas. No backwards compatibility breakage.

It's a long patch-y road out to Las Vegas, but thankfully, with this option,
Pablo already did most of the driving!

Behind curtain #2: ** Free enemas for you, and your friends! **

Forget about ass-backwards compatibility and purge your cache! Alter the
iptables extension API in xtables.h so the function prototypes for ->init()
and ->parse() stop causing all the crap to be passed. But leave the really
hard ob-struct-ion in your ipt_entry. It may be too painful to reach that
deep down into the kernel to remove it.

Then, you can flush out all of those toxins in the extensions and cleanse
the calls to them in iptables.c. Those nasty blockages that iptables can't
purge because of the (a->nfcache != b->nfcache) comparison can be rooted out
too (as in #1).

But let's be realistic, the fresh healthy feeling won't last forever.
The next time you come down with a bug and really need to make a dump,
dump_entry() should still be able to pass the bits of cache out of your
ipt_entry. At least keep this bit: printf("Cache: %08X ", e->nfcache);

Now, every john out there with cache stuck in his libipt_POOBAR.c extension
is going to have to join in. So the downside is, while this option might be
cathartic for you, some of your friends may end up feeling a little ... violated.
And to be pointed and blunt (ouch), a lot of old code will go into the toilet,
down the drain.

With that newfound looseness in the hips, though, your handicap can greatly
improve: nfcache-golf score = 134 - 128 = 6.


Behind curtain #3: Is that a goat? a gnu? No, a penguin!!
(Plus we'll let your friends can keep their enemas. Penguin gets one too!)

Go deeper, purge every last one of the 134 stinky bits of nfcache! The iptables
headers change as before, and now kernel headers ip_tables.h and ip6_tables.h
can drop nfcache in struct ipt_entry/compat_ipt_entry/ip6t_entry. Even get rid
of the #define NFC_* in ./include/linux/netfilter*.h. Hold nothing back...

Those with kernel patches or userspace tools will all just have to suck it up
like the extensions people had to in #2. But when you're asked what you did
last summer, you'll have a big change to tell them about! :-)


Time to choose!

(Apologies to Monty Hall, The City of Las Vegas, and all who thought that was lame...)

Best Regards,
Peter

PS- My vote, if indeed I have one, is for #1 with no breakage of backwards
compatibility. See the fixed up patch attached. Is it worthwhile to go further?

Peter Riley wrote:
> Jan Engelhardt wrote:
>> On Aug 30 2007 08:13, Peter Riley wrote:
>>> Patrick McHardy wrote:
>>>> I count 132 occurences of nfcache (a few are in headers that must stay
>>>> though). I'll happily apply a patch that kills them all.
>>>>
>>> Patrick, yes I get 134 occurrences on 132 lines in current svn.
>>> The breakdown appears to me to be:
>> [...]
>>
>> Do we still need nfcache anyway?
>>
>
> It seems to me there are three options....
>
> [...]
> Forget about ass-backwards compatibility and purge your cache!


We don't care about binary compatiblity between different userspace
releases. All we care about is not breaking userspace<->kernel
compatiblity.

> Alter the
> iptables extension API in xtables.h so the function prototypes for ->init()
> and ->parse() stop causing all the crap to be passed. But leave the really
> hard ob-struct-ion in your ipt_entry. It may be too painful to reach that
> deep down into the kernel to remove it.
>
> Then, you can flush out all of those toxins in the extensions and cleanse
> the calls to them in iptables.c. Those nasty blockages that iptables can't
> purge because of the (a->nfcache != b->nfcache) comparison can be rooted out
> too (as in #1).
>
> But let's be realistic, the fresh healthy feeling won't last forever.
> The next time you come down with a bug and really need to make a dump,
> dump_entry() should still be able to pass the bits of cache out of your
> ipt_entry. At least keep this bit: printf("Cache: %08X ", e->nfcache);


The kernel doesn't use it, its *always* zero.

> Behind curtain #3: Is that a goat? a gnu? No, a penguin!!
> (Plus we'll let your friends can keep their enemas. Penguin gets one too!)
>
> Go deeper, purge every last one of the 134 stinky bits of nfcache! The iptables
> headers change as before, and now kernel headers ip_tables.h and ip6_tables.h
> can drop nfcache in struct ipt_entry/compat_ipt_entry/ip6t_entry. Even get rid
> of the #define NFC_* in ./include/linux/netfilter*.h. Hold nothing back...

Thats not possible since it breaks userspace <-> kernel compatiblity.

I prefer to get rid of all of them where possible, but if you want
to do only #1, thats also fine.

Patrick McHardy wrote:
> Peter Riley wrote:
>> Jan Engelhardt wrote:
>>> On Aug 30 2007 08:13, Peter Riley wrote:
>>>> Patrick McHardy wrote:
>>>>>
>>>>> I count 132 occurences of nfcache (a few are in headers that must stay
>>>>> though). I'll happily apply a patch that kills them all.
>>>>>
> [...]
> We don't care about binary compatiblity between different userspace
> releases. All we care about is not breaking userspace<->kernel
> compatiblity.

Ahh, ok ok. I was thrown off by "a few are in headers that must stay."
Since the only occurrences in iptables headers are the prototypes for
the ->init() and ->parse() members in the extensions API, that implied
nearly all occurrences really had to stay.

Attached patch *does* change that header incompatibly.

>> [...]
>> At least keep this bit: printf("Cache: %08X ", e->nfcache);
>
> The kernel doesn't use it, its *always* zero.

heh, well the whole point of this thread was about dealing with the
fact that it isn't! :-P But no matter, it's all cool now..

In the end I kept that one line in dump_entry() in libip[46]tc.c,
only for the sake of completeness. The dump_entry() function exists
to dump out the members of an ipt_entry. As you said, nfcache must
remain in the struct. Please delete the line if you still really
want it gone.

> I prefer to get rid of all of them where possible, but if you want

Gotcha, patch attached.

I think there should at least be some kind of prominent changelog or
warning notice somewhere that "prototypes in the iptables extension
API have changed incompatibly after so many years so your custom match
extension may now segmentation fault upon parsing if not updated".

p-o-m-ng probably needs patching now too. I'll take a look...

Best Regards,
Peter

Peter Riley wrote:
>
> p-o-m-ng probably needs patching now too. I'll take a look...

A quick check found four occurrences.
These two are in old kernel code.

patchlets/TARPIT/linux/net/ipv4/netfilter/ipt_TARPIT.c
- nskb->nfcache = 0;

patchlets/IPV4OPTSSTRIP/linux/net/ipv4/netfilter/ipt_IPV4OPTSSTRIP.c
- skb->nfcache |= NFC_ALTERED;


But these two are in match extensions. See patch.

patchlets/ipv4options/iptables/extensions/libipt_ipv4options.c
patchlets/u32/iptables/extensions/libipt_u32.c


Best Regards,
Peter

Peter Riley wrote:
> Patrick McHardy wrote:
>>
>> The kernel doesn't use it, its *always* zero.
>
> heh, well the whole point of this thread was about dealing with the
> fact that it isn't! :-P But no matter, it's all cool now..
>
> In the end I kept that one line in dump_entry() in libip[46]tc.c,
> only for the sake of completeness. The dump_entry() function exists
> to dump out the members of an ipt_entry. As you said, nfcache must
> remain in the struct. Please delete the line if you still really
> want it gone.


I kept it.

>> I prefer to get rid of all of them where possible, but if you want
>
> Gotcha, patch attached.


Applied, thanks a lot Peter.

> I think there should at least be some kind of prominent changelog or
> warning notice somewhere that "prototypes in the iptables extension
> API have changed incompatibly after so many years so your custom match
> extension may now segmentation fault upon parsing if not updated".


We had lots of changes in this area very recently anyway
because of the new userspace xtables support, probably
things won't even compile anymore. But I agree, we'll add
a warning to next release announcement.

Peter Riley wrote:
> Peter Riley wrote:
>> p-o-m-ng probably needs patching now too. I'll take a look...
>
> A quick check found four occurrences.
> These two are in old kernel code.
>
> patchlets/TARPIT/linux/net/ipv4/netfilter/ipt_TARPIT.c
> - nskb->nfcache = 0;
>
> patchlets/IPV4OPTSSTRIP/linux/net/ipv4/netfilter/ipt_IPV4OPTSSTRIP.c
> - skb->nfcache |= NFC_ALTERED;


That seems to be 2.4 code.

> But these two are in match extensions. See patch.
>
> patchlets/ipv4options/iptables/extensions/libipt_ipv4options.c
> patchlets/u32/iptables/extensions/libipt_u32.c


Also applied, thanks.