Re: Last vestiges of NFC

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_IPV4OPT
SSTRIP.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.