* [RFC net] Discuss seg6 potential wrong behavior
@ 2022-03-18 20:21 Justin Iurman
2022-03-18 22:01 ` kernel test robot
2022-03-23 2:10 ` David Ahern
0 siblings, 2 replies; 4+ 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] 4+ 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-18 22:01 ` kernel test robot
2022-03-23 2:10 ` David Ahern
1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2022-03-18 22:01 UTC (permalink / raw)
To: Justin Iurman; +Cc: llvm, kbuild-all
Hi Justin,
[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]
url: https://github.com/0day-ci/linux/commits/Justin-Iurman/Discuss-seg6-potential-wrong-behavior/20220319-042938
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 6bd0c76bd70447aedfeafa9e1fcc249991d6c678
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220319/202203190555.sLM83GRv-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project a6e70e4056dff962ec634c5bd4f2f4105a0bef71)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/92248dc32f87d0aae6c02a08f29376a32f57b669
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Justin-Iurman/Discuss-seg6-potential-wrong-behavior/20220319-042938
git checkout 92248dc32f87d0aae6c02a08f29376a32f57b669
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/ipv6/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> net/ipv6/exthdrs.c:394:8: warning: mixing declarations and code is a C99 extension [-Wdeclaration-after-statement]
int offset = (hdr->hdrlen + 1) << 3;
^
1 warning generated.
vim +394 net/ipv6/exthdrs.c
9baee83406d6a4b David Lebrun 2016-11-08 385
1ababeba4a21f3d David Lebrun 2016-11-08 386 looped_back:
013e8167899d389 David Lebrun 2017-02-02 387 if (hdr->segments_left == 0) {
ee90c6ba341f7f7 Julien Massonneau 2021-03-11 388 if (hdr->nexthdr == NEXTHDR_IPV6 || hdr->nexthdr == NEXTHDR_IPV4) {
92248dc32f87d0a Justin Iurman 2022-03-18 389 if (!accept_seg6) {
92248dc32f87d0a Justin Iurman 2022-03-18 390 kfree_skb(skb);
92248dc32f87d0a Justin Iurman 2022-03-18 391 return -1;
92248dc32f87d0a Justin Iurman 2022-03-18 392 }
92248dc32f87d0a Justin Iurman 2022-03-18 393
1ababeba4a21f3d David Lebrun 2016-11-08 @394 int offset = (hdr->hdrlen + 1) << 3;
1ababeba4a21f3d David Lebrun 2016-11-08 395
1ababeba4a21f3d David Lebrun 2016-11-08 396 skb_postpull_rcsum(skb, skb_network_header(skb),
1ababeba4a21f3d David Lebrun 2016-11-08 397 skb_network_header_len(skb));
1ababeba4a21f3d David Lebrun 2016-11-08 398
1ababeba4a21f3d David Lebrun 2016-11-08 399 if (!pskb_pull(skb, offset)) {
1ababeba4a21f3d David Lebrun 2016-11-08 400 kfree_skb(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 401 return -1;
1ababeba4a21f3d David Lebrun 2016-11-08 402 }
1ababeba4a21f3d David Lebrun 2016-11-08 403 skb_postpull_rcsum(skb, skb_transport_header(skb),
1ababeba4a21f3d David Lebrun 2016-11-08 404 offset);
1ababeba4a21f3d David Lebrun 2016-11-08 405
1ababeba4a21f3d David Lebrun 2016-11-08 406 skb_reset_network_header(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 407 skb_reset_transport_header(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 408 skb->encapsulation = 0;
ee90c6ba341f7f7 Julien Massonneau 2021-03-11 409 if (hdr->nexthdr == NEXTHDR_IPV4)
ee90c6ba341f7f7 Julien Massonneau 2021-03-11 410 skb->protocol = htons(ETH_P_IP);
1ababeba4a21f3d David Lebrun 2016-11-08 411 __skb_tunnel_rx(skb, skb->dev, net);
1ababeba4a21f3d David Lebrun 2016-11-08 412
1ababeba4a21f3d David Lebrun 2016-11-08 413 netif_rx(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 414 return -1;
1ababeba4a21f3d David Lebrun 2016-11-08 415 }
1ababeba4a21f3d David Lebrun 2016-11-08 416
1ababeba4a21f3d David Lebrun 2016-11-08 417 opt->srcrt = skb_network_header_len(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 418 opt->lastopt = opt->srcrt;
1ababeba4a21f3d David Lebrun 2016-11-08 419 skb->transport_header += (hdr->hdrlen + 1) << 3;
1ababeba4a21f3d David Lebrun 2016-11-08 420 opt->nhoff = (&hdr->nexthdr) - skb_network_header(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 421
1ababeba4a21f3d David Lebrun 2016-11-08 422 return 1;
1ababeba4a21f3d David Lebrun 2016-11-08 423 }
1ababeba4a21f3d David Lebrun 2016-11-08 424
1ababeba4a21f3d David Lebrun 2016-11-08 425 if (hdr->segments_left >= (hdr->hdrlen >> 1)) {
bdb7cc643fc9db8 Stephen Suryaputra 2018-04-16 426 __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
1ababeba4a21f3d David Lebrun 2016-11-08 427 icmpv6_param_prob(skb, ICMPV6_HDR_FIELD,
1ababeba4a21f3d David Lebrun 2016-11-08 428 ((&hdr->segments_left) -
1ababeba4a21f3d David Lebrun 2016-11-08 429 skb_network_header(skb)));
92248dc32f87d0a Justin Iurman 2022-03-18 430 return -1;
92248dc32f87d0a Justin Iurman 2022-03-18 431 }
92248dc32f87d0a Justin Iurman 2022-03-18 432
92248dc32f87d0a Justin Iurman 2022-03-18 433 if (!accept_seg6) {
92248dc32f87d0a Justin Iurman 2022-03-18 434 kfree_skb(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 435 return -1;
1ababeba4a21f3d David Lebrun 2016-11-08 436 }
1ababeba4a21f3d David Lebrun 2016-11-08 437
1ababeba4a21f3d David Lebrun 2016-11-08 438 if (skb_cloned(skb)) {
1ababeba4a21f3d David Lebrun 2016-11-08 439 if (pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
1ababeba4a21f3d David Lebrun 2016-11-08 440 __IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
1ababeba4a21f3d David Lebrun 2016-11-08 441 IPSTATS_MIB_OUTDISCARDS);
1ababeba4a21f3d David Lebrun 2016-11-08 442 kfree_skb(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 443 return -1;
1ababeba4a21f3d David Lebrun 2016-11-08 444 }
1ababeba4a21f3d David Lebrun 2016-11-08 445 }
1ababeba4a21f3d David Lebrun 2016-11-08 446
1ababeba4a21f3d David Lebrun 2016-11-08 447 hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 448
1ababeba4a21f3d David Lebrun 2016-11-08 449 hdr->segments_left--;
1ababeba4a21f3d David Lebrun 2016-11-08 450 addr = hdr->segments + hdr->segments_left;
1ababeba4a21f3d David Lebrun 2016-11-08 451
1ababeba4a21f3d David Lebrun 2016-11-08 452 skb_push(skb, sizeof(struct ipv6hdr));
1ababeba4a21f3d David Lebrun 2016-11-08 453
1ababeba4a21f3d David Lebrun 2016-11-08 454 if (skb->ip_summed == CHECKSUM_COMPLETE)
1ababeba4a21f3d David Lebrun 2016-11-08 455 seg6_update_csum(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 456
1ababeba4a21f3d David Lebrun 2016-11-08 457 ipv6_hdr(skb)->daddr = *addr;
1ababeba4a21f3d David Lebrun 2016-11-08 458
1ababeba4a21f3d David Lebrun 2016-11-08 459 skb_dst_drop(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 460
1ababeba4a21f3d David Lebrun 2016-11-08 461 ip6_route_input(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 462
1ababeba4a21f3d David Lebrun 2016-11-08 463 if (skb_dst(skb)->error) {
1ababeba4a21f3d David Lebrun 2016-11-08 464 dst_input(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 465 return -1;
1ababeba4a21f3d David Lebrun 2016-11-08 466 }
1ababeba4a21f3d David Lebrun 2016-11-08 467
1ababeba4a21f3d David Lebrun 2016-11-08 468 if (skb_dst(skb)->dev->flags & IFF_LOOPBACK) {
1ababeba4a21f3d David Lebrun 2016-11-08 469 if (ipv6_hdr(skb)->hop_limit <= 1) {
bdb7cc643fc9db8 Stephen Suryaputra 2018-04-16 470 __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
1ababeba4a21f3d David Lebrun 2016-11-08 471 icmpv6_send(skb, ICMPV6_TIME_EXCEED,
1ababeba4a21f3d David Lebrun 2016-11-08 472 ICMPV6_EXC_HOPLIMIT, 0);
1ababeba4a21f3d David Lebrun 2016-11-08 473 kfree_skb(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 474 return -1;
1ababeba4a21f3d David Lebrun 2016-11-08 475 }
1ababeba4a21f3d David Lebrun 2016-11-08 476 ipv6_hdr(skb)->hop_limit--;
1ababeba4a21f3d David Lebrun 2016-11-08 477
1ababeba4a21f3d David Lebrun 2016-11-08 478 skb_pull(skb, sizeof(struct ipv6hdr));
1ababeba4a21f3d David Lebrun 2016-11-08 479 goto looped_back;
1ababeba4a21f3d David Lebrun 2016-11-08 480 }
1ababeba4a21f3d David Lebrun 2016-11-08 481
1ababeba4a21f3d David Lebrun 2016-11-08 482 dst_input(skb);
1ababeba4a21f3d David Lebrun 2016-11-08 483
1ababeba4a21f3d David Lebrun 2016-11-08 484 return -1;
1ababeba4a21f3d David Lebrun 2016-11-08 485 }
1ababeba4a21f3d David Lebrun 2016-11-08 486
---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 4+ 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-18 22:01 ` kernel test robot
@ 2022-03-23 2:10 ` David Ahern
2022-03-30 9:53 ` Justin Iurman
1 sibling, 1 reply; 4+ 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] 4+ 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; 4+ 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] 4+ messages in thread
end of thread, other threads:[~2022-03-30 9:53 UTC | newest]
Thread overview: 4+ 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-18 22:01 ` kernel test robot
2022-03-23 2:10 ` David Ahern
2022-03-30 9:53 ` Justin Iurman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.