* [RFC net] Discuss seg6 potential wrong behavior
@ 2022-03-18 20:21 Justin Iurman
2022-03-23 2:10 ` David Ahern
0 siblings, 1 reply; 3+ messages in thread
From: Justin Iurman @ 2022-03-18 20:21 UTC (permalink / raw)
To: netdev; +Cc: davem, yoshfuji, dsahern, kuba, pabeni, Justin Iurman
This thread aims to discuss a potential wrong behavior regarding seg6
(as well as rpl). I'm curious to know if there is a specific reason for
discarding the packet when seg6 is not enabled on an interface and when
segments_left == 0. Indeed, reading RFC8754, I'm not sure this is the
right thing to do. I think it would be more correct to process the next
header in the packet. It does not make any sense to prevent further
processing when the SRv6 node has literally nothing to do in that
specific case. For that, we need to postpone the check of accept_seg6.
And, in order to avoid a breach, we also check for accept_seg6 before
decapsulation when segments_left == 0. Any comments on this?
Also, I'm not sure why accept_seg6 is set the current way. Are we not
suppose to prioritize devconf_all? If "all" is set to 1, then it is
enabled for all interfaces. Therefore, having an OR condition looks more
correct. Right now, we need to set both "all" and the interface to 1 so
that seg6 is enabled on this specific interface. Looks odd, thoughts?
Note: this patch could also be applied to rpl, in the same way.
Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
---
net/ipv6/exthdrs.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 77e34aec7e82..a47c05ac504e 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -374,13 +374,7 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
idev = __in6_dev_get(skb->dev);
accept_seg6 = net->ipv6.devconf_all->seg6_enabled;
- if (accept_seg6 > idev->cnf.seg6_enabled)
- accept_seg6 = idev->cnf.seg6_enabled;
-
- if (!accept_seg6) {
- kfree_skb(skb);
- return -1;
- }
+ accept_seg6 |= idev->cnf.seg6_enabled;
#ifdef CONFIG_IPV6_SEG6_HMAC
if (!seg6_hmac_validate_skb(skb)) {
@@ -392,6 +386,11 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
looped_back:
if (hdr->segments_left == 0) {
if (hdr->nexthdr == NEXTHDR_IPV6 || hdr->nexthdr == NEXTHDR_IPV4) {
+ if (!accept_seg6) {
+ kfree_skb(skb);
+ return -1;
+ }
+
int offset = (hdr->hdrlen + 1) << 3;
skb_postpull_rcsum(skb, skb_network_header(skb),
@@ -431,6 +430,11 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
return -1;
}
+ if (!accept_seg6) {
+ kfree_skb(skb);
+ return -1;
+ }
+
if (skb_cloned(skb)) {
if (pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
__IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC net] Discuss seg6 potential wrong behavior
2022-03-18 20:21 [RFC net] Discuss seg6 potential wrong behavior Justin Iurman
@ 2022-03-23 2:10 ` David Ahern
2022-03-30 9:53 ` Justin Iurman
0 siblings, 1 reply; 3+ messages in thread
From: David Ahern @ 2022-03-23 2:10 UTC (permalink / raw)
To: Justin Iurman, netdev; +Cc: davem, yoshfuji, kuba, pabeni
On 3/18/22 2:21 PM, Justin Iurman wrote:
> This thread aims to discuss a potential wrong behavior regarding seg6
> (as well as rpl). I'm curious to know if there is a specific reason for
> discarding the packet when seg6 is not enabled on an interface and when
that is standard. MPLS for example does the same.
> segments_left == 0. Indeed, reading RFC8754, I'm not sure this is the
> right thing to do. I think it would be more correct to process the next
> header in the packet. It does not make any sense to prevent further
> processing when the SRv6 node has literally nothing to do in that
> specific case. For that, we need to postpone the check of accept_seg6.
> And, in order to avoid a breach, we also check for accept_seg6 before
> decapsulation when segments_left == 0. Any comments on this?
>
> Also, I'm not sure why accept_seg6 is set the current way. Are we not
> suppose to prioritize devconf_all? If "all" is set to 1, then it is
sadly, ipv6 is all over the place with 'all' vs 'dev' settings.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC net] Discuss seg6 potential wrong behavior
2022-03-23 2:10 ` David Ahern
@ 2022-03-30 9:53 ` Justin Iurman
0 siblings, 0 replies; 3+ messages in thread
From: Justin Iurman @ 2022-03-30 9:53 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: davem, yoshfuji, kuba, pabeni
Hi David,
Thanks for your feedback and sorry for the delay in my response.
On 3/23/22 03:10, David Ahern wrote:
> On 3/18/22 2:21 PM, Justin Iurman wrote:
>> This thread aims to discuss a potential wrong behavior regarding seg6
>> (as well as rpl). I'm curious to know if there is a specific reason for
>> discarding the packet when seg6 is not enabled on an interface and when
>
> that is standard. MPLS for example does the same.
I guess you mean "standard" from a kernel implementation point of view,
not from the RFC one, right? Again, I understand *why* it would be
implemented the current way so that one doesn't bother distinguishing
between the case I mentioned and other cases (only talking about routing
headers here, did not give a look at the mpls implementation nor RFCs).
But still, I can't stop thinking it's wrong, especially when the RFC
(more than) suggests the opposite.
Indeed, having the case (!seg6_enabled && segments_left == 0) is not
harmful for a node (only if nexthdr != (IPv6||IPv4) of course, otherwise
there could probably be something suspicious). So, right now, if an
operator forgets to enable it on one of its egresses, the packet is just
silently dropped. And there goes the wasted time trying to figure out
what's wrong. Also, let's just assume that you want your packet to be
sent towards a destination Y, and going through a specific node X on the
path. As long as you don't cross a domain/AS where seg6 is enabled, your
packet goes through. Fine, but, if you don't own the destination and it
runs a Linux kernel, the packet will be discarded if seg6 is not enabled
(which is the case for each interface, by default). Which is sad,
because there is literally nothing to do and the packet should be accepted.
So, the correct way of handling this specific case mentioned previously
(which is also in the RFC) would be to continue processing next header.
It would semantically sound better. And, such a change would not
introduce any issue, whether seg6 is enabled or not in your domain/AS. I
insist on this point as it is important.
>> segments_left == 0. Indeed, reading RFC8754, I'm not sure this is the
>
>
>> right thing to do. I think it would be more correct to process the next
>> header in the packet. It does not make any sense to prevent further
>> processing when the SRv6 node has literally nothing to do in that
>> specific case. For that, we need to postpone the check of accept_seg6.
>> And, in order to avoid a breach, we also check for accept_seg6 before
>> decapsulation when segments_left == 0. Any comments on this?
>>
>> Also, I'm not sure why accept_seg6 is set the current way. Are we not
>> suppose to prioritize devconf_all? If "all" is set to 1, then it is
>
> sadly, ipv6 is all over the place with 'all' vs 'dev' settings.
I see. Indeed, changing that might bring some compatibility issues
depending on configurations. In that case, maybe should we update the
documentation for each concerned part, so that people know that "all &&
dev" is mandatory to enable the feature (at least for seg6, though).
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-30 9:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-18 20:21 [RFC net] Discuss seg6 potential wrong behavior Justin Iurman
2022-03-23 2:10 ` David Ahern
2022-03-30 9:53 ` Justin Iurman
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).