netdev - RE: [PATCH net] act_ife: Add support for machines with hard_header_len != mac

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: <DB3PR05MB0764D20F281FA6AEC643D744ACC80@DB3PR05MB0764.eurprd05.prod.outlook.com>
Date:   Fri, 23 Sep 2016 05:49:06 +0000
From:   Yotam Gigi <yotamg@...lanox.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Roman Mashak <mrv@...atatu.com>
Subject: RE: [PATCH net] act_ife: Add support for machines with
 hard_header_len != mac_len

>-----Original Message-----
>From: Jamal Hadi Salim [mailto:jhs@...atatu.com]
>Sent: Friday, September 23, 2016 1:40 AM
>To: Yotam Gigi <yotamg@...lanox.com>; davem@...emloft.net;
>netdev@...r.kernel.org; Roman Mashak <mrv@...atatu.com>
>Subject: Re: [PATCH net] act_ife: Add support for machines with hard_header_len
>!= mac_len
>
>On 16-09-21 08:54 AM, Yotam Gigi wrote:
>> Without that fix, the following could occur:
>>  - On encode ingress, the total amount of skb_pushes (in lines 751 and
>>    753) was more than specified in cow.
>>  - On machines with hard_header_len > mac_len, the packet format was not
>
>Just curious: What hardware would this be?

On mlxsw, in order to send a packet there needed to be added small tx header. In 
order to tell the kernel to reserve that space in the allocated skb, we set that 
hard_header_len field to include the tx header in addition to the mac header.

>
>
>> Fixes: ef6980b6becb ("net sched: introduce IFE action")
>> Signed-off-by: Yotam Gigi <yotamg@...lanox.com>
>> ---
>>  net/sched/act_ife.c | 34 +++++++++++++++++++++++++---------
>>  1 file changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
>> index e87cd81..27b19ca 100644
>> --- a/net/sched/act_ife.c
>> +++ b/net/sched/act_ife.c
>> @@ -708,11 +708,13 @@ static int tcf_ife_encode(struct sk_buff *skb, const
>struct tc_action *a,
>>  	   where ORIGDATA = original ethernet header ...
>>  	 */
>>  	u16 metalen = ife_get_sz(skb, ife);
>> -	int hdrm = metalen + skb->dev->hard_header_len + IFE_METAHDRLEN;
>> -	unsigned int skboff = skb->dev->hard_header_len;
>>  	u32 at = G_TC_AT(skb->tc_verd);
>> -	int new_len = skb->len + hdrm;
>>  	bool exceed_mtu = false;
>> +	unsigned int skboff;
>> +	int total_push;
>> +	int reserve;
>> +	int new_len;
>> +	int hdrm;
>>  	int err;
>>
>>  	if (at & AT_EGRESS) {
>> @@ -724,6 +726,22 @@ static int tcf_ife_encode(struct sk_buff *skb, const struct
>tc_action *a,
>>  	bstats_update(&ife->tcf_bstats, skb);
>>  	tcf_lastuse_update(&ife->tcf_tm);
>>
>> +	if (at & AT_EGRESS) {
>> +		/* on egress, reserve space for hard_header_len instead of
>> +		 * mac_len
>> +		 */
>> +		skb_reset_mac_len(skb);
>
>The skb_reset_mac_len() above is unneeded.
>
>> +		hdrm = metalen + skb->mac_len + IFE_METAHDRLEN;
>
>Can you move this line outside of the if? It appears on the else
>so factoring it out is useful.
>
>> +		total_push = hdrm;
>> +		reserve = metalen + skb->dev->hard_header_len +
>IFE_METAHDRLEN;
>> +	} else {
>> +		/* on ingress, push mac_len as it already get parsed from tc */
>> +		hdrm = metalen + skb->mac_len + IFE_METAHDRLEN;
>> +		total_push = hdrm + skb->mac_len;
>> +		reserve = total_push;
>> +	}
>> +	new_len =  skb->len + hdrm;
>> +
>>  	if (!metalen) {		/* no metadata to send */
>>  		/* abuse overlimits to count when we allow packet
>>  		 * with no metadata
>> @@ -742,19 +760,17 @@ static int tcf_ife_encode(struct sk_buff *skb, const
>struct tc_action *a,
>>
>>  	iethh = eth_hdr(skb);
>>
>> -	err = skb_cow_head(skb, hdrm);
>> +	err = skb_cow_head(skb, reserve);
>>  	if (unlikely(err)) {
>>  		ife->tcf_qstats.drops++;
>>  		spin_unlock(&ife->tcf_lock);
>>  		return TC_ACT_SHOT;
>>  	}
>>
>> -	if (!(at & AT_EGRESS))
>> -		skb_push(skb, skb->dev->hard_header_len);
>> -
>> -	__skb_push(skb, hdrm);
>> +	__skb_push(skb, total_push);
>>  	memcpy(skb->data, iethh, skb->mac_len);
>>  	skb_reset_mac_header(skb);
>> +	skboff += skb->mac_len;
>
>Above looks dangerous. Did the compiler not warn?
>Maybe init skboff to skb->mac_len at the top.

That's look weird. I will fix it and repost in the next couple of days.

>
>Otherwise the ingress bits look good. Thanks!
>
>Please fix above and resend with:
>Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
>
>cheers,
>jamal