Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1508718 > unrolled thread

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

Started byAbdelrhman Ahmed <ab@abahmed.com>
First post2016-10-26 02:00 +0200
Last post2016-10-26 19:10 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] net: Reset skb to network header in neigh_hh_output Abdelrhman Ahmed <ab@abahmed.com> - 2016-10-26 02:00 +0200
    Re: [PATCH] net: Reset skb to network header in neigh_hh_output Eric Dumazet <eric.dumazet@gmail.com> - 2016-10-26 02:20 +0200
      Re: [PATCH] net: Reset skb to network header in neigh_hh_output Abdelrhman Ahmed <ab@abahmed.com> - 2016-10-26 19:00 +0200
        Re: [PATCH] net: Reset skb to network header in neigh_hh_output Eric Dumazet <eric.dumazet@gmail.com> - 2016-10-26 19:10 +0200

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

FromAbdelrhman Ahmed <ab@abahmed.com>
Date2016-10-26 02:00 +0200
SubjectRe: [PATCH] net: Reset skb to network header in neigh_hh_output
Message-ID<swjNn-3Zo-9@gated-at.bofh.it>
 > 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?



 ---- On Fri, 07 Oct 2016 23:10:56 +0200 Eric Dumazet <eric.dumazet@gmail.com> 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. 
 >  
 >  
 >

[toc] | [next] | [standalone]

#1508724

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-10-26 02:20 +0200
Message-ID<swk6J-4oR-5@gated-at.bofh.it>
In reply to#1508718
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@gmail.com> 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. 
>  >  
>  >  
>  > 
>

[toc] | [prev] | [next] | [standalone]

#1509599

FromAbdelrhman Ahmed <ab@abahmed.com>
Date2016-10-26 19:00 +0200
Message-ID<swzIu-6tK-19@gated-at.bofh.it>
In reply to#1508724
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@gmail.com> 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@gmail.com> 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.  
 > >  >   
 > >  >   
 > >  >  
 > >  
 >  
 >  
 >

[toc] | [prev] | [next] | [standalone]

#1509604

FromEric Dumazet <eric.dumazet@gmail.com>
Date2016-10-26 19:10 +0200
Message-ID<swzS9-6Mc-3@gated-at.bofh.it>
In reply to#1509599
On Wed, 2016-10-26 at 18:53 +0200, Abdelrhman Ahmed wrote:
> 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.

Right you are, thanks for the clarification !

Back to the cause of the bug then.

If netfilter is the only case this might be needed, can't this be fixed
in netfilter ?

neigh_hh_output() is in fast path, it is quite annoying adding this
operation.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web