Re: [PATCH nf-next 0/2] xt_cgroups fix

[Daniel Borkmann <daniel@iogearbox.net>]

On 03/24/2015 04:42 PM, Florian Westphal wrote:
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>> here's a possible fix for xt_cgroups that was previously reported
>> by Daniel Mack.
>>
>> The first patch refactors common helpers, which is later on being
>> used by the actual fix. Please see individual patches for more
>> details.
>>
>> I have based the changes on nf-next as they're rather big, they
>> are, however, on top of Eric's a94070000388 ("netfilter: xt_socket:
>> prepare for TCP_NEW_SYN_RECV support") from net-next to avoid ugly
>> merge conflicts in xt_socket.
>>
>> If you nevertheless think it's more suited for nf, or I should
>> ignore the above conflicting commit, I'd be happy to rebase.
>
> My main problem with these patches is that we're now paving the way
> for skb->sk testing in input, i.e. doing protocol demuxing steps
> in iptables modules.

Well, we're doing this in xt_socket already, here it's just fixing
a real issue in xt_cgroup. The only alternative I currently see
would be to revert Alexey's commit, but that would limit the scope
of possibilities for xt_cgroup quite a lot. :(

> E.g. why not accept similar patch for xt_owner?
> What about sctp (or any other protocol) support?

Right, I see your concern, but at the same time I think that i.e.
SCTP has much more severe problems, i) being the horrible state of
the stack implementation itself ;), ii) being the lack of more
important features in iptables (i.e. NAT on multi-homing). Anyway,
taken that aside, you could restrict that support, as we currently
do only for available early demuxes, but given that activity on
SCTP I truly doubt that's coming any time soon.

> I don't see anything wrong with the implementation per se but I'm afraid
> we're starting down a slippery slope here.