Re: [Cake] Using firewall connmarks as tin selectors

[Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>]

> On 4 Mar 2019, at 16:39, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
> [ ... snipping a bit of context here ... ]
> 
>>>>>> +void cake_update_ct_mark(struct sk_buff *skb, u8 dscp)
>>>>>> +{
>>>>>> +	enum ip_conntrack_info ctinfo;
>>>>>> +	struct nf_conn *ct;
>>>>>> +
>>>>>> +	ct = nf_ct_get(skb, &ctinfo);
>>>>>> +	if (!ct)
>>>>>> +		return;
>>>>>> +
>>>>>> +	ct->mark &= 0x80ffffff;
>>>>>> +	ct->mark |= (0x40 | dscp) << 24;
>>>>> 
>>>>> Right, so we *might* have an argument that putting the *tin* into the
>>>>> fwmark is CAKE's business, but copying over the dscp mark is not
>>>>> something a qdisc should be doing…
>>>> 
>>>> Why ever not?  It’s not the DSCP, it’s a lookup value into the cake
>>>> priority table, it just happens to look like the DSCP ;-)
>>> 
>>> If it quacks like a duck…
>> 
>> I honestly don’t know where to go from here. I’m clearly trying to do
>> something that the kernel doesn’t want to do.
> 
> I'm not disputing that what you're trying to do (moving DSCP field into
> connmark) is useful. I'm just questioning whether CAKE is the right
> place to do this. I think it would fit better in a TC action; either
> modify act_connmark, or create a new action to sync fwmarks and DSCP
> marks…

Interesting.  Thinks out loud - Two actions - ‘act_storedscp’, ‘act_restoredscp'

As I said earlier I couldn't work out how m_conntrack did… anything at all to be honest!

> 
> This would both sidestep the whole conntrack dependency issue, and make
> the same functionality available outside of CAKE (for an HTB-based
> setup, for instance).
> 
>> v2 addressing some of the comments attached.  Is it best to keep the
>> in progress patches here or should they be github PRs ?
> 
> Patches on the mailing list is fine by me, and it seems there are people
> reading the list, but not github, so let's keep it here for now at
> least. However, I'll hold off on more detailed comments on the patch
> until we've resolved the point above. With one exception:

There’s certainly been some response here that’s for sure :-)

> 
>> @@ -1661,13 +1695,14 @@ static struct cake_tin_data *cake_select_tin(struct Qdisc *sch,
>> 		tin = 0;
>> 
>> 	else if (q->rate_flags & CAKE_FLAG_FWMARK && /* use fw mark */
>> -		 skb->mark &&
>> -		 skb->mark <= q->tin_cnt)
>> -		tin = q->tin_order[skb->mark - 1];
>> -
>> -	else if (TC_H_MAJ(skb->priority) == sch->handle &&
>> -		 TC_H_MIN(skb->priority) > 0 &&
>> -		 TC_H_MIN(skb->priority) <= q->tin_cnt)
>> +		   skb->mark & 0x40000000) {
> 
> I think there's something odd with this mask?  There's only one bit set
> in it…

I use the single bit as a flag to indicate cake has stored the  DSCP in the lower 6 bits of the byte.  Otherwise with a DSCP of 0 (the default) it’s rather difficult to know if a connection has been through the cake ’save dscp to fwmark’ process or not.  That also indicates to user space whether it should consider mangling packets or not e.g.

#if bit 6 0 then not been marked by cake - go & mangle the DSCP ready for cake to find & set
#the mark.
    ipt -t mangle -A PREROUTING  -i $IFACE -m mark --mark 0x00/0x40000000 -g QOS_MARK_${IFACE}
    ipt -t mangle -A POSTROUTING -o $IFACE -m mark --mark 0x00/0x40000000 -g QOS_MARK_${IFACE}



Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A