[PATCH net-next] net: PIM register decapsulation and Forwarding.

[PATCH net-next] net: PIM register decapsulation and Forwarding.

[Saranya_PL]

PIM register packet is decapsulated but not forwarded in RP

__pim_rcv decapsulates the PIM register packet and reinjects for forwarding
after replacing the skb->dev to reg_dev (vif with VIFF_Register)

Ideally the incoming device should be same as skb->dev where the
original PIM register packet is received. mcache would not have
reg_vif as IIF. Decapsulated packet forwarding is failing
because of IIF mismatch. In RP for this S,G RPF interface would be
skb->dev vif only, so that would be IIF for the cache entry.

Signed-off-by: Saranya_PL <plsaranya@gmail.com>
---
 net/ipv4/ipmr.c  | 2 +-
 net/ipv6/ip6mr.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 13e6329784fb..7b9586335fb7 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -598,7 +598,7 @@ static int __pim_rcv(struct mr_table *mrt, struct sk_buff *skb,
 	skb->protocol = htons(ETH_P_IP);
 	skb->ip_summed = CHECKSUM_NONE;
 
-	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
+	skb_tunnel_rx(skb, skb->dev, dev_net(skb->dev));
 
 	netif_rx(skb);
 
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 4e74bc61a3db..147e29a818ca 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -566,7 +566,7 @@ static int pim6_rcv(struct sk_buff *skb)
 	skb->protocol = htons(ETH_P_IPV6);
 	skb->ip_summed = CHECKSUM_NONE;
 
-	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
+	skb_tunnel_rx(skb, skb->dev, net);
 
 	netif_rx(skb);
 
-- 
2.20.1

Re: [PATCH net-next] net: PIM register decapsulation and Forwarding.

[Jakub Kicinski]

On Thu, 12 May 2022 00:01:38 -0700 Saranya_PL wrote:
> PIM register packet is decapsulated but not forwarded in RP
> 
> __pim_rcv decapsulates the PIM register packet and reinjects for forwarding
> after replacing the skb->dev to reg_dev (vif with VIFF_Register)
> 
> Ideally the incoming device should be same as skb->dev where the
> original PIM register packet is received. mcache would not have
> reg_vif as IIF. Decapsulated packet forwarding is failing
> because of IIF mismatch. In RP for this S,G RPF interface would be
> skb->dev vif only, so that would be IIF for the cache entry.
> 
> Signed-off-by: Saranya_PL <plsaranya@gmail.com>

You need to use your real, full name for the sign-off, and please make
sure you CC maintainers (scripts/get_maintainer.pl is your friend).

[patch netdev] net: PIM register decapsulation and Forwarding.

[Saranya Panjarathina]

From: Saranya_PL <plsaranya@gmail.com>

PIM register packet is decapsulated but not forwarded in RP

__pim_rcv decapsulates the PIM register packet and reinjects for forwarding
after replacing the skb->dev to reg_dev (vif with VIFF_Register)

Ideally the incoming device should be same as skb->dev where the
original PIM register packet is received. mcache would not have
reg_vif as IIF. Decapsulated packet forwarding is failing
because of IIF mismatch. In RP for this S,G RPF interface would be
skb->dev vif only, so that would be IIF for the cache entry.

Signed-off-by: Saranya Panjarathina <plsaranya@gmail.com>
---
 net/ipv4/ipmr.c  | 2 +-
 net/ipv6/ip6mr.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 13e6329784fb..7b9586335fb7 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -598,7 +598,7 @@ static int __pim_rcv(struct mr_table *mrt, struct sk_buff *skb,
 	skb->protocol = htons(ETH_P_IP);
 	skb->ip_summed = CHECKSUM_NONE;
 
-	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
+	skb_tunnel_rx(skb, skb->dev, dev_net(skb->dev));
 
 	netif_rx(skb);
 
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 4e74bc61a3db..147e29a818ca 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -566,7 +566,7 @@ static int pim6_rcv(struct sk_buff *skb)
 	skb->protocol = htons(ETH_P_IPV6);
 	skb->ip_summed = CHECKSUM_NONE;
 
-	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
+	skb_tunnel_rx(skb, skb->dev, net);
 
 	netif_rx(skb);
 
-- 
2.20.1

[PATCH net-next] net: PIM register decapsulation and Forwarding.

[Saranya Panjarathina]

PIM register packet is decapsulated but not forwarded in RP

__pim_rcv decapsulates the PIM register packet and reinjects for forwarding
after replacing the skb->dev to reg_dev (vif with VIFF_Register)

Ideally the incoming device should be same as skb->dev where the
original PIM register packet is received. mcache would not have
reg_vif as IIF. Decapsulated packet forwarding is failing
because of IIF mismatch. In RP for this S,G RPF interface would be
skb->dev vif only, so that would be IIF for the cache entry.

Signed-off-by: Saranya Panjarathina <plsaranya@gmail.com>
---
 net/ipv4/ipmr.c  | 2 +-
 net/ipv6/ip6mr.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 13e6329784fb..7b9586335fb7 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -598,7 +598,7 @@ static int __pim_rcv(struct mr_table *mrt, struct sk_buff *skb,
 	skb->protocol = htons(ETH_P_IP);
 	skb->ip_summed = CHECKSUM_NONE;
 
-	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
+	skb_tunnel_rx(skb, skb->dev, dev_net(skb->dev));
 
 	netif_rx(skb);
 
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 4e74bc61a3db..147e29a818ca 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -566,7 +566,7 @@ static int pim6_rcv(struct sk_buff *skb)
 	skb->protocol = htons(ETH_P_IPV6);
 	skb->ip_summed = CHECKSUM_NONE;
 
-	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
+	skb_tunnel_rx(skb, skb->dev, net);
 
 	netif_rx(skb);
 
-- 
2.20.1

Re: [PATCH net-next] net: PIM register decapsulation and Forwarding.

[Jakub Kicinski]

On Mon, 16 May 2022 04:29:06 -0700 Saranya Panjarathina wrote:
> PIM register packet is decapsulated but not forwarded in RP
> 
> __pim_rcv decapsulates the PIM register packet and reinjects for forwarding
> after replacing the skb->dev to reg_dev (vif with VIFF_Register)
> 
> Ideally the incoming device should be same as skb->dev where the
> original PIM register packet is received. mcache would not have
> reg_vif as IIF. Decapsulated packet forwarding is failing
> because of IIF mismatch. In RP for this S,G RPF interface would be
> skb->dev vif only, so that would be IIF for the cache entry.
> 
> Signed-off-by: Saranya Panjarathina <plsaranya@gmail.com>

Not sure if this can cause any trouble. And why it had become 
a problem now, seems like the code has been this way forever.
David? Ido?

> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 13e6329784fb..7b9586335fb7 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -598,7 +598,7 @@ static int __pim_rcv(struct mr_table *mrt, struct sk_buff *skb,
>  	skb->protocol = htons(ETH_P_IP);
>  	skb->ip_summed = CHECKSUM_NONE;
>  
> -	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
> +	skb_tunnel_rx(skb, skb->dev, dev_net(skb->dev));
>  
>  	netif_rx(skb);
>  
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index 4e74bc61a3db..147e29a818ca 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -566,7 +566,7 @@ static int pim6_rcv(struct sk_buff *skb)
>  	skb->protocol = htons(ETH_P_IPV6);
>  	skb->ip_summed = CHECKSUM_NONE;
>  
> -	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
> +	skb_tunnel_rx(skb, skb->dev, net);
>  
>  	netif_rx(skb);
>  

Re: [PATCH net-next] net: PIM register decapsulation and Forwarding.

[Ido Schimmel]

On Tue, May 17, 2022 at 05:10:26PM -0700, Jakub Kicinski wrote:
> On Mon, 16 May 2022 04:29:06 -0700 Saranya Panjarathina wrote:
> > PIM register packet is decapsulated but not forwarded in RP
> > 
> > __pim_rcv decapsulates the PIM register packet and reinjects for forwarding
> > after replacing the skb->dev to reg_dev (vif with VIFF_Register)
> > 
> > Ideally the incoming device should be same as skb->dev where the
> > original PIM register packet is received. mcache would not have
> > reg_vif as IIF. Decapsulated packet forwarding is failing
> > because of IIF mismatch. In RP for this S,G RPF interface would be
> > skb->dev vif only, so that would be IIF for the cache entry.
> > 
> > Signed-off-by: Saranya Panjarathina <plsaranya@gmail.com>
> 
> Not sure if this can cause any trouble. And why it had become 
> a problem now, seems like the code has been this way forever.
> David? Ido?

Trying to understand the problem:

1. The RP has an (*, G) towards the receiver(s) (receiver joins first)
2. The RP receives a PIM Register packet encapsulating the packet from
the source
3. The kernel decapsulates the packet and injects it into the Rx path as
if the packet was received by the pimreg netdev
4. The kernel forwards the packet according to the (*, G) route (no RPF
check)

At the same time, the PIM Register packet should be received by whatever
routing daemon is running in user space via a raw socket for the PIM
protocol. My understanding is that it should cause the RP to send a PIM
Join towards the FHR, causing the FHR to send two copies of each packet
from the source: encapsulated in the PIM Register packet and over the
(S, G) Tree.

If the RP already has an (S, G) route with IIF of skb->dev and the
decapsulated packet is injected into the Rx path via skb->dev, then what
prevents the RP from forwarding the same packet twice towards the
receiver(s)?

I'm not a PIM expert so the above might be nonsense. Anyway, I will
check with someone from the FRR teams who understands PIM better than
me.

> 
> > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> > index 13e6329784fb..7b9586335fb7 100644
> > --- a/net/ipv4/ipmr.c
> > +++ b/net/ipv4/ipmr.c
> > @@ -598,7 +598,7 @@ static int __pim_rcv(struct mr_table *mrt, struct sk_buff *skb,
> >  	skb->protocol = htons(ETH_P_IP);
> >  	skb->ip_summed = CHECKSUM_NONE;
> >  
> > -	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
> > +	skb_tunnel_rx(skb, skb->dev, dev_net(skb->dev));
> >  
> >  	netif_rx(skb);
> >  
> > diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> > index 4e74bc61a3db..147e29a818ca 100644
> > --- a/net/ipv6/ip6mr.c
> > +++ b/net/ipv6/ip6mr.c
> > @@ -566,7 +566,7 @@ static int pim6_rcv(struct sk_buff *skb)
> >  	skb->protocol = htons(ETH_P_IPV6);
> >  	skb->ip_summed = CHECKSUM_NONE;
> >  
> > -	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
> > +	skb_tunnel_rx(skb, skb->dev, net);
> >  
> >  	netif_rx(skb);
> >  
> 

Re: [PATCH net-next] net: PIM register decapsulation and Forwarding.

[Ido Schimmel]

On Wed, May 18, 2022 at 12:08:35PM +0300, Ido Schimmel wrote:
> On Tue, May 17, 2022 at 05:10:26PM -0700, Jakub Kicinski wrote:
> > On Mon, 16 May 2022 04:29:06 -0700 Saranya Panjarathina wrote:
> > > PIM register packet is decapsulated but not forwarded in RP
> > > 
> > > __pim_rcv decapsulates the PIM register packet and reinjects for forwarding
> > > after replacing the skb->dev to reg_dev (vif with VIFF_Register)
> > > 
> > > Ideally the incoming device should be same as skb->dev where the
> > > original PIM register packet is received. mcache would not have
> > > reg_vif as IIF. Decapsulated packet forwarding is failing
> > > because of IIF mismatch. In RP for this S,G RPF interface would be
> > > skb->dev vif only, so that would be IIF for the cache entry.
> > > 
> > > Signed-off-by: Saranya Panjarathina <plsaranya@gmail.com>
> > 
> > Not sure if this can cause any trouble. And why it had become 
> > a problem now, seems like the code has been this way forever.
> > David? Ido?
> 
> Trying to understand the problem:
> 
> 1. The RP has an (*, G) towards the receiver(s) (receiver joins first)
> 2. The RP receives a PIM Register packet encapsulating the packet from
> the source
> 3. The kernel decapsulates the packet and injects it into the Rx path as
> if the packet was received by the pimreg netdev
> 4. The kernel forwards the packet according to the (*, G) route (no RPF
> check)
> 
> At the same time, the PIM Register packet should be received by whatever
> routing daemon is running in user space via a raw socket for the PIM
> protocol. My understanding is that it should cause the RP to send a PIM
> Join towards the FHR, causing the FHR to send two copies of each packet
> from the source: encapsulated in the PIM Register packet and over the
> (S, G) Tree.
> 
> If the RP already has an (S, G) route with IIF of skb->dev and the
> decapsulated packet is injected into the Rx path via skb->dev, then what
> prevents the RP from forwarding the same packet twice towards the
> receiver(s)?
> 
> I'm not a PIM expert so the above might be nonsense. Anyway, I will
> check with someone from the FRR teams who understands PIM better than
> me.

We discussed this patch in FRR slack with the author and PIM experts.
The tl;dr is that the patch is working around what we currently believe
is an FRR bug, which the author will try to fix.

After receiving a PIM Register message on the RP, FRR installs an (S, G)
route with IIF being the interface via which the packet was received
(skb->dev). FRR also sends a PIM Join towards the FHR and eventually a
PIM Register Stop.

The current behavior means that due to RPF assertion, all the
encapsulated traffic from the source is dropped on the RP after FRR
installs the (S, G) route.

The patch is problematic because during the time the FHR sends both
encapsulated and native traffic towards the RP, the RP will forward both
copies towards the receiver(s).

Instead, the suggestion is for FRR to install the initial (S, G) route
with IIF being the pimreg device. This should allow decapsulated traffic
to be forwarded correctly. Native traffic will trigger RPF assertion and
thereby prompt FRR to: a) Replace the IIF from pimreg to the one via
which traffic is received b) Send a PIM Register Stop towards the FHR,
instructing it to stop sending encapsulated traffic.

> 
> > 
> > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> > > index 13e6329784fb..7b9586335fb7 100644
> > > --- a/net/ipv4/ipmr.c
> > > +++ b/net/ipv4/ipmr.c
> > > @@ -598,7 +598,7 @@ static int __pim_rcv(struct mr_table *mrt, struct sk_buff *skb,
> > >  	skb->protocol = htons(ETH_P_IP);
> > >  	skb->ip_summed = CHECKSUM_NONE;
> > >  
> > > -	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
> > > +	skb_tunnel_rx(skb, skb->dev, dev_net(skb->dev));
> > >  
> > >  	netif_rx(skb);
> > >  
> > > diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> > > index 4e74bc61a3db..147e29a818ca 100644
> > > --- a/net/ipv6/ip6mr.c
> > > +++ b/net/ipv6/ip6mr.c
> > > @@ -566,7 +566,7 @@ static int pim6_rcv(struct sk_buff *skb)
> > >  	skb->protocol = htons(ETH_P_IPV6);
> > >  	skb->ip_summed = CHECKSUM_NONE;
> > >  
> > > -	skb_tunnel_rx(skb, reg_dev, dev_net(reg_dev));
> > > +	skb_tunnel_rx(skb, skb->dev, net);
> > >  
> > >  	netif_rx(skb);
> > >  
> > 

Re: [PATCH net-next] net: PIM register decapsulation and Forwarding.

[David Ahern]

On 5/18/22 8:16 AM, Ido Schimmel wrote:
> On Wed, May 18, 2022 at 12:08:35PM +0300, Ido Schimmel wrote:
>> On Tue, May 17, 2022 at 05:10:26PM -0700, Jakub Kicinski wrote:
>>> On Mon, 16 May 2022 04:29:06 -0700 Saranya Panjarathina wrote:
>>>> PIM register packet is decapsulated but not forwarded in RP
>>>>
>>>> __pim_rcv decapsulates the PIM register packet and reinjects for forwarding
>>>> after replacing the skb->dev to reg_dev (vif with VIFF_Register)
>>>>
>>>> Ideally the incoming device should be same as skb->dev where the
>>>> original PIM register packet is received. mcache would not have
>>>> reg_vif as IIF. Decapsulated packet forwarding is failing
>>>> because of IIF mismatch. In RP for this S,G RPF interface would be
>>>> skb->dev vif only, so that would be IIF for the cache entry.
>>>>
>>>> Signed-off-by: Saranya Panjarathina <plsaranya@gmail.com>
>>>
>>> Not sure if this can cause any trouble. And why it had become 
>>> a problem now, seems like the code has been this way forever.
>>> David? Ido?
>>
>> Trying to understand the problem:
>>
>> 1. The RP has an (*, G) towards the receiver(s) (receiver joins first)
>> 2. The RP receives a PIM Register packet encapsulating the packet from
>> the source
>> 3. The kernel decapsulates the packet and injects it into the Rx path as
>> if the packet was received by the pimreg netdev
>> 4. The kernel forwards the packet according to the (*, G) route (no RPF
>> check)
>>
>> At the same time, the PIM Register packet should be received by whatever
>> routing daemon is running in user space via a raw socket for the PIM
>> protocol. My understanding is that it should cause the RP to send a PIM
>> Join towards the FHR, causing the FHR to send two copies of each packet
>> from the source: encapsulated in the PIM Register packet and over the
>> (S, G) Tree.
>>
>> If the RP already has an (S, G) route with IIF of skb->dev and the
>> decapsulated packet is injected into the Rx path via skb->dev, then what
>> prevents the RP from forwarding the same packet twice towards the
>> receiver(s)?
>>
>> I'm not a PIM expert so the above might be nonsense. Anyway, I will
>> check with someone from the FRR teams who understands PIM better than
>> me.
> 
> We discussed this patch in FRR slack with the author and PIM experts.
> The tl;dr is that the patch is working around what we currently believe
> is an FRR bug, which the author will try to fix.
> 
> After receiving a PIM Register message on the RP, FRR installs an (S, G)
> route with IIF being the interface via which the packet was received
> (skb->dev). FRR also sends a PIM Join towards the FHR and eventually a
> PIM Register Stop.
> 
> The current behavior means that due to RPF assertion, all the
> encapsulated traffic from the source is dropped on the RP after FRR
> installs the (S, G) route.
> 
> The patch is problematic because during the time the FHR sends both
> encapsulated and native traffic towards the RP, the RP will forward both
> copies towards the receiver(s).
> 
> Instead, the suggestion is for FRR to install the initial (S, G) route
> with IIF being the pimreg device. This should allow decapsulated traffic
> to be forwarded correctly. Native traffic will trigger RPF assertion and
> thereby prompt FRR to: a) Replace the IIF from pimreg to the one via
> which traffic is received b) Send a PIM Register Stop towards the FHR,
> instructing it to stop sending encapsulated traffic.
> 

Thanks for diving into the problem and for the detailed response.

Re: [PATCH net-next] net: PIM register decapsulation and Forwarding.

[Jakub Kicinski]

On Wed, 18 May 2022 08:36:26 -0600 David Ahern wrote:
> >> Trying to understand the problem:
> >>
> >> 1. The RP has an (*, G) towards the receiver(s) (receiver joins first)
> >> 2. The RP receives a PIM Register packet encapsulating the packet from
> >> the source
> >> 3. The kernel decapsulates the packet and injects it into the Rx path as
> >> if the packet was received by the pimreg netdev
> >> 4. The kernel forwards the packet according to the (*, G) route (no RPF
> >> check)
> >>
> >> At the same time, the PIM Register packet should be received by whatever
> >> routing daemon is running in user space via a raw socket for the PIM
> >> protocol. My understanding is that it should cause the RP to send a PIM
> >> Join towards the FHR, causing the FHR to send two copies of each packet
> >> from the source: encapsulated in the PIM Register packet and over the
> >> (S, G) Tree.
> >>
> >> If the RP already has an (S, G) route with IIF of skb->dev and the
> >> decapsulated packet is injected into the Rx path via skb->dev, then what
> >> prevents the RP from forwarding the same packet twice towards the
> >> receiver(s)?
> >>
> >> I'm not a PIM expert so the above might be nonsense. Anyway, I will
> >> check with someone from the FRR teams who understands PIM better than
> >> me.  
> > 
> > We discussed this patch in FRR slack with the author and PIM experts.
> > The tl;dr is that the patch is working around what we currently believe
> > is an FRR bug, which the author will try to fix.
> > 
> > After receiving a PIM Register message on the RP, FRR installs an (S, G)
> > route with IIF being the interface via which the packet was received
> > (skb->dev). FRR also sends a PIM Join towards the FHR and eventually a
> > PIM Register Stop.
> > 
> > The current behavior means that due to RPF assertion, all the
> > encapsulated traffic from the source is dropped on the RP after FRR
> > installs the (S, G) route.
> > 
> > The patch is problematic because during the time the FHR sends both
> > encapsulated and native traffic towards the RP, the RP will forward both
> > copies towards the receiver(s).
> > 
> > Instead, the suggestion is for FRR to install the initial (S, G) route
> > with IIF being the pimreg device. This should allow decapsulated traffic
> > to be forwarded correctly. Native traffic will trigger RPF assertion and
> > thereby prompt FRR to: a) Replace the IIF from pimreg to the one via
> > which traffic is received b) Send a PIM Register Stop towards the FHR,
> > instructing it to stop sending encapsulated traffic.
> >   
> 
> Thanks for diving into the problem and for the detailed response.

+1, thanks Ido!