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] [day] [month] [year] [list]

Message-ID: <4bfc7a0f380c452aa2f88886feed34b1@nokia-sbell.com>
Date:   Tue, 20 Mar 2018 06:11:07 +0000
From:   "Yuan, Linyu (NSB - CN/Shanghai)" <linyu.yuan@...ia-sbell.com>
To:     Yonghong Song <yhs@...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

Sorry, I should not add "Here cause next BUG_ON always false."
It cause misunderstanding, I just comment on BUG_ON in else branch.

> -----Original Message-----
> From: Yonghong Song [mailto:yhs@...com]
> Sent: Tuesday, March 20, 2018 1:54 PM
> To: Yuan, Linyu (NSB - CN/Shanghai); edumazet@...gle.com; ast@...com;
> daniel@...earbox.net; diptanu@...com; netdev@...r.kernel.org
> Cc: 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
> >