netdev - Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to skbedit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Jun 2018 12:47:18 -0400
From:   Michel Machado <michel@...irati.com.br>
To:     Davide Caratti <dcaratti@...hat.com>,
        "Fu, Qiaobin" <qiaobinf@...edu>,
        "davem@...emloft.net" <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
Subject: Re: [PATCH v4 net-next] net:sched: add action inheritdsfield to
 skbedit

On 06/20/2018 12:08 PM, Davide Caratti wrote:
> On Tue, 2018-06-12 at 15:42 +0000, Fu, Qiaobin wrote:
>> The new action inheritdsfield copies the field DS of
>> IPv4 and IPv6 packets into skb->priority. This enables
>> later classification of packets based on the DS field.
>>
>> v4:
>> *Not allow setting flags other than the expected ones.
>>
>> *Allow dumping the pure flags.
>>
>> Original idea by Jamal Hadi Salim <jhs@...atatu.com>
>>
>> Signed-off-by: Qiaobin Fu <qiaobinf@...edu>
>> Reviewed-by: Michel Machado <michel@...irati.com.br>
>> ---
>>
> 
> [...]
> 
>> @@ -41,6 +44,25 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
>>   
>>   	if (d->flags & SKBEDIT_F_PRIORITY)
>>   		skb->priority = d->priority;
>> +	if (d->flags & SKBEDIT_F_INHERITDSFIELD) {
>> +		int wlen = skb_network_offset(skb);
>> +
>> +		switch (tc_skb_protocol(skb)) {
>> +		case htons(ETH_P_IP):
>> +			wlen += sizeof(struct iphdr);
>> +			if (!pskb_may_pull(skb, wlen))
>> +				goto err;
>> +			skb->priority = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
>> +			break;
>> +
>> +		case htons(ETH_P_IPV6):
>> +			wlen += sizeof(struct ipv6hdr);
>> +			if (!pskb_may_pull(skb, wlen))
>> +				goto err;
>> +			skb->priority = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
>> +			break;
>> +		}
>> +	}
>>   	if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
>>   	    skb->dev->real_num_tx_queues > d->queue_mapping)
>>   		skb_set_queue_mapping(skb, d->queue_mapping);
>> @@ -53,6 +75,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
>>   
>>   	spin_unlock(&d->tcf_lock);
>>   	return d->tcf_action;
>> +
>> +err:
>> +	spin_unlock(&d->tcf_lock);
>> +	return TC_ACT_SHOT;
>>   }
>>   
> 
> sorry for asking this when the patch is a v4...
> 
> I spotted this, as I'm rebasing a small series that removes the tcf_lock
> from the data plane of skbedit to gain some speed, and it converts the
> stats to be per-cpu counters.
> 
> in the code above, you are catching failures of pskb_may_pull(skb) and
> then you return TC_ACT_SHOT. That's OK, but I think you should update the
> drop counter, like other TC actions do.
> 
> If you (author / reviewers) think this is a minor issue, like I do think,
> then I can add the missing update in my series and post it when net-next
> reopens.
> 
> WDYT?
> 
> thank you in advance!
> regards,
> 

Hi Davide,

    I agree that we should update the drop counter, but given that 
you're already converting the stats to be per-cpu counters, whatever we 
add now will be just symbolic since you're going to change it anyway. If 
reviewers think that Qiaobin's patch must add the update line, could you 
provide the exact line and location so we avoid going to v6 of this patch?

[ ]'s
Michel Machado