Re: [PATCH] net: Reset skb to network header in neigh_hh_output

[Abdelrhman Ahmed]

I think it's at the right place as the current one is a little different from the
commit e1f165032c8bade3a6bdf546f8faf61fda4dd01c.

In the next lines, skb_push is called after copying the hardware header and there
is no change to the data pointer inside the retry loop. We only need to reset
before this loop.

__skb_pull(skb, skb_network_offset(skb));

do {
	seq = read_seqbegin(&hh->hh_lock);
	hh_len = hh->hh_len;
	if (likely(hh_len <= HH_DATA_MOD)) {
		/* this is inlined by gcc */
		memcpy(skb->data - HH_DATA_MOD, hh->hh_data, HH_DATA_MOD);
	} else {
		int hh_alen = HH_DATA_ALIGN(hh_len);

		memcpy(skb->data - hh_alen, hh->hh_data, hh_alen);
	}
} while (read_seqretry(&hh->hh_lock, seq));

skb_push(skb, hh_len);

In the commit e1f165032c8bade3a6bdf546f8faf61fda4dd01c, dev_hard_header which
calls create method for adding hardware header (uses skb_push) so it was
required to reset to network header in the beginning of the retry loop.


 ---- On Wed, 26 Oct 2016 02:12:22 +0200 Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote ---- 
 > On Wed, 2016-10-26 at 01:57 +0200, Abdelrhman Ahmed wrote: 
 > >  > What is the issue you want to fix exactly ?  
 > >  > Please describe the use case.  
 > >  
 > > When netfilter hook uses skb_push to add a specific header between network 
 > > header and hardware header. 
 > > For the first time(s) before caching hardware header, this header will be 
 > > removed / overwritten by hardware header due to resetting to network header. 
 > > After using the cached hardware header, this header will be kept as we do not 
 > > reset. I think this behavior is inconsistent, so we need to reset in both cases. 
 > >  
 > >  > Otherwise, your fix is in fact adding a critical bug.  
 > >  
 > > Could you explain more as it's not clear to me? 
 > >  
 >  
 > Maybe my wording was not good here. 
 >  
 > What I intended to say is that the  
 > __skb_pull(skb, skb_network_offset(skb)) might not be at the right 
 > place. 
 >  
 > Look at commit e1f165032c8bade3a6bdf546f8faf61fda4dd01c to find the 
 > reason. 
 >  
 >  
 > >  
 > >  
 > >  ---- On Fri, 07 Oct 2016 23:10:56 +0200 Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote ----  
 > >  > On Fri, 2016-10-07 at 16:14 +0200, Abdelrhman Ahmed wrote:  
 > >  > > When hardware header is added without using cached one, neigh_resolve_output  
 > >  > > and neigh_connected_output reset skb to network header before adding it.  
 > >  > > When cached one is used, neigh_hh_output does not reset the skb to network  
 > >  > > header.  
 > >  > >   
 > >  > > The fix is to reset skb to network header before adding cached hardware header  
 > >  > > to keep the behavior consistent in all cases.  
 > >  >   
 > >  > What is the issue you want to fix exactly ?  
 > >  >   
 > >  > Please describe the use case.  
 > >  >   
 > >  > I highly suggest you take a look at commit  
 > >  >   
 > >  > e1f165032c8bade3a6bdf546f8faf61fda4dd01c  
 > >  > ("net: Fix skb_under_panic oops in neigh_resolve_output")  
 > >  >   
 > >  > Otherwise, your fix is in fact adding a critical bug.  
 > >  >   
 > >  >   
 > >  >  
 > >  
 >  
 >  
 >