[PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[Chenbo Feng]

From: Chenbo Feng <fengc@google.com>

Move the initialization of skb->dev and skb->protocol from
ip6_finish_output2 to ip6_output. This can make the skb->dev and
skb->protocol information avalaible to the CGROUP eBPF filter.

Signed-off-by: Chenbo Feng <fengc@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/ip6_output.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index bf8a58a..02cd44f 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -67,9 +67,6 @@ static int ip6_finish_output2(struct net *net, struct sock *sk, struct sk_buff *
 	struct in6_addr *nexthop;
 	int ret;
 
-	skb->protocol = htons(ETH_P_IPV6);
-	skb->dev = dev;
-
 	if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr)) {
 		struct inet6_dev *idev = ip6_dst_idev(skb_dst(skb));
 
@@ -154,6 +151,9 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 	struct net_device *dev = skb_dst(skb)->dev;
 	struct inet6_dev *idev = ip6_dst_idev(skb_dst(skb));
 
+	skb->protocol = htons(ETH_P_IPV6);
+	skb->dev = dev;
+
 	if (unlikely(idev->cnf.disable_ipv6)) {
 		IP6_INC_STATS(net, idev, IPSTATS_MIB_OUTDISCARDS);
 		kfree_skb(skb);
-- 
2.7.4

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[David Miller]

From: Chenbo Feng <chenbofeng.kernel@gmail.com>
Date: Fri,  9 Jun 2017 12:06:07 -0700

> From: Chenbo Feng <fengc@google.com>
> 
> Move the initialization of skb->dev and skb->protocol from
> ip6_finish_output2 to ip6_output. This can make the skb->dev and
> skb->protocol information avalaible to the CGROUP eBPF filter.
> 
> Signed-off-by: Chenbo Feng <fengc@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>

Applied, thanks.

This makes ipv6 consistent with ipv4.

I am surprised this wasn't noticed, for example, in netfilter.

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[Chenbo Feng]

On 06/09/2017 12:08 PM, David Miller wrote:
> From: Chenbo Feng <chenbofeng.kernel@gmail.com>
> Date: Fri,  9 Jun 2017 12:06:07 -0700
>
>> From: Chenbo Feng <fengc@google.com>
>>
>> Move the initialization of skb->dev and skb->protocol from
>> ip6_finish_output2 to ip6_output. This can make the skb->dev and
>> skb->protocol information avalaible to the CGROUP eBPF filter.
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>> Acked-by: Eric Dumazet <edumazet@google.com>
> Applied, thanks.
>
> This makes ipv6 consistent with ipv4.
>
> I am surprised this wasn't noticed, for example, in netfilter.
> .
>
Hi David,

This patch is still under working since it may have problem with 
ip_fragment() call, did you applied it already? Should I send a revert 
patch to you then?

Chenbo Feng

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[Bjørn Mork]

Chenbo Feng <chenbofeng.kernel@gmail.com> writes:

> This patch is still under working since it may have problem with
> ip_fragment() call, did you applied it already? Should I send a revert
> patch to you then?

It does? I initially thought so too, but looking closer I believe the
ip6_copy_metadata() calls in ip6_fragment() takes care of it.



Bjørn

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[David Miller]

From: Chenbo Feng <fengc@google.com>
Date: Fri, 9 Jun 2017 12:08:39 -0700

> Sorry, this is the wrong patch, please ignore it.

:-/ already applied it.

You must now send a relative fixup patch.

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[David Miller]

From: Chenbo Feng <chenbofeng.kernel@gmail.com>
Date: Fri, 9 Jun 2017 12:13:57 -0700

> 
> 
> On 06/09/2017 12:08 PM, David Miller wrote:
>> From: Chenbo Feng <chenbofeng.kernel@gmail.com>
>> Date: Fri,  9 Jun 2017 12:06:07 -0700
>>
>>> From: Chenbo Feng <fengc@google.com>
>>>
>>> Move the initialization of skb->dev and skb->protocol from
>>> ip6_finish_output2 to ip6_output. This can make the skb->dev and
>>> skb->protocol information avalaible to the CGROUP eBPF filter.
>>>
>>> Signed-off-by: Chenbo Feng <fengc@google.com>
>>> Acked-by: Eric Dumazet <edumazet@google.com>
>> Applied, thanks.
>>
>> This makes ipv6 consistent with ipv4.
>>
>> I am surprised this wasn't noticed, for example, in netfilter.
>> .
>>
> Hi David,
> 
> This patch is still under working since it may have problem with
> ip_fragment() call, did you applied it already? Should I send a revert
> patch to you then?

A revert is necessary or a relative fixup.

Thank you.

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[Chenbo Feng]

On 06/09/2017 12:24 PM, Bjørn Mork wrote:
> Chenbo Feng <chenbofeng.kernel@gmail.com> writes:
>
>> This patch is still under working since it may have problem with
>> ip_fragment() call, did you applied it already? Should I send a revert
>> patch to you then?
> It does? I initially thought so too, but looking closer I believe the
> ip6_copy_metadata() calls in ip6_fragment() takes care of it.
>
>
>
> Bjørn
>
At least in the fail_toobig code path of ip_fragment() call, skb->dev 
get assigned again. It seems to be redundant with this patch or it will 
rewrite the skb->dev field. I will revert this one and upload again 
after I have a proper handle for that.

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[Eric Dumazet]

On Fri, Jun 9, 2017 at 12:06 PM, Chenbo Feng
<chenbofeng.kernel@gmail.com> wrote:
> From: Chenbo Feng <fengc@google.com>
>
> Move the initialization of skb->dev and skb->protocol from
> ip6_finish_output2 to ip6_output. This can make the skb->dev and
> skb->protocol information avalaible to the CGROUP eBPF filter.
>
> Signed-off-by: Chenbo Feng <fengc@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
> ---

Arg, you mixed my Acked-by for your other patch :/

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[Chenbo Feng]

On 06/09/2017 12:39 PM, David Miller wrote:
> From: Chenbo Feng <chenbofeng.kernel@gmail.com>
> Date: Fri, 9 Jun 2017 12:13:57 -0700
>
>>
>> On 06/09/2017 12:08 PM, David Miller wrote:
>>> From: Chenbo Feng <chenbofeng.kernel@gmail.com>
>>> Date: Fri,  9 Jun 2017 12:06:07 -0700
>>>
>>>> From: Chenbo Feng <fengc@google.com>
>>>>
>>>> Move the initialization of skb->dev and skb->protocol from
>>>> ip6_finish_output2 to ip6_output. This can make the skb->dev and
>>>> skb->protocol information avalaible to the CGROUP eBPF filter.
>>>>
>>>> Signed-off-by: Chenbo Feng <fengc@google.com>
>>>> Acked-by: Eric Dumazet <edumazet@google.com>
>>> Applied, thanks.
>>>
>>> This makes ipv6 consistent with ipv4.
>>>
>>> I am surprised this wasn't noticed, for example, in netfilter.
>>> .
>>>
>> Hi David,
>>
>> This patch is still under working since it may have problem with
>> ip_fragment() call, did you applied it already? Should I send a revert
>> patch to you then?
> A revert is necessary or a relative fixup.
>
> Thank you.
>
Hi David,

The revert is uploaded here: http://patchwork.ozlabs.org/patch/774136/

Thanks and sorry for the trouble caused

Chenbo Feng

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[Eric Dumazet]

On Fri, 2017-06-09 at 16:12 -0700, Chenbo Feng wrote:
> 
> On 06/09/2017 12:39 PM, David Miller wrote:
> > From: Chenbo Feng <chenbofeng.kernel@gmail.com>
> > Date: Fri, 9 Jun 2017 12:13:57 -0700
> >
> >>
> >> On 06/09/2017 12:08 PM, David Miller wrote:
> >>> From: Chenbo Feng <chenbofeng.kernel@gmail.com>
> >>> Date: Fri,  9 Jun 2017 12:06:07 -0700
> >>>
> >>>> From: Chenbo Feng <fengc@google.com>
> >>>>
> >>>> Move the initialization of skb->dev and skb->protocol from
> >>>> ip6_finish_output2 to ip6_output. This can make the skb->dev and
> >>>> skb->protocol information avalaible to the CGROUP eBPF filter.
> >>>>
> >>>> Signed-off-by: Chenbo Feng <fengc@google.com>
> >>>> Acked-by: Eric Dumazet <edumazet@google.com>
> >>> Applied, thanks.
> >>>
> >>> This makes ipv6 consistent with ipv4.
> >>>
> >>> I am surprised this wasn't noticed, for example, in netfilter.
> >>> .
> >>>
> >> Hi David,
> >>
> >> This patch is still under working since it may have problem with
> >> ip_fragment() call, did you applied it already? Should I send a revert
> >> patch to you then?
> > A revert is necessary or a relative fixup.
> >
> > Thank you.
> >
> Hi David,
> 
> The revert is uploaded here: http://patchwork.ozlabs.org/patch/774136/
> 
> Thanks and sorry for the trouble caused
> 
> Chenbo Feng

No worries !

It seems the revert is not needed, after further analysis.

One of the point I raised was this no longer needed chunk, that can be
added as a separate patch :

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 02cd44f0953900108701895108b2fdaa9f9980e5..0d6f3b6345de26c329ae1d6f25dde652a5452d4b 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -869,7 +869,6 @@ int ip6_fragment(struct net *net, struct sock *sk, struct sk_buff *skb,
 	if (skb->sk && dst_allfrag(skb_dst(skb)))
 		sk_nocaps_add(skb->sk, NETIF_F_GSO_MASK);
 
-	skb->dev = skb_dst(skb)->dev;
 	icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 	err = -EMSGSIZE;
 

Re: [PATCH net-next] ipv6: Initial skb->dev and skb->protocol in ip6_output

[Eric Dumazet]

On Fri, 2017-06-09 at 21:24 +0200, Bjørn Mork wrote:
> Chenbo Feng <chenbofeng.kernel@gmail.com> writes:
> 
> > This patch is still under working since it may have problem with
> > ip_fragment() call, did you applied it already? Should I send a revert
> > patch to you then?
> 
> It does? I initially thought so too, but looking closer I believe the
> ip6_copy_metadata() calls in ip6_fragment() takes care of it.

Indeed, this should be fine.