Increasing skb->mark size

Increasing skb->mark size

[Matt Bennett]

Hello,

Currently we have a number of router features (firewall, QoS, etc)
making use of ip tables and connection tracking. We do this by giving
each feature a certain area of skb->mark (say 8 bits each). This allows
us to simply restore skb->mark (using connection tracking) for packets
in a flow using the logic below.

Our software logic is:

1. The first packet in a flow traverses through ip-tables where
each feature has set rules to mark their section of skb->mark. 

2. We then store the mark into connmark. 

3. Then as each packet in the flow hits ip tables the first rule in
ip-tables simply restores the connmark and the packet goes to egress.

Up until now this has worked very well for us. However since skb->mark
is only 32 bits we have quickly run out of bits for marking. 

This leaves us with two options:
 - Don't allow all features to be enabled at once (i.e. multiple
features share the same area of skb->mark). This is not ideal.

 - Increase the size of skb->mark (or another solution such as adding an
additional field into sk_buff for marking).

Hopefully what I have explained above is a strong example of where
skb->mark is no longer large enough on routers using connection tracking
to achieve superior performance. 

I'm emailing this list for feedback on the feasibility of increasing
skb->mark or adding a new field for marking. Perhaps this extension
could be done under a new CONFIG option. Perhaps there are other ways we
could achieve the desired behaviour?

Thanks,
Matt

Re: Increasing skb->mark size

[Florian Westphal]

Matt Bennett <Matt.Bennett@alliedtelesis.co.nz> wrote:
> I'm emailing this list for feedback on the feasibility of increasing
> skb->mark or adding a new field for marking. Perhaps this extension
> could be done under a new CONFIG option. Perhaps there are other ways we
> could achieve the desired behaviour?

Well I pointed you towards connlabels which provide 128 bit of space
in the conntrack extension area but you did not tell me why you cannot
use it.

Re: Increasing skb->mark size

[Matt Bennett]

On Tue, 2015-11-24 at 21:36 +0100, Florian Westphal wrote:
> Matt Bennett <Matt.Bennett@alliedtelesis.co.nz> wrote:
> > I'm emailing this list for feedback on the feasibility of increasing
> > skb->mark or adding a new field for marking. Perhaps this extension
> > could be done under a new CONFIG option. Perhaps there are other ways we
> > could achieve the desired behaviour?
> 
> Well I pointed you towards connlabels which provide 128 bit of space
> in the conntrack extension area but you did not tell me why you cannot
> use it.
Sorry, I moved the discussion to this list to hopefully gather some new
ideas/opinions.

While connlabels provide 128bits of space skb->mark is still only 32
bits. Since we are using connection tracking to simply restore skb->mark
the use of connlabels by itself doesn't solve the problem I outlined
above. skb->mark would still needs to be increased in size.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Increasing skb->mark size

[Luuk Paulussen]

On 11/25/2015 09:56 AM, Matt Bennett wrote:
> On Tue, 2015-11-24 at 21:36 +0100, Florian Westphal wrote:
>> Matt Bennett <Matt.Bennett@alliedtelesis.co.nz> wrote:
>>> I'm emailing this list for feedback on the feasibility of increasing
>>> skb->mark or adding a new field for marking. Perhaps this extension
>>> could be done under a new CONFIG option. Perhaps there are other ways we
>>> could achieve the desired behaviour?
>> Well I pointed you towards connlabels which provide 128 bit of space
>> in the conntrack extension area but you did not tell me why you cannot
>> use it.
> Sorry, I moved the discussion to this list to hopefully gather some new
> ideas/opinions.
>
> While connlabels provide 128bits of space skb->mark is still only 32
> bits. Since we are using connection tracking to simply restore skb->mark
> the use of connlabels by itself doesn't solve the problem I outlined
> above. skb->mark would still needs to be increased in size.
I've been looking into this further and it does look like something like 
connlabels could be useful in certain cases.  For example, netfilter 
could be used to classify a packet based on its label, rather than 
saving the mark and then classifying/filtering based on the mark in the 
tc subsystem.  However, it looks like currently connlabels is set up 
around setting/clearing individual bits, rather than using masks, so 
while there are 128 bits, it doesn't actually give us that many more 
distinct marks.

Re: Increasing skb->mark size

[Lorenzo Colitti]

On Wed, Nov 25, 2015 at 5:32 AM, Matt Bennett
<Matt.Bennett@alliedtelesis.co.nz> wrote:
> I'm emailing this list for feedback on the feasibility of increasing
> skb->mark or adding a new field for marking. Perhaps this extension
> could be done under a new CONFIG option.

