netdev - Re: [PATCH net-next 1/2] net: permit skb_segment on head_frag frag

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]

Date:   Mon, 19 Mar 2018 22:54:03 -0700
From:   Yonghong Song <yhs@...com>
To:     "Yuan, Linyu (NSB - CN/Shanghai)" <linyu.yuan@...ia-sbell.com>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "ast@...com" <ast@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "diptanu@...com" <diptanu@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "kernel-team@...com" <kernel-team@...com>
Subject: Re: [PATCH net-next 1/2] net: permit skb_segment on head_frag
 frag_list skb



On 3/19/18 10:30 PM, Yuan, Linyu (NSB - CN/Shanghai) wrote:
> 
> 
>> -----Original Message-----
>> From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org]
>> On Behalf Of Yonghong Song
>> Sent: Tuesday, March 20, 2018 1:16 PM
>> To: edumazet@...gle.com; ast@...com; daniel@...earbox.net;
>> diptanu@...com; netdev@...r.kernel.org
>> Cc: kernel-team@...com
>> Subject: [PATCH net-next 1/2] net: permit skb_segment on head_frag frag_list
>> skb
>>
>>
>>   		while (pos < offset + len) {
>>   			if (i >= nfrags) {
>> -				BUG_ON(skb_headlen(list_skb));
>> +				if (skb_headlen(list_skb) && check_list_skb == list_skb) {
> Here cause next BUG_ON always false.

The idea is since in this branch, we did not do list_skb = 
list_skb->next. So we update check_list_skb. Next time, when the
control reaches here, list_skb may still be the same, but check_list_skb
is not, so we proceed to process list_skb->frags in the else branch.

In the else branch, we have
    list_skb = list_skb->next;
    check_list_skb = list_skb;

So when the current frags are processed and ready for the list_skb. 
list_skb will be equal to check_list_skb and it will be processed again.

It is a little bit convoluted. Please let me know you have better idea.

>> +				} else {
>> +					BUG_ON(skb_headlen(list_skb) && check_list_skb ==
>> list_skb);
> Just according code logic, no need BUG_ON, right?

Oh, yes, we do not need this. Will remove in the next version.

>>
>> -				i = 0;
>> -				nfrags = skb_shinfo(list_skb)->nr_frags;
>> -				frag = skb_shinfo(list_skb)->frags;
>> -				frag_skb = list_skb;
>> +					i = 0;
>> +					nfrags = skb_shinfo(list_skb)->nr_frags;
>> +					frag = skb_shinfo(list_skb)->frags;
>> +					frag_skb = list_skb;
>>
>> -				BUG_ON(!nfrags);
>> +					BUG_ON(!nfrags);
>>
>> -				if (skb_orphan_frags(frag_skb, GFP_ATOMIC) ||
>> -				    skb_zerocopy_clone(nskb, frag_skb,
>> -						       GFP_ATOMIC))
>> -					goto err;
>> +					if (skb_orphan_frags(frag_skb, GFP_ATOMIC) ||
>> +					    skb_zerocopy_clone(nskb, frag_skb,
>> GFP_ATOMIC))
>> +						goto err;
>>
>> -				list_skb = list_skb->next;
>> +					list_skb = list_skb->next;
>> +					check_list_skb = list_skb;
>> +				}
>>   			}
>>
>>   			if (unlikely(skb_shinfo(nskb)->nr_frags >=
>> --
>> 2.9.5
>