* [PATCH net] seg6: fix route leak for encap routes
@ 2025-12-08 10:24 Nicolas Dichtel
2025-12-10 10:37 ` Andrea Mayer
0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Dichtel @ 2025-12-08 10:24 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet,
David Lebrun, Andrea Mayer, Paolo Lungaroni, David Ahern
Cc: netdev, Nicolas Dichtel
The goal is to take into account the device used to set up the route.
Before this commit, it was mandatory but ignored. After encapsulation, a
second route lookup is performed using the encapsulated IPv6 address.
This route lookup is now done in the vrf where the route device is set.
The l3vpn tests show the inconsistency; they are updated to reflect the
fix. Before the commit, the route to 'fc00:21:100::6046' was put in the
vrf-100 table while the encap route was pointing to veth0, which is not
associated with a vrf.
Before:
> $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
> cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
> fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
After:
> $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
> cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
> $ ip -n rt_2-Rh5GP7 -6 r list | grep fc00:21:100::6046
> fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels")
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv6/seg6_iptunnel.c | 6 ++++++
tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh | 2 +-
tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh | 2 +-
tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh | 2 +-
4 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
index 3e1b9991131a..9535aea28357 100644
--- a/net/ipv6/seg6_iptunnel.c
+++ b/net/ipv6/seg6_iptunnel.c
@@ -484,6 +484,12 @@ static int seg6_input_core(struct net *net, struct sock *sk,
* now and use it later as a comparison.
*/
lwtst = orig_dst->lwtstate;
+ if (orig_dst->dev) {
+ rcu_read_lock();
+ skb->dev = l3mdev_master_dev_rcu(orig_dst->dev) ?:
+ dev_net(skb->dev)->loopback_dev;
+ rcu_read_unlock();
+ }
slwt = seg6_lwt_lwtunnel(lwtst);
diff --git a/tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh b/tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh
index a5e959a080bb..682fb5b4509d 100755
--- a/tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh
+++ b/tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh
@@ -333,7 +333,7 @@ setup_vpn_config()
encap seg6 mode encap segs ${vpn_sid} dev veth0
ip -netns ${rtsrc_name} -4 route add ${IPv4_HS_NETWORK}.${hsdst}/32 vrf vrf-${tid} \
encap seg6 mode encap segs ${vpn_sid} dev veth0
- ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 vrf vrf-${tid} \
+ ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 \
via fd00::${rtdst} dev veth0
# set the decap route for decapsulating packets which arrive from
diff --git a/tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh b/tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh
index a649dba3cb77..11f693c65169 100755
--- a/tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh
+++ b/tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh
@@ -287,7 +287,7 @@ setup_vpn_config()
# host hssrc and destined to the access router rtsrc.
ip -netns ${rtsrc_name} -4 route add ${IPv4_HS_NETWORK}.${hsdst}/32 vrf vrf-${tid} \
encap seg6 mode encap segs ${vpn_sid} dev veth0
- ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 vrf vrf-${tid} \
+ ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 \
via fd00::${rtdst} dev veth0
# set the decap route for decapsulating packets which arrive from
diff --git a/tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh b/tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh
index e408406d8489..7d7e056c645c 100755
--- a/tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh
+++ b/tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh
@@ -297,7 +297,7 @@ setup_vpn_config()
# host hssrc and destined to the access router rtsrc.
ip -netns ${rtsrc_name} -6 route add ${IPv6_HS_NETWORK}::${hsdst}/128 vrf vrf-${tid} \
encap seg6 mode encap segs ${vpn_sid} dev veth0
- ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 vrf vrf-${tid} \
+ ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 \
via fd00::${rtdst} dev veth0
# set the decap route for decapsulating packets which arrive from
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] seg6: fix route leak for encap routes
2025-12-08 10:24 [PATCH net] seg6: fix route leak for encap routes Nicolas Dichtel
@ 2025-12-10 10:37 ` Andrea Mayer
2025-12-10 17:00 ` Nicolas Dichtel
0 siblings, 1 reply; 7+ messages in thread
From: Andrea Mayer @ 2025-12-10 10:37 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet,
David Lebrun, Paolo Lungaroni, David Ahern, netdev,
stefano.salsano, Andrea Mayer
On Mon, 8 Dec 2025 11:24:34 +0100
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> The goal is to take into account the device used to set up the route.
> Before this commit, it was mandatory but ignored. After encapsulation, a
> second route lookup is performed using the encapsulated IPv6 address.
> This route lookup is now done in the vrf where the route device is set.
>
Hi Nicolas,
I've got your point. However, I'm still concerned about the implications of
using the *dev* field in the root lookup. This field has been ignored for this
purpose so far, so some existing configurations/scripts may need to be adapted
to work again. The adjustments made to the self-tests below show what might
happen.
> The l3vpn tests show the inconsistency; they are updated to reflect the
> fix. Before the commit, the route to 'fc00:21:100::6046' was put in the
> vrf-100 table while the encap route was pointing to veth0, which is not
> associated with a vrf.
>
> Before:
> > $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
> > cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
> > fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
>
> After:
> > $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
> > cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
> > $ ip -n rt_2-Rh5GP7 -6 r list | grep fc00:21:100::6046
> > fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
>
> Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> net/ipv6/seg6_iptunnel.c | 6 ++++++
> tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh | 2 +-
> tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh | 2 +-
> tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh | 2 +-
> 4 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
> index 3e1b9991131a..9535aea28357 100644
> --- a/net/ipv6/seg6_iptunnel.c
> +++ b/net/ipv6/seg6_iptunnel.c
> @@ -484,6 +484,12 @@ static int seg6_input_core(struct net *net, struct sock *sk,
> * now and use it later as a comparison.
> */
> lwtst = orig_dst->lwtstate;
> + if (orig_dst->dev) {
When can 'orig_dst->dev' be NULL in this context?
> + rcu_read_lock();
> + skb->dev = l3mdev_master_dev_rcu(orig_dst->dev) ?:
> + dev_net(skb->dev)->loopback_dev;
> + rcu_read_unlock();
> + }
>
> slwt = seg6_lwt_lwtunnel(lwtst);
>
> diff --git a/tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh b/tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh
> index a5e959a080bb..682fb5b4509d 100755
> --- a/tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh
> +++ b/tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh
> @@ -333,7 +333,7 @@ setup_vpn_config()
> encap seg6 mode encap segs ${vpn_sid} dev veth0
> ip -netns ${rtsrc_name} -4 route add ${IPv4_HS_NETWORK}.${hsdst}/32 vrf vrf-${tid} \
> encap seg6 mode encap segs ${vpn_sid} dev veth0
> - ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 vrf vrf-${tid} \
> + ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 \
> via fd00::${rtdst} dev veth0
>
> # set the decap route for decapsulating packets which arrive from
> diff --git a/tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh b/tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh
> index a649dba3cb77..11f693c65169 100755
> --- a/tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh
> +++ b/tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh
> @@ -287,7 +287,7 @@ setup_vpn_config()
> # host hssrc and destined to the access router rtsrc.
> ip -netns ${rtsrc_name} -4 route add ${IPv4_HS_NETWORK}.${hsdst}/32 vrf vrf-${tid} \
> encap seg6 mode encap segs ${vpn_sid} dev veth0
> - ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 vrf vrf-${tid} \
> + ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 \
> via fd00::${rtdst} dev veth0
>
> # set the decap route for decapsulating packets which arrive from
> diff --git a/tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh b/tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh
> index e408406d8489..7d7e056c645c 100755
> --- a/tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh
> +++ b/tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh
> @@ -297,7 +297,7 @@ setup_vpn_config()
> # host hssrc and destined to the access router rtsrc.
> ip -netns ${rtsrc_name} -6 route add ${IPv6_HS_NETWORK}::${hsdst}/128 vrf vrf-${tid} \
> encap seg6 mode encap segs ${vpn_sid} dev veth0
> - ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 vrf vrf-${tid} \
> + ip -netns ${rtsrc_name} -6 route add ${vpn_sid}/128 \
> via fd00::${rtdst} dev veth0
>
> # set the decap route for decapsulating packets which arrive from
> --
> 2.47.1
>
Thanks,
Andrea
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] seg6: fix route leak for encap routes
2025-12-10 10:37 ` Andrea Mayer
@ 2025-12-10 17:00 ` Nicolas Dichtel
2025-12-14 13:39 ` Andrea Mayer
0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Dichtel @ 2025-12-10 17:00 UTC (permalink / raw)
To: Andrea Mayer
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet,
David Lebrun, Paolo Lungaroni, David Ahern, netdev,
stefano.salsano
Le 10/12/2025 à 11:37, Andrea Mayer a écrit :
> On Mon, 8 Dec 2025 11:24:34 +0100
> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>
>> The goal is to take into account the device used to set up the route.
>> Before this commit, it was mandatory but ignored. After encapsulation, a
>> second route lookup is performed using the encapsulated IPv6 address.
>> This route lookup is now done in the vrf where the route device is set.
>>
>
> Hi Nicolas,
Hi Andrea,
>
> I've got your point. However, I'm still concerned about the implications of
> using the *dev* field in the root lookup. This field has been ignored for this
> purpose so far, so some existing configurations/scripts may need to be adapted
> to work again. The adjustments made to the self-tests below show what might
> happen.
Yes, I was wondering how users use this *dev* arg. Maybe adding a new attribute,
something like SEG6_IPTUNNEL_USE_NH_DEV will avoid any regressions.
>
>
>> The l3vpn tests show the inconsistency; they are updated to reflect the
>> fix. Before the commit, the route to 'fc00:21:100::6046' was put in the
>> vrf-100 table while the encap route was pointing to veth0, which is not
>> associated with a vrf.
>>
>> Before:
>>> $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
>>> cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
>>> fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
>>
>> After:
>>> $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
>>> cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
>>> $ ip -n rt_2-Rh5GP7 -6 r list | grep fc00:21:100::6046
>>> fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
>>
>> Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels")
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>> net/ipv6/seg6_iptunnel.c | 6 ++++++
>> tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh | 2 +-
>> tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh | 2 +-
>> tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh | 2 +-
>> 4 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
>> index 3e1b9991131a..9535aea28357 100644
>> --- a/net/ipv6/seg6_iptunnel.c
>> +++ b/net/ipv6/seg6_iptunnel.c
>> @@ -484,6 +484,12 @@ static int seg6_input_core(struct net *net, struct sock *sk,
>> * now and use it later as a comparison.
>> */
>> lwtst = orig_dst->lwtstate;
>> + if (orig_dst->dev) {
>
> When can 'orig_dst->dev' be NULL in this context?
I was cautious to avoid any unpleasant surprises. A dst can have dst->dev set to
NULL.
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] seg6: fix route leak for encap routes
2025-12-10 17:00 ` Nicolas Dichtel
@ 2025-12-14 13:39 ` Andrea Mayer
2025-12-18 10:35 ` Paolo Abeni
2025-12-18 14:24 ` Nicolas Dichtel
0 siblings, 2 replies; 7+ messages in thread
From: Andrea Mayer @ 2025-12-14 13:39 UTC (permalink / raw)
To: nicolas.dichtel
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet,
David Lebrun, Paolo Lungaroni, David Ahern, netdev,
stefano.salsano, Andrea Mayer
On Wed, 10 Dec 2025 18:00:39 +0100
Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> Le 10/12/2025 à 11:37, Andrea Mayer a écrit :
> > On Mon, 8 Dec 2025 11:24:34 +0100
> > Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
> >
> >> The goal is to take into account the device used to set up the route.
> >> Before this commit, it was mandatory but ignored. After encapsulation, a
> >> second route lookup is performed using the encapsulated IPv6 address.
> >> This route lookup is now done in the vrf where the route device is set.
> >>
> >
> > Hi Nicolas,
> Hi Andrea,
>
Hi Nicolas,
> >
> > I've got your point. However, I'm still concerned about the implications of
> > using the *dev* field in the root lookup. This field has been ignored for this
> > purpose so far, so some existing configurations/scripts may need to be adapted
> > to work again. The adjustments made to the self-tests below show what might
> > happen.
> Yes, I was wondering how users use this *dev* arg. Maybe adding a new attribute,
> something like SEG6_IPTUNNEL_USE_NH_DEV will avoid any regressions.
>
IMHO using a new attribute seems to be a safer approach.
Is this new attribute intended to be used (a) to enable/disable the use of *dev*
during the route lookup, or (b) to carry the interface identifier (oif)
explicitly for use in the lookup?
In the latter case (b), the route *dev* would no longer be consulted at all for
this purpose.
> >
> >
> >> The l3vpn tests show the inconsistency; they are updated to reflect the
> >> fix. Before the commit, the route to 'fc00:21:100::6046' was put in the
> >> vrf-100 table while the encap route was pointing to veth0, which is not
> >> associated with a vrf.
> >>
> >> Before:
> >>> $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
> >>> cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
> >>> fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
> >>
> >> After:
> >>> $ ip -n rt_2-Rh5GP7 -6 r list vrf vrf-100 | grep fc00:21:100::6046
> >>> cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0 metric 1024 pref medium
> >>> $ ip -n rt_2-Rh5GP7 -6 r list | grep fc00:21:100::6046
> >>> fc00:21:100::6046 via fd00::1 dev veth0 metric 1024 pref medium
> >>
> >> Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels")
> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> >> ---
> >> net/ipv6/seg6_iptunnel.c | 6 ++++++
> >> tools/testing/selftests/net/srv6_end_dt46_l3vpn_test.sh | 2 +-
> >> tools/testing/selftests/net/srv6_end_dt4_l3vpn_test.sh | 2 +-
> >> tools/testing/selftests/net/srv6_end_dt6_l3vpn_test.sh | 2 +-
> >> 4 files changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
> >> index 3e1b9991131a..9535aea28357 100644
> >> --- a/net/ipv6/seg6_iptunnel.c
> >> +++ b/net/ipv6/seg6_iptunnel.c
> >> @@ -484,6 +484,12 @@ static int seg6_input_core(struct net *net, struct sock *sk,
> >> * now and use it later as a comparison.
> >> */
> >> lwtst = orig_dst->lwtstate;
> >> + if (orig_dst->dev) {
> >
> > When can 'orig_dst->dev' be NULL in this context?
> I was cautious to avoid any unpleasant surprises. A dst can have dst->dev set to
> NULL.
>
I see your point regarding caution.
However, if 'orig_dst->dev' were NULL at this point, the kernel would crash
anyway because subsequent functions (e.g., __seg6_do_srh_encap()) rely on
'orig_dst->dev' (not NULL) to retrieve the net.
> >> + rcu_read_lock();
> >> + skb->dev = l3mdev_master_dev_rcu(orig_dst->dev) ?:
> >> + dev_net(skb->dev)->loopback_dev;
One issue here is that the outgoing device (*dev*) is being treated as the
packet's *incoming* interface.
ip6_route_input() uses 'skb->dev->ifindex' to populate 'flowi6_iif'.
Consequently, if there is an 'ip rule' matching on 'iif' (ingress interface),
it will evaluate against the *dev* (the VRF or the loopback) instead of the
actual interface the packet was received on.
This can lead to incorrect policy routing lookups.
> >> + rcu_read_unlock();
> >> + }
> Thanks,
> Nicolas
Thanks,
Ciao,
Andrea
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] seg6: fix route leak for encap routes
2025-12-14 13:39 ` Andrea Mayer
@ 2025-12-18 10:35 ` Paolo Abeni
2025-12-18 14:28 ` Nicolas Dichtel
2025-12-18 14:24 ` Nicolas Dichtel
1 sibling, 1 reply; 7+ messages in thread
From: Paolo Abeni @ 2025-12-18 10:35 UTC (permalink / raw)
To: Andrea Mayer, nicolas.dichtel
Cc: David S . Miller, Jakub Kicinski, Eric Dumazet, David Lebrun,
Paolo Lungaroni, David Ahern, netdev, stefano.salsano
On 12/14/25 2:39 PM, Andrea Mayer wrote:
> On Wed, 10 Dec 2025 18:00:39 +0100
> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>> Le 10/12/2025 à 11:37, Andrea Mayer a écrit :
>>> I've got your point. However, I'm still concerned about the implications of
>>> using the *dev* field in the root lookup. This field has been ignored for this
>>> purpose so far, so some existing configurations/scripts may need to be adapted
>>> to work again. The adjustments made to the self-tests below show what might
>>> happen.
>> Yes, I was wondering how users use this *dev* arg. Maybe adding a new attribute,
>> something like SEG6_IPTUNNEL_USE_NH_DEV will avoid any regressions.
>>
>
> IMHO using a new attribute seems to be a safer approach.
Given the functional implication I suggest using a new attribute. Given
that I also suggest targeting net-next for the next revision of this patch.
>>>> diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
>>>> index 3e1b9991131a..9535aea28357 100644
>>>> --- a/net/ipv6/seg6_iptunnel.c
>>>> +++ b/net/ipv6/seg6_iptunnel.c
>>>> @@ -484,6 +484,12 @@ static int seg6_input_core(struct net *net, struct sock *sk,
>>>> * now and use it later as a comparison.
>>>> */
>>>> lwtst = orig_dst->lwtstate;
>>>> + if (orig_dst->dev) {
Here you should probably use dst_dev_rcu(), under the rcu lock, avoiding
touching dev twice.
/P
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] seg6: fix route leak for encap routes
2025-12-14 13:39 ` Andrea Mayer
2025-12-18 10:35 ` Paolo Abeni
@ 2025-12-18 14:24 ` Nicolas Dichtel
1 sibling, 0 replies; 7+ messages in thread
From: Nicolas Dichtel @ 2025-12-18 14:24 UTC (permalink / raw)
To: Andrea Mayer
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Eric Dumazet,
David Lebrun, Paolo Lungaroni, David Ahern, netdev,
stefano.salsano
Le 14/12/2025 à 14:39, Andrea Mayer a écrit :
[snip]
>>> I've got your point. However, I'm still concerned about the implications of
>>> using the *dev* field in the root lookup. This field has been ignored for this
>>> purpose so far, so some existing configurations/scripts may need to be adapted
>>> to work again. The adjustments made to the self-tests below show what might
>>> happen.
>> Yes, I was wondering how users use this *dev* arg. Maybe adding a new attribute,
>> something like SEG6_IPTUNNEL_USE_NH_DEV will avoid any regressions.
>>
>
> IMHO using a new attribute seems to be a safer approach.
>
> Is this new attribute intended to be used (a) to enable/disable the use of *dev*
> during the route lookup, or (b) to carry the interface identifier (oif)
> explicitly for use in the lookup?
> In the latter case (b), the route *dev* would no longer be consulted at all for
> this purpose.
I would tend to prefer option (a), something like:
> cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] use_dev dev eth0
With option (b), we could end up with this kind of route:
> cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] vrf blue dev eth0
where eth0 is not in vrf blue. This is confusing.
[snip]
>>>> --- a/net/ipv6/seg6_iptunnel.c
>>>> +++ b/net/ipv6/seg6_iptunnel.c
>>>> @@ -484,6 +484,12 @@ static int seg6_input_core(struct net *net, struct sock *sk,
>>>> * now and use it later as a comparison.
>>>> */
>>>> lwtst = orig_dst->lwtstate;
>>>> + if (orig_dst->dev) {
>>>
>>> When can 'orig_dst->dev' be NULL in this context?
>> I was cautious to avoid any unpleasant surprises. A dst can have dst->dev set to
>> NULL.
>>
>
> I see your point regarding caution.
>
> However, if 'orig_dst->dev' were NULL at this point, the kernel would crash
> anyway because subsequent functions (e.g., __seg6_do_srh_encap()) rely on
> 'orig_dst->dev' (not NULL) to retrieve the net.
Right, I will remove the test.
>
>
>>>> + rcu_read_lock();
>>>> + skb->dev = l3mdev_master_dev_rcu(orig_dst->dev) ?:
>>>> + dev_net(skb->dev)->loopback_dev;
>
> One issue here is that the outgoing device (*dev*) is being treated as the
> packet's *incoming* interface.
>
> ip6_route_input() uses 'skb->dev->ifindex' to populate 'flowi6_iif'.
> Consequently, if there is an 'ip rule' matching on 'iif' (ingress interface),
> it will evaluate against the *dev* (the VRF or the loopback) instead of the
> actual interface the packet was received on.
> This can lead to incorrect policy routing lookups.
Hmm, right, it should be changed only in case of x-vrf.
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] seg6: fix route leak for encap routes
2025-12-18 10:35 ` Paolo Abeni
@ 2025-12-18 14:28 ` Nicolas Dichtel
0 siblings, 0 replies; 7+ messages in thread
From: Nicolas Dichtel @ 2025-12-18 14:28 UTC (permalink / raw)
To: Paolo Abeni, Andrea Mayer
Cc: David S . Miller, Jakub Kicinski, Eric Dumazet, David Lebrun,
Paolo Lungaroni, David Ahern, netdev, stefano.salsano
Le 18/12/2025 à 11:35, Paolo Abeni a écrit :
> On 12/14/25 2:39 PM, Andrea Mayer wrote:
>> On Wed, 10 Dec 2025 18:00:39 +0100
>> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>>> Le 10/12/2025 à 11:37, Andrea Mayer a écrit :
>>>> I've got your point. However, I'm still concerned about the implications of
>>>> using the *dev* field in the root lookup. This field has been ignored for this
>>>> purpose so far, so some existing configurations/scripts may need to be adapted
>>>> to work again. The adjustments made to the self-tests below show what might
>>>> happen.
>>> Yes, I was wondering how users use this *dev* arg. Maybe adding a new attribute,
>>> something like SEG6_IPTUNNEL_USE_NH_DEV will avoid any regressions.
>>>
>>
>> IMHO using a new attribute seems to be a safer approach.
I agree.
>
> Given the functional implication I suggest using a new attribute. Given
> that I also suggest targeting net-next for the next revision of this patch.
I see this as a bug. A route such as
cafe::1 encap seg6 mode encap segs 1 [ fc00:21:100::6046 ] dev veth0
with veth0 completely ignored but mandatory is buggy.
>
>>>>> diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
>>>>> index 3e1b9991131a..9535aea28357 100644
>>>>> --- a/net/ipv6/seg6_iptunnel.c
>>>>> +++ b/net/ipv6/seg6_iptunnel.c
>>>>> @@ -484,6 +484,12 @@ static int seg6_input_core(struct net *net, struct sock *sk,
>>>>> * now and use it later as a comparison.
>>>>> */
>>>>> lwtst = orig_dst->lwtstate;
>>>>> + if (orig_dst->dev) {
>
> Here you should probably use dst_dev_rcu(), under the rcu lock, avoiding
> touching dev twice.
Ok.
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-12-18 14:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-08 10:24 [PATCH net] seg6: fix route leak for encap routes Nicolas Dichtel
2025-12-10 10:37 ` Andrea Mayer
2025-12-10 17:00 ` Nicolas Dichtel
2025-12-14 13:39 ` Andrea Mayer
2025-12-18 10:35 ` Paolo Abeni
2025-12-18 14:28 ` Nicolas Dichtel
2025-12-18 14:24 ` Nicolas Dichtel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).