[Devel] [PATCH RFC rh7] vznetstat: Move the code to drop redundant skb marks to *_xmit() functions #PSBM-122082

[Konstantin Khorenko]

On 11/17/2020 08:55 AM, Vasily Averin wrote:
> See comments below
> On 11/16/20 10:40 PM, Konstantin Khorenko wrote:
>> We have added code to clear vz specific skb marks, but it's done in the
>> hook NF_INET_LOCAL_IN (which is not the very first one) and if someone
>> sets a mark with xt_mark in PREROUTING chain, the mark will be set in
>> NF_INET_PRE_ROUTING hook (which is prior to NF_INET_LOCAL_IN) and
>> dropped by our hook venet_acct_in_hook() in later NF_INET_LOCAL_IN hook.
>>
>
>> diff --git a/drivers/net/venetdev.c b/drivers/net/venetdev.c
>> index 28d06bca1f86..5385e3e7f30f 100644
>> --- a/drivers/net/venetdev.c
>> +++ b/drivers/net/venetdev.c
>> @@ -464,6 +464,12 @@ static int venet_xmit(struct sk_buff *skb, struct net_device *dev)
>>
>>  	stats = venet_stats(dev, smp_processor_id());
>>  	ve = dev_net(dev)->owner_ve;
>> +	/*
>> +	 * If the packet goes in VE0 -> VE direction, drop the skb mark used
>> +	 * for vz traffic shaping.
>
> Please make comment more clear.
> Why we should drop vz traffic shaping mark here?
> You know that it is used for outgoing traffic only, but reader does not.

Ok, will do in the next version.

>
>> diff --git a/kernel/ve/vznetstat/ip_vznetstat.c b/kernel/ve/vznetstat/ip_vznetstat.c
>> index 5ea978d6dd88..a33c41c05dba 100644
>> --- a/kernel/ve/vznetstat/ip_vznetstat.c
>> +++ b/kernel/ve/vznetstat/ip_vznetstat.c
>> @@ -92,6 +103,15 @@ static unsigned int venet_acct_out_hook(const struct nf_hook_ops *hook,
>>  	 */
>>  	if (dst && (dst->dev->flags & IFF_LOOPBACK))
>>  		goto out;
>> +	/*
>> +	 * Don't account and mark VE traffic if it does not cross VE/VE0
>> +	 * boundary.
>> +	 * For VE0 there can be extra accounting in case of traffic
>> +	 * between Docker Containers in VE0.
>> +	 */
>> +	if (!(ve_is_super(out->nd_net->owner_ve)) &&
>> +	    dst && !(dst->dev->features & NETIF_F_VENET))
>> +		goto out;
>
> Are you sure that we can use dst for correct "outgoing traffic"-check here?
> Can be traffic re-routed after this chain?
>
> I mean the following scenario:
> dst can  be "internal" first but the skb is rerouted to outside
> or vice versa:
> when "external" traffic is re-routed to local netdevices?

1) i do not change the place for accounting and setting mark by this patch,
    so i definitely don't make it worse.

2) i do see packets in venet_acct_out_hook() with "venet0" "out" net device and
    "lo" net device in skb->dst, so at least _some_ rerouting to local netdevice
    is already done up to the moment.

So the only question left - can dst change after NF_INET_LOCAL_OUT netfilter hook?
Cannot find the answer up to the moment, will dig further.

+ i've just found that i forgot to handle ipv6 venet hooks in the similar manner.