64-bit marks (both skb->mark and sk->sk_mark) would be useful for
hosts doing complex policy routing as well. Current Android releases
use 20 of the 32 bits. If the mark were 64 bits, we could put the UID
in it, and stop using ip rules to implement per-UID routing.

Re: Increasing skb->mark size

[David Miller]

From: Lorenzo Colitti <lorenzo@google.com>
Date: Sun, 29 Nov 2015 17:37:11 +0900

> On Wed, Nov 25, 2015 at 5:32 AM, Matt Bennett
> <Matt.Bennett@alliedtelesis.co.nz> wrote:
>> I'm emailing this list for feedback on the feasibility of increasing
>> skb->mark or adding a new field for marking. Perhaps this extension
>> could be done under a new CONFIG option.
> 
> 64-bit marks (both skb->mark and sk->sk_mark) would be useful for
> hosts doing complex policy routing as well. Current Android releases
> use 20 of the 32 bits. If the mark were 64 bits, we could put the UID
> in it, and stop using ip rules to implement per-UID routing.

If you guys, really anyone, can find a way to remove some other
32-bit item from sk_buff, you can expand skb->mark to 64-bits.

But otherwise, I'm going to be strongly against it.

sk_buff is already enormous and larger than it should be.  So
I'm going to resist any change that makes it even larger.

Thanks.

Re: Increasing skb->mark size

[Florian Westphal]

Matt Bennett <Matt.Bennett@alliedtelesis.co.nz> wrote:
> On Tue, 2015-11-24 at 21:36 +0100, Florian Westphal wrote:
> > Matt Bennett <Matt.Bennett@alliedtelesis.co.nz> wrote:
> > > I'm emailing this list for feedback on the feasibility of increasing
> > > skb->mark or adding a new field for marking. Perhaps this extension
> > > could be done under a new CONFIG option. Perhaps there are other ways we
> > > could achieve the desired behaviour?
> > 
> > Well I pointed you towards connlabels which provide 128 bit of space
> > in the conntrack extension area but you did not tell me why you cannot
> > use it.
> Sorry, I moved the discussion to this list to hopefully gather some new
> ideas/opinions.
> 
> While connlabels provide 128bits of space skb->mark is still only 32
> bits. Since we are using connection tracking to simply restore skb->mark
> the use of connlabels by itself doesn't solve the problem I outlined
> above. skb->mark would still needs to be increased in size.

We have ctnetlink which allows direct setting of ctmark/ctlabels.

Regarding ctlabels, they were originally designed to provide a bit-set
so that ctmark could be used as an 'enumeration' type while ctlabels
could be used to provide distinct, non-overlapping labels.

Re: Increasing skb->mark size

[Lorenzo Colitti]

On Mon, Nov 30, 2015 at 11:08 AM, Florian Westphal <fw@strlen.de> wrote:
>
> > While connlabels provide 128bits of space skb->mark is still only 32
> > bits. Since we are using connection tracking to simply restore skb->mark
> > the use of connlabels by itself doesn't solve the problem I outlined
> > above. skb->mark would still needs to be increased in size.
>
> We have ctnetlink which allows direct setting of ctmark/ctlabels.

Can those labels be set early enough in the stack to affect the source
address chosen for outgoing connections?

Re: Increasing skb->mark size

[Florian Westphal]

Lorenzo Colitti <lorenzo@google.com> wrote:
> On Mon, Nov 30, 2015 at 11:08 AM, Florian Westphal <fw@strlen.de> wrote:
> > We have ctnetlink which allows direct setting of ctmark/ctlabels.
> 
> Can those labels be set early enough in the stack to affect the source
> address chosen for outgoing connections?

No, same restrictions as connmarks or skb marks apply.

Furthermore, since its stored in the conntrack area, the conntrack isn't
created until after the first packet hits NF output hook, and its not
visible until after POSTROUTING hook.

There was an out of tree patch to allow to use ipsets to select
source addresses instead of mark+fwmark:

https://lkml.org/lkml/2012/12/3/93

I think thats something we shouldn't really go for this.

However, if fwmark is too limiting (and given these hacks exist),
maybe we should start thinking for alternatives?

Re: Increasing skb->mark size

[Luuk Paulussen]

On 11/30/2015 02:58 PM, David Miller wrote:
> If you guys, really anyone, can find a way to remove some other 32-bit 
> item from sk_buff, you can expand skb->mark to 64-bits. But otherwise, 
> I'm going to be strongly against it. sk_buff is already enormous and 
> larger than it should be. So I'm going to resist any change that makes 
> it even larger. Thanks. 
Would the level of objection be the same if this was done as an 
"extended mark" field under a configurable off-by-default option? This 
would allow users who need this functionality to enable it while making 
it clear that this is at the expense of increasing sk_buff size.

Re: Increasing skb->mark size

[David Miller]

From: Luuk Paulussen <Luuk.Paulussen@alliedtelesis.co.nz>
Date: Mon, 30 Nov 2015 04:10:43 +0000

> On 11/30/2015 02:58 PM, David Miller wrote:
>> If you guys, really anyone, can find a way to remove some other 32-bit 
>> item from sk_buff, you can expand skb->mark to 64-bits. But otherwise, 
>> I'm going to be strongly against it. sk_buff is already enormous and 
>> larger than it should be. So I'm going to resist any change that makes 
>> it even larger. Thanks. 
> Would the level of objection be the same if this was done as an 
> "extended mark" field under a configurable off-by-default option?

Every distribtion will turn the option on.

Config options hiding "cost" is never an argument to bloat
a critical core datstructure up, sorry.

Re: Increasing skb->mark size

[Luuk Paulussen]

On 11/30/2015 05:49 PM, David Miller wrote:
> From: Luuk Paulussen <Luuk.Paulussen@alliedtelesis.co.nz>
> Date: Mon, 30 Nov 2015 04:10:43 +0000
>
>> On 11/30/2015 02:58 PM, David Miller wrote:
>>> If you guys, really anyone, can find a way to remove some other 32-bit
>>> item from sk_buff, you can expand skb->mark to 64-bits. But otherwise,
>>> I'm going to be strongly against it. sk_buff is already enormous and
>>> larger than it should be. So I'm going to resist any change that makes
>>> it even larger. Thanks.
>> Would the level of objection be the same if this was done as an
>> "extended mark" field under a configurable off-by-default option?
> Every distribtion will turn the option on.
>
> Config options hiding "cost" is never an argument to bloat
> a critical core datstructure up, sorry.
>
Fair enough, although if most distributions would turn it on, it does 
suggest that it is interesting...

I have been looking at the tc_index field in the skb, and I think that 
this might fit our current purposes with some modifications.
The extra bits we are currently looking for are actually for traffic 
control.

We would need a marking target for tc_index in nf/ip tables and a way to 
save/restore it into the connmark.
This would become an additional method of setting tc_index alongside 
dsmark, and the existing tcindex tc classifier can then be used to 
classify the packets.

The only data structure change needed for this would be to add a u16 
tc_index to the nf_conn structure, as we would still
want to reduce the number of rules processed for each individual packet 
by restoring the mark from the connection for all
but the first packet in each flow.

Would this be an acceptable alternative, or is the nf_conn structure 
size restricted in the same way as the sk_buff?

Re: Increasing skb->mark size

[David Miller]

From: Luuk Paulussen <Luuk.Paulussen@alliedtelesis.co.nz>
Date: Tue, 1 Dec 2015 00:12:24 +0000

> On 11/30/2015 05:49 PM, David Miller wrote:
>> From: Luuk Paulussen <Luuk.Paulussen@alliedtelesis.co.nz>
>> Date: Mon, 30 Nov 2015 04:10:43 +0000
>>
>>> On 11/30/2015 02:58 PM, David Miller wrote:
>>>> If you guys, really anyone, can find a way to remove some other 32-bit
>>>> item from sk_buff, you can expand skb->mark to 64-bits. But otherwise,
>>>> I'm going to be strongly against it. sk_buff is already enormous and
>>>> larger than it should be. So I'm going to resist any change that makes
>>>> it even larger. Thanks.
>>> Would the level of objection be the same if this was done as an
>>> "extended mark" field under a configurable off-by-default option?
>> Every distribtion will turn the option on.
>>
>> Config options hiding "cost" is never an argument to bloat
>> a critical core datstructure up, sorry.
>>
> Fair enough, although if most distributions would turn it on, it does 
> suggest that it is interesting...

Lots of things are interesting and useful to many people.

Even the most useful feature I would balk at it's implementation
if it bloated up sk_buff.  Period.

You don't understand what the core issue is, which is sk_buff size
which has an effect on all users.

Re: Increasing skb->mark size

[Luuk Paulussen]

On 12/01/2015 04:55 PM, David Miller wrote:
> Lots of things are interesting and useful to many people.
>
> Even the most useful feature I would balk at it's implementation
> if it bloated up sk_buff.  Period.
>
> You don't understand what the core issue is, which is sk_buff size
> which has an effect on all users.

I have understood the core issue, which is why the rest of the email was 
proposing an alternative solution that didn't involve increasing sk_buff 
size at all (I would still like a comment on the possibility of adding a 
tc_index field to the nf_conn structure to match the one in the skb).

Re: Increasing skb->mark size

[Andi Kleen]

Lorenzo Colitti <lorenzo@google.com> writes:

> On Wed, Nov 25, 2015 at 5:32 AM, Matt Bennett
> <Matt.Bennett@alliedtelesis.co.nz> wrote:
>> I'm emailing this list for feedback on the feasibility of increasing
>> skb->mark or adding a new field for marking. Perhaps this extension
>> could be done under a new CONFIG option.
>
> 64-bit marks (both skb->mark and sk->sk_mark) would be useful for
> hosts doing complex policy routing as well. Current Android releases
> use 20 of the 32 bits. If the mark were 64 bits, we could put the UID
> in it, and stop using ip rules to implement per-UID routing.

This would be be great. I've recently ran into some issues with
the overhead of the Android firewall setup.

So basically you need 4 extra bytes in sk_buff. How about:

- shrinking skb->priority to 2 byte
- skb_iff is either skb->dev->iff or 0. so it could be replaced with a
single bit flag for the 0 case.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: Increasing skb->mark size

[Daniel Borkmann]

On 12/01/2015 08:13 PM, Andi Kleen wrote:
> Lorenzo Colitti <lorenzo@google.com> writes:
>> On Wed, Nov 25, 2015 at 5:32 AM, Matt Bennett
>> <Matt.Bennett@alliedtelesis.co.nz> wrote:
>>> I'm emailing this list for feedback on the feasibility of increasing
>>> skb->mark or adding a new field for marking. Perhaps this extension
>>> could be done under a new CONFIG option.
>>
>> 64-bit marks (both skb->mark and sk->sk_mark) would be useful for
>> hosts doing complex policy routing as well. Current Android releases
>> use 20 of the 32 bits. If the mark were 64 bits, we could put the UID
>> in it, and stop using ip rules to implement per-UID routing.
>
> This would be be great. I've recently ran into some issues with
> the overhead of the Android firewall setup.
>
> So basically you need 4 extra bytes in sk_buff. How about:
>
> - shrinking skb->priority to 2 byte

That wouldn't work, see SO_PRIORITY and such (4 bytes) ...

> - skb_iff is either skb->dev->iff or 0. so it could be replaced with a
> single bit flag for the 0 case.

... and that one wouldn't work on ingress.

Hmm, thinking out loud, maybe it makes sense to combine {mark, priority}
into a mark64 field as union, if the use-case allows to ignore/overwrite
priorities set by applications, or to infer them otherwise based on
different policies like net_prio cgroup (see skb_update_prio()).

Re: Increasing skb->mark size

[Andi Kleen]

> >This would be be great. I've recently ran into some issues with
> >the overhead of the Android firewall setup.
> >
> >So basically you need 4 extra bytes in sk_buff. How about:
> >
> >- shrinking skb->priority to 2 byte
> 
> That wouldn't work, see SO_PRIORITY and such (4 bytes) ...

But does anybody really use the full 4 bytes for the priority?
SO_PRIORITY could well truncate the value.

> 
> >- skb_iff is either skb->dev->iff or 0. so it could be replaced with a
> >single bit flag for the 0 case.
> 
> ... and that one wouldn't work on ingress.

Please explain.

-Andi

Re: Increasing skb->mark size

[Lorenzo Colitti]

On Wed, Dec 2, 2015 at 4:13 AM, Andi Kleen <andi@firstfloor.org> wrote:
> This would be be great. I've recently ran into some issues with
> the overhead of the Android firewall setup.

Can you elaborate on which issues? If such a discussion would be
off-topic here, you can open a bug at
https://code.google.com/p/android/issues/list .

Re: Increasing skb->mark size

[David Ahern]

On 12/1/15 6:58 PM, Andi Kleen wrote:
>>> - skb_iff is either skb->dev->iff or 0. so it could be replaced with a
>>> single bit flag for the 0 case.
>>
>> ... and that one wouldn't work on ingress.
>
> Please explain.

skb->dev is dropped fairly early in the Rx path. skb_iif is needed for 
example for IP_CMSG_PKTINFO.

Re: Increasing skb->mark size

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Tue, 1 Dec 2015 21:42:45 -0800

> On 12/1/15 6:58 PM, Andi Kleen wrote:
>>>> - skb_iff is either skb->dev->iff or 0. so it could be replaced with a
>>>> single bit flag for the 0 case.
>>>
>>> ... and that one wouldn't work on ingress.
>>
>> Please explain.
> 
> skb->dev is dropped fairly early in the Rx path. skb_iif is needed for
> example for IP_CMSG_PKTINFO.

Right, we cannot let skb->dev escape the software interrupt handler, yet
we need some cheap reference to it for socket options et al.

Andi, this is not a 15-20 minute problem to solve.  Many very smart people
have spent days/weeks/months trying to even discover reasonable ways in
which to shrink sk_buff.

The low hanging fruit has been gone for a very long time now.