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

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

[Abdelrhman Ahmed]

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.

Signed-off-by: Abdelrhman Ahmed <ab@abahmed.com>
---
 include/net/neighbour.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/net/neighbour.h b/include/net/neighbour.h
index 8b68384..4d89fc2 100644
--- a/include/net/neighbour.h
+++ b/include/net/neighbour.h
@@ -424,7 +424,7 @@ static inline struct neighbour * neigh_clone(struct neighbour *neigh)
 static inline int neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
 {
        unsigned long now = jiffies;
-       
+
        if (neigh->used != now)
                neigh->used = now;
        if (!(neigh->nud_state&(NUD_CONNECTED|NUD_DELAY|NUD_PROBE)))
@@ -451,6 +451,8 @@ static inline int neigh_hh_output(const struct hh_cache *hh, struct sk_buff *skb
        unsigned int seq;
        int hh_len;
 
+       __skb_pull(skb, skb_network_offset(skb));
+
        do {
                seq = read_seqbegin(&hh->hh_lock);
                hh_len = hh->hh_len;
-- 
1.9.1

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

[Sergei Shtylyov]

Hello.

On 10/07/2016 05:14 PM, 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.
>
> Signed-off-by: Abdelrhman Ahmed <ab@abahmed.com>
> ---
>  include/net/neighbour.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 8b68384..4d89fc2 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -424,7 +424,7 @@ static inline struct neighbour * neigh_clone(struct neighbour *neigh)
>  static inline int neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
>  {
>         unsigned long now = jiffies;
> -
> +

    Unraleted white-space change.

>         if (neigh->used != now)
>                 neigh->used = now;
>         if (!(neigh->nud_state&(NUD_CONNECTED|NUD_DELAY|NUD_PROBE)))
[...]

MBR, Sergei

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

[Sergei Shtylyov]

On 10/07/2016 07:27 PM, Sergei Shtylyov 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.
>>
>> Signed-off-by: Abdelrhman Ahmed <ab@abahmed.com>
>> ---
>>  include/net/neighbour.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
>> index 8b68384..4d89fc2 100644
>> --- a/include/net/neighbour.h
>> +++ b/include/net/neighbour.h
>> @@ -424,7 +424,7 @@ static inline struct neighbour * neigh_clone(struct
>> neighbour *neigh)
>>  static inline int neigh_event_send(struct neighbour *neigh, struct sk_buff
>> *skb)
>>  {
>>         unsigned long now = jiffies;
>> -
>> +
>
>    Unraleted white-space change.

    And I thought I fixed this word... it's "unrelated" of/c. :-)

>
>>         if (neigh->used != now)
>>                 neigh->used = now;
>>         if (!(neigh->nud_state&(NUD_CONNECTED|NUD_DELAY|NUD_PROBE)))
> [...]

MBR, Sergei

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

[Eric Dumazet]

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.

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

[Abdelrhman Ahmed]

 > 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. 
 >  
 >  
 > 

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

[Eric Dumazet]

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. 
>  >  
>  >  
>  > 
> 

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@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.  
 > >  >   
 > >  >   
 > >  >  
 > >  
 >  
 >  
 > 

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

[Eric Dumazet]

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.