Why revert commit 2271c95 ("vrf: mark skb for multicast or link-local as enslaved to VRF")?

Why revert commit 2271c95 ("vrf: mark skb for multicast or link-local as enslaved to VRF")?

[Stephen Suryaputra]

Greetings,

We noticed that the commit was reverted after upgrading to v4.14.200.
Any reason why it is reverted? We rely on it.

Thanks,

Stephen.

Re: Why revert commit 2271c95 ("vrf: mark skb for multicast or link-local as enslaved to VRF")?

[David Ahern]

On 10/18/20 7:24 AM, Stephen Suryaputra wrote:
> Greetings,
> 
> We noticed that the commit was reverted after upgrading to v4.14.200.
> Any reason why it is reverted? We rely on it.
> 

$ git show 2271c95
fatal: ambiguous argument '2271c95': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

Re: Why revert commit 2271c95 ("vrf: mark skb for multicast or link-local as enslaved to VRF")?

[Stephen Suryaputra]

On Sun, Oct 18, 2020 at 09:27:16AM -0600, David Ahern wrote:
> On 10/18/20 7:24 AM, Stephen Suryaputra wrote:
> > Greetings,
> > 
> > We noticed that the commit was reverted after upgrading to v4.14.200.
> > Any reason why it is reverted? We rely on it.
> > 
> 
> $ git show 2271c95
> fatal: ambiguous argument '2271c95': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions, like this:
> 'git <command> [<revision>...] -- [<file>...]'

$ git clone git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
$ git checkout v4.14.200
$ git --no-pager show 2271c95

commit 2271c9500434af2a26b2c9eadeb3c0b075409fb5
Author: Mike Manning <mmanning@vyatta.att-mail.com>
Date:   Wed Nov 7 15:36:07 2018 +0000

    vrf: mark skb for multicast or link-local as enslaved to VRF
    
    [ Upstream commit 6f12fa775530195a501fb090d092c637f32d0cc5 ]
    
    The skb for packets that are multicast or to a link-local address are
    not marked as being enslaved to a VRF, if they are received on a socket
    bound to the VRF. This is needed for ND and it is preferable for the
    kernel not to have to deal with the additional use-cases if ll or mcast
    packets are handled as enslaved. However, this does not allow service
    instances listening on unbound and bound to VRF sockets to distinguish
    the VRF used, if packets are sent as multicast or to a link-local
    address. The fix is for the VRF driver to also mark these skb as being
    enslaved to the VRF.
    
    Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
    Reviewed-by: David Ahern <dsahern@gmail.com>
    Tested-by: David Ahern <dsahern@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 03e4fcdfeab7..e0cea5c05f0e 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -996,24 +996,23 @@ static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev,
 				   struct sk_buff *skb)
 {
 	int orig_iif = skb->skb_iif;
-	bool need_strict;
+	bool need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr);
+	bool is_ndisc = ipv6_ndisc_frame(skb);
 
-	/* loopback traffic; do not push through packet taps again.
-	 * Reset pkt_type for upper layers to process skb
+	/* loopback, multicast & non-ND link-local traffic; do not push through
+	 * packet taps again. Reset pkt_type for upper layers to process skb
 	 */
-	if (skb->pkt_type == PACKET_LOOPBACK) {
+	if (skb->pkt_type == PACKET_LOOPBACK || (need_strict && !is_ndisc)) {
 		skb->dev = vrf_dev;
 		skb->skb_iif = vrf_dev->ifindex;
 		IP6CB(skb)->flags |= IP6SKB_L3SLAVE;
-		skb->pkt_type = PACKET_HOST;
+		if (skb->pkt_type == PACKET_LOOPBACK)
+			skb->pkt_type = PACKET_HOST;
 		goto out;
 	}
 
-	/* if packet is NDISC or addressed to multicast or link-local
-	 * then keep the ingress interface
-	 */
-	need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr);
-	if (!ipv6_ndisc_frame(skb) && !need_strict) {
+	/* if packet is NDISC then keep the ingress interface */
+	if (!is_ndisc) {
 		vrf_rx_stats(vrf_dev, skb->len);
 		skb->dev = vrf_dev;
 		skb->skb_iif = vrf_dev->ifindex;

$ git --no-pager show afed1a4

commit afed1a4dbb76c81900f10fd77397fb91ad442702
Author: Sasha Levin <sashal@kernel.org>
Date:   Mon Mar 23 16:21:31 2020 -0400

    Revert "vrf: mark skb for multicast or link-local as enslaved to VRF"
    
    This reverts commit 2271c9500434af2a26b2c9eadeb3c0b075409fb5.
    
    This patch shouldn't have been backported to 4.14.
    
    Signed-off-by: Sasha Levin <sashal@kernel.org>

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index e0cea5c05f0e..03e4fcdfeab7 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -996,23 +996,24 @@ static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev,
 				   struct sk_buff *skb)
 {
 	int orig_iif = skb->skb_iif;
-	bool need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr);
-	bool is_ndisc = ipv6_ndisc_frame(skb);
+	bool need_strict;
 
-	/* loopback, multicast & non-ND link-local traffic; do not push through
-	 * packet taps again. Reset pkt_type for upper layers to process skb
+	/* loopback traffic; do not push through packet taps again.
+	 * Reset pkt_type for upper layers to process skb
 	 */
-	if (skb->pkt_type == PACKET_LOOPBACK || (need_strict && !is_ndisc)) {
+	if (skb->pkt_type == PACKET_LOOPBACK) {
 		skb->dev = vrf_dev;
 		skb->skb_iif = vrf_dev->ifindex;
 		IP6CB(skb)->flags |= IP6SKB_L3SLAVE;
-		if (skb->pkt_type == PACKET_LOOPBACK)
-			skb->pkt_type = PACKET_HOST;
+		skb->pkt_type = PACKET_HOST;
 		goto out;
 	}
 
-	/* if packet is NDISC then keep the ingress interface */
-	if (!is_ndisc) {
+	/* if packet is NDISC or addressed to multicast or link-local
+	 * then keep the ingress interface
+	 */
+	need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr);
+	if (!ipv6_ndisc_frame(skb) && !need_strict) {
 		vrf_rx_stats(vrf_dev, skb->len);
 		skb->dev = vrf_dev;
 		skb->skb_iif = vrf_dev->ifindex;

Re: Why revert commit 2271c95 ("vrf: mark skb for multicast or link-local as enslaved to VRF")?

[David Ahern]

On 10/18/20 10:06 AM, Stephen Suryaputra wrote:
> $ git --no-pager show afed1a4
> 
> commit afed1a4dbb76c81900f10fd77397fb91ad442702
> Author: Sasha Levin <sashal@kernel.org>
> Date:   Mon Mar 23 16:21:31 2020 -0400
> 
>     Revert "vrf: mark skb for multicast or link-local as enslaved to VRF"
>     
>     This reverts commit 2271c9500434af2a26b2c9eadeb3c0b075409fb5.
>     
>     This patch shouldn't have been backported to 4.14.
>     
>     Signed-off-by: Sasha Levin <sashal@kernel.org>
> 

My response last November was:

'backporting this patch and it's bug fix, "ipv6: Fix handling of LLA
with VRF and sockets bound to VRF" to 4.14 is a bit questionable. They
definitely do not need to come back to 4.9.'

Basically, my point is that this is work that was committed to 4.19-next
I believe and given the state of the VRF feature over the releases, I
could not confirm for 4.14 that everything works as intended. Hence, the
comment about it being questionable.

If you / your company are actively using and testing VRF on 4.14 and can
confirm it works, then I am fine with the patch (and its bugfix) getting
applied.

Re: Why revert commit 2271c95 ("vrf: mark skb for multicast or link-local as enslaved to VRF")?

[Mike Manning]

On 19/10/2020 02:53, David Ahern wrote:
> On 10/18/20 10:06 AM, Stephen Suryaputra wrote:
>> $ git --no-pager show afed1a4
>>
>> commit afed1a4dbb76c81900f10fd77397fb91ad442702
>> Author: Sasha Levin <sashal@kernel.org>
>> Date:   Mon Mar 23 16:21:31 2020 -0400
>>
>>     Revert "vrf: mark skb for multicast or link-local as enslaved to VRF"
>>     
>>     This reverts commit 2271c9500434af2a26b2c9eadeb3c0b075409fb5.
>>     
>>     This patch shouldn't have been backported to 4.14.
>>     
>>     Signed-off-by: Sasha Levin <sashal@kernel.org>
>>
> My response last November was:
>
> 'backporting this patch and it's bug fix, "ipv6: Fix handling of LLA
> with VRF and sockets bound to VRF" to 4.14 is a bit questionable. They
> definitely do not need to come back to 4.9.'
>
> Basically, my point is that this is work that was committed to 4.19-next
> I believe and given the state of the VRF feature over the releases, I
> could not confirm for 4.14 that everything works as intended. Hence, the
> comment about it being questionable.
>
> If you / your company are actively using and testing VRF on 4.14 and can
> confirm it works, then I am fine with the patch (and its bugfix) getting
> applied.

Hi,

This fix is part of a series "vrf: allow simultaneous service instances
in default and other VRFs" that is present in 5.x kernels and should not
be used in isolation.

But it was at a later stage erroneously backported as a standalone fix
(without the rest of the series) to 4.14 and 4.19.

So it was reverted from these kernels, especially as it was causing this
regression:

VRF: All router multicast entry(FF02:2) not added to VRF Dev but added
on VLAN Dev

Sorry for any inconvenience.

Thanks, Mike

Re: Why revert commit 2271c95 ("vrf: mark skb for multicast or link-local as enslaved to VRF")?

[Mike Manning]

On 19/10/2020 13:04, Mike Manning wrote:
> On 19/10/2020 02:53, David Ahern wrote:
>> On 10/18/20 10:06 AM, Stephen Suryaputra wrote:
>>> $ git --no-pager show afed1a4
>>>
>>> commit afed1a4dbb76c81900f10fd77397fb91ad442702
>>> Author: Sasha Levin <sashal@kernel.org>
>>> Date:   Mon Mar 23 16:21:31 2020 -0400
>>>
>>>     Revert "vrf: mark skb for multicast or link-local as enslaved to VRF"
>>>     
>>>     This reverts commit 2271c9500434af2a26b2c9eadeb3c0b075409fb5.
>>>     
>>>     This patch shouldn't have been backported to 4.14.
>>>     
>>>     Signed-off-by: Sasha Levin <sashal@kernel.org>
>>>
>> My response last November was:
>>
>> 'backporting this patch and it's bug fix, "ipv6: Fix handling of LLA
>> with VRF and sockets bound to VRF" to 4.14 is a bit questionable. They
>> definitely do not need to come back to 4.9.'
>>
>> Basically, my point is that this is work that was committed to 4.19-next
>> I believe and given the state of the VRF feature over the releases, I
>> could not confirm for 4.14 that everything works as intended. Hence, the
>> comment about it being questionable.
>>
>> If you / your company are actively using and testing VRF on 4.14 and can
>> confirm it works, then I am fine with the patch (and its bugfix) getting
>> applied.
> Hi,
>
> This fix is part of a series "vrf: allow simultaneous service instances
> in default and other VRFs" that is present in 5.x kernels and should not
> be used in isolation.
>
> But it was at a later stage erroneously backported as a standalone fix
> (without the rest of the series) to 4.14 and 4.19.
>
> So it was reverted from these kernels, especially as it was causing this
> regression:
>
> VRF: All router multicast entry(FF02:2) not added to VRF Dev but added
> on VLAN Dev
>
> Sorry for any inconvenience.
>
> Thanks, Mike
>
>
>
To clarify, the regression in 4.14 only occurred when the commit was
used in isolation, not when applied with the rest of the series.

It may be worth mentioning that we had been extensively using the series
in our local fork with 4.14 & 4.19 kernels before proceeding with
submitting the series and then switching to 5.x kernel, so that may be
an approach you can take.

Re: Why revert commit 2271c95 ("vrf: mark skb for multicast or link-local as enslaved to VRF")?

[Stephen Suryaputra]

On Mon, Oct 19, 2020 at 01:24:26PM +0100, Mike Manning wrote:
> To clarify, the regression in 4.14 only occurred when the commit was
> used in isolation, not when applied with the rest of the series.
> 
> It may be worth mentioning that we had been extensively using the series
> in our local fork with 4.14 & 4.19 kernels before proceeding with
> submitting the series and then switching to 5.x kernel, so that may be
> an approach you can take.
> 
Thanks Mike and David. Yes, I think it's best to use it in our local
4.14 fork.

Regards,
Stephen.