netdev - Re: Is it a possible bug in dev_gro

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:	Tue, 3 Aug 2010 06:45:15 +0000
From:	Jarek Poplawski <jarkao2@...il.com>
To:	"Xin, Xiaohui" <xiaohui.xin@...el.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
	"davem@...emloft.net" <davem@...emloft.net>
Subject: Re: Is it a possible bug in dev_gro_receive()?

On Tue, Aug 03, 2010 at 10:33:24AM +0800, Xin, Xiaohui wrote:
> >-----Original Message-----
> >From: Jarek Poplawski [mailto:jarkao2@...il.com]
> >Sent: Monday, August 02, 2010 6:29 PM
> >To: Xin, Xiaohui
> >Cc: netdev@...r.kernel.org; herbert@...dor.apana.org.au; davem@...emloft.net
> >Subject: Re: Is it a possible bug in dev_gro_receive()?
> >
> >Xin Xiaohui wrote:
> >> I looked into the code dev_gro_receive(), found the code here:
> >> if the frags[0] is pulled to 0, then the page will be released,
> >> and memmove() frags left.
> >> Is that right? I'm not sure if memmove do right or not, but
> >> frags[0].size is never set after memove at least. what I think
> >> a simple way is not to do anything if we found frags[0].size == 0.
> >> The patch is as followed.
> >>
> >> Or am I missing something here?
> >
> >I think, you're right, but fixing memmove looks nicer to me:
> >
> > -	--skb_shinfo(skb)->nr_frags);
> > +	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
> >
> >Jarek P.
> 
> Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is large?
> We're now working on the zero-copy patches based on napi_gro_frags() interface, and in 
> this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove is
> fixed, each frags[x].size is needed to modify too.
> So I think don't do anything is better. Or is there any side effect with a null page in the stack?

Even if it's better, generally you should separate fixes from
optimizations. On the other hand, it was expected to be "unlikely" by
design, so you should probably explain more why it has to be changed
here too.

Thanks,
Jarek P.

> 
> Thanks
> Xiaohui
> >
> >>
> >> ---
> >>  net/core/dev.c |    7 -------
> >>  1 files changed, 0 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 264137f..28cdbbf 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -2730,13 +2730,6 @@ pull:
> >>
> >>  		skb_shinfo(skb)->frags[0].page_offset += grow;
> >>  		skb_shinfo(skb)->frags[0].size -= grow;
> >> -
> >> -		if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
> >> -			put_page(skb_shinfo(skb)->frags[0].page);
> >> -			memmove(skb_shinfo(skb)->frags,
> >> -				skb_shinfo(skb)->frags + 1,
> >> -				--skb_shinfo(skb)->nr_frags);
> >> -		}
> >>  	}
> >>
> >>  ok:
> >
> >
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html