netdev - Re: [PATCH v2 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <17d6d359-8809-0fe7-9880-39fb24cf95ac@mojatatu.com>
Date:   Mon, 28 May 2018 12:01:01 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "Fu, Qiaobin" <qiaobinf@...edu>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Michel Machado <michel@...irati.com.br>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
Subject: Re: [PATCH v2 net-next] net:sched: add action inheritdsfield to
 skbedit

On 28/05/18 11:42 AM, Marcelo Ricardo Leitner wrote:
> On Mon, May 28, 2018 at 05:40:18AM +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.
>>
>> Original idea by Jamal Hadi Salim <jhs@...atatu.com>
>>
>> Signed-off-by: Qiaobin Fu <qiaobinf@...edu>
>> Reviewed-by: Michel Machado <michel@...irati.com.br>
>> ---
>>
>> Note that the motivation for this patch is found in the following discussion:
>> https://www.spinics.net/lists/netdev/msg501061.html
>> ---
>>
>> diff --git a/include/uapi/linux/tc_act/tc_skbedit.h b/include/uapi/linux/tc_act/tc_skbedit.h
>> index fbcfe27..432ad2f 100644
>> --- a/include/uapi/linux/tc_act/tc_skbedit.h
>> +++ b/include/uapi/linux/tc_act/tc_skbedit.h
>> @@ -30,9 +30,11 @@
>>   #define SKBEDIT_F_MARK			0x4
>>   #define SKBEDIT_F_PTYPE			0x8
>>   #define SKBEDIT_F_MASK			0x10
>> +#define SKBEDIT_F_INHERITDSFIELD	0x20
>>   
>>   struct tc_skbedit {
>>   	tc_gen;
>> +	__u64 flags;
> 
> I don't think this is doable. It looks like it was prepared for such
> change, but it breaks UAPI as it causes tc without the respective
> patch to not be able to talk to skbedit anymore:
> 
> With this patch:
> [root@f28 ~]# tc action add action skbedit priority 1
> RTNETLINK answers: Numerical result out of range
> We have an error talking to the kernel
> [root@f28 ~]#
> 
> While without this patch:
> [root@f28 ~]# tc action add action skbedit priority 1
> [root@f28 ~]#

And the new flags field doesnt even seem to be in use.
Qiaobin, you dont seem to need this.
Also you have added a workaround for default_priority
which I dont think is needed either (sounds like you
want to make sure that SKBEDIT_F_PRIORITY and new
feature are mutually exclusive - which seems unnecessary.
My suggestion is to remove:
+	} else {
+default_priority:
+		if (d->flags & SKBEDIT_F_PRIORITY)
+			skb->priority = d->priority;
+	}

and leave alone:
-	if (d->flags & SKBEDIT_F_PRIORITY)
-		skb->priority = d->priority;

If someone wants to set both flags, then let them.

cheers,
jamal