| Message ID | 20210618151553.59456-1-atenart@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 603113c514e95c3350598bc3cccbd03af7ea4ab2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] vrf: do not push non-ND strict packets with a source LLA through packet taps again | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 85 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
On 6/18/21 9:15 AM, Antoine Tenart wrote: > Non-ND strict packets with a source LLA go through the packet taps > again, while non-ND strict packets with other source addresses do not, > and we can see a clone of those packets on the vrf interface (we should > not). This is due to a series of changes: > > Commit 6f12fa775530[1] made non-ND strict packets not being pushed again > in the packet taps. This changed with commit 205704c618af[2] for those > packets having a source LLA, as they need a lookup with the orig_iif. > > The issue now is those packets do not skip the 'vrf_ip6_rcv' function to > the end (as the ones without a source LLA) and go through the check to > call packet taps again. This check was changed by commit 6f12fa775530[1] > and do not exclude non-strict packets anymore. Packets matching > 'need_strict && !is_ndisc && is_ll_src' are now being sent through the > packet taps again. This can be seen by dumping packets on the vrf > interface. > > Fix this by having the same code path for all non-ND strict packets and > selectively lookup with the orig_iif for those with a source LLA. This > has the effect to revert to the pre-205704c618af[2] condition, which > should also be easier to maintain. > > [1] 6f12fa775530 ("vrf: mark skb for multicast or link-local as enslaved to VRF") > [2] 205704c618af ("vrf: packets with lladdr src needs dst at input with orig_iif when needs strict") > > Fixes: 205704c618af ("vrf: packets with lladdr src needs dst at input with orig_iif when needs strict") > Cc: Stephen Suryaputra <ssuryaextr@gmail.com> > Reported-by: Paolo Abeni <pabeni@redhat.com> > Signed-off-by: Antoine Tenart <atenart@kernel.org> > --- > drivers/net/vrf.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c > index 28a6c4cfe9b8..414afcb0a23f 100644 > --- a/drivers/net/vrf.c > +++ b/drivers/net/vrf.c > @@ -1366,22 +1366,22 @@ static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev, > int orig_iif = skb->skb_iif; > bool need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr); > bool is_ndisc = ipv6_ndisc_frame(skb); > - bool is_ll_src; > > /* loopback, multicast & non-ND link-local traffic; do not push through > * packet taps again. Reset pkt_type for upper layers to process skb. > - * for packets with lladdr src, however, skip so that the dst can be > - * determine at input using original ifindex in the case that daddr > - * needs strict > + * For strict packets with a source LLA, determine the dst using the > + * original ifindex. > */ > - is_ll_src = ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL; > - if (skb->pkt_type == PACKET_LOOPBACK || > - (need_strict && !is_ndisc && !is_ll_src)) { > + 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; > + > if (skb->pkt_type == PACKET_LOOPBACK) > skb->pkt_type = PACKET_HOST; > + else if (ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) > + vrf_ip6_input_dst(skb, vrf_dev, orig_iif); > + > goto out; > } > > you are basically moving Stephen's is_ll_src within the need_strict and not ND. Did you run the fcnal-test script and verify no change in test results?
Quoting David Ahern (2021-06-19 03:18:50) > On 6/18/21 9:15 AM, Antoine Tenart wrote: > > --- a/drivers/net/vrf.c > > +++ b/drivers/net/vrf.c > > @@ -1366,22 +1366,22 @@ static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev, > > int orig_iif = skb->skb_iif; > > bool need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr); > > bool is_ndisc = ipv6_ndisc_frame(skb); > > - bool is_ll_src; > > > > /* loopback, multicast & non-ND link-local traffic; do not push through > > * packet taps again. Reset pkt_type for upper layers to process skb. > > - * for packets with lladdr src, however, skip so that the dst can be > > - * determine at input using original ifindex in the case that daddr > > - * needs strict > > + * For strict packets with a source LLA, determine the dst using the > > + * original ifindex. > > */ > > - is_ll_src = ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL; > > - if (skb->pkt_type == PACKET_LOOPBACK || > > - (need_strict && !is_ndisc && !is_ll_src)) { > > + 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; > > + > > if (skb->pkt_type == PACKET_LOOPBACK) > > skb->pkt_type = PACKET_HOST; > > + else if (ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) > > + vrf_ip6_input_dst(skb, vrf_dev, orig_iif); > > + > > goto out; > > } > > you are basically moving Stephen's is_ll_src within the need_strict and > not ND. That's right. > Did you run the fcnal-test script and verify no change in test results? Yes, I saw no regression, and the tests Stephen added were still OK. Antoine
On 6/20/21 6:12 AM, Antoine Tenart wrote: > Quoting David Ahern (2021-06-19 03:18:50) >> On 6/18/21 9:15 AM, Antoine Tenart wrote: >>> --- a/drivers/net/vrf.c >>> +++ b/drivers/net/vrf.c >>> @@ -1366,22 +1366,22 @@ static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev, >>> int orig_iif = skb->skb_iif; >>> bool need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr); >>> bool is_ndisc = ipv6_ndisc_frame(skb); >>> - bool is_ll_src; >>> >>> /* loopback, multicast & non-ND link-local traffic; do not push through >>> * packet taps again. Reset pkt_type for upper layers to process skb. >>> - * for packets with lladdr src, however, skip so that the dst can be >>> - * determine at input using original ifindex in the case that daddr >>> - * needs strict >>> + * For strict packets with a source LLA, determine the dst using the >>> + * original ifindex. >>> */ >>> - is_ll_src = ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL; >>> - if (skb->pkt_type == PACKET_LOOPBACK || >>> - (need_strict && !is_ndisc && !is_ll_src)) { >>> + 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; >>> + >>> if (skb->pkt_type == PACKET_LOOPBACK) >>> skb->pkt_type = PACKET_HOST; >>> + else if (ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) >>> + vrf_ip6_input_dst(skb, vrf_dev, orig_iif); >>> + >>> goto out; >>> } >> >> you are basically moving Stephen's is_ll_src within the need_strict and >> not ND. > > That's right. > >> Did you run the fcnal-test script and verify no change in test results? > > Yes, I saw no regression, and the tests Stephen added were still OK. > > Antoine > Reviewed-by: David Ahern <dsahern@kernel.org>
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 18 Jun 2021 17:15:53 +0200 you wrote: > Non-ND strict packets with a source LLA go through the packet taps > again, while non-ND strict packets with other source addresses do not, > and we can see a clone of those packets on the vrf interface (we should > not). This is due to a series of changes: > > Commit 6f12fa775530[1] made non-ND strict packets not being pushed again > in the packet taps. This changed with commit 205704c618af[2] for those > packets having a source LLA, as they need a lookup with the orig_iif. > > [...] Here is the summary with links: - [net] vrf: do not push non-ND strict packets with a source LLA through packet taps again https://git.kernel.org/netdev/net/c/603113c514e9 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c index 28a6c4cfe9b8..414afcb0a23f 100644 --- a/drivers/net/vrf.c +++ b/drivers/net/vrf.c @@ -1366,22 +1366,22 @@ static struct sk_buff *vrf_ip6_rcv(struct net_device *vrf_dev, int orig_iif = skb->skb_iif; bool need_strict = rt6_need_strict(&ipv6_hdr(skb)->daddr); bool is_ndisc = ipv6_ndisc_frame(skb); - bool is_ll_src; /* loopback, multicast & non-ND link-local traffic; do not push through * packet taps again. Reset pkt_type for upper layers to process skb. - * for packets with lladdr src, however, skip so that the dst can be - * determine at input using original ifindex in the case that daddr - * needs strict + * For strict packets with a source LLA, determine the dst using the + * original ifindex. */ - is_ll_src = ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL; - if (skb->pkt_type == PACKET_LOOPBACK || - (need_strict && !is_ndisc && !is_ll_src)) { + 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; + if (skb->pkt_type == PACKET_LOOPBACK) skb->pkt_type = PACKET_HOST; + else if (ipv6_addr_type(&ipv6_hdr(skb)->saddr) & IPV6_ADDR_LINKLOCAL) + vrf_ip6_input_dst(skb, vrf_dev, orig_iif); + goto out; }
Non-ND strict packets with a source LLA go through the packet taps again, while non-ND strict packets with other source addresses do not, and we can see a clone of those packets on the vrf interface (we should not). This is due to a series of changes: Commit 6f12fa775530[1] made non-ND strict packets not being pushed again in the packet taps. This changed with commit 205704c618af[2] for those packets having a source LLA, as they need a lookup with the orig_iif. The issue now is those packets do not skip the 'vrf_ip6_rcv' function to the end (as the ones without a source LLA) and go through the check to call packet taps again. This check was changed by commit 6f12fa775530[1] and do not exclude non-strict packets anymore. Packets matching 'need_strict && !is_ndisc && is_ll_src' are now being sent through the packet taps again. This can be seen by dumping packets on the vrf interface. Fix this by having the same code path for all non-ND strict packets and selectively lookup with the orig_iif for those with a source LLA. This has the effect to revert to the pre-205704c618af[2] condition, which should also be easier to maintain. [1] 6f12fa775530 ("vrf: mark skb for multicast or link-local as enslaved to VRF") [2] 205704c618af ("vrf: packets with lladdr src needs dst at input with orig_iif when needs strict") Fixes: 205704c618af ("vrf: packets with lladdr src needs dst at input with orig_iif when needs strict") Cc: Stephen Suryaputra <ssuryaextr@gmail.com> Reported-by: Paolo Abeni <pabeni@redhat.com> Signed-off-by: Antoine Tenart <atenart@kernel.org> --- drivers/net/vrf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)