* [bug report] tcp: accecn: AccECN option
@ 2025-09-23 11:27 Dan Carpenter
2025-09-23 18:23 ` Ilpo Järvinen
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-09-23 11:27 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: netdev
Hello Ilpo Järvinen,
Commit b5e74132dfbe ("tcp: accecn: AccECN option") from Sep 16, 2025
(linux-next), leads to the following Smatch static checker warning:
net/ipv4/tcp_output.c:747 tcp_options_write()
error: we previously assumed 'tp' could be null (see line 711)
net/ipv4/tcp_output.c
630 static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
631 const struct tcp_request_sock *tcprsk,
632 struct tcp_out_options *opts,
633 struct tcp_key *key)
634 {
635 u8 leftover_highbyte = TCPOPT_NOP; /* replace 1st NOP if avail */
636 u8 leftover_lowbyte = TCPOPT_NOP; /* replace 2nd NOP in succession */
637 __be32 *ptr = (__be32 *)(th + 1);
638 u16 options = opts->options; /* mungable copy */
639
640 if (tcp_key_is_md5(key)) {
641 *ptr++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) |
642 (TCPOPT_MD5SIG << 8) | TCPOLEN_MD5SIG);
643 /* overload cookie hash location */
644 opts->hash_location = (__u8 *)ptr;
645 ptr += 4;
646 } else if (tcp_key_is_ao(key)) {
647 ptr = process_tcp_ao_options(tp, tcprsk, opts, key, ptr);
^^
Sometimes dereferenced here.
648 }
649 if (unlikely(opts->mss)) {
650 *ptr++ = htonl((TCPOPT_MSS << 24) |
651 (TCPOLEN_MSS << 16) |
652 opts->mss);
653 }
654
655 if (likely(OPTION_TS & options)) {
656 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
657 *ptr++ = htonl((TCPOPT_SACK_PERM << 24) |
658 (TCPOLEN_SACK_PERM << 16) |
659 (TCPOPT_TIMESTAMP << 8) |
660 TCPOLEN_TIMESTAMP);
661 options &= ~OPTION_SACK_ADVERTISE;
662 } else {
663 *ptr++ = htonl((TCPOPT_NOP << 24) |
664 (TCPOPT_NOP << 16) |
665 (TCPOPT_TIMESTAMP << 8) |
666 TCPOLEN_TIMESTAMP);
667 }
668 *ptr++ = htonl(opts->tsval);
669 *ptr++ = htonl(opts->tsecr);
670 }
671
672 if (OPTION_ACCECN & options) {
673 const u32 *ecn_bytes = opts->use_synack_ecn_bytes ?
674 synack_ecn_bytes :
675 tp->received_ecn_bytes;
^^^^
Dereference
676 const u8 ect0_idx = INET_ECN_ECT_0 - 1;
677 const u8 ect1_idx = INET_ECN_ECT_1 - 1;
678 const u8 ce_idx = INET_ECN_CE - 1;
679 u32 e0b;
680 u32 e1b;
681 u32 ceb;
682 u8 len;
683
684 e0b = ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
685 e1b = ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
686 ceb = ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;
687 len = TCPOLEN_ACCECN_BASE +
688 opts->num_accecn_fields * TCPOLEN_ACCECN_PERFIELD;
689
690 if (opts->num_accecn_fields == 2) {
691 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
692 ((e1b >> 8) & 0xffff));
693 *ptr++ = htonl(((e1b & 0xff) << 24) |
694 (ceb & 0xffffff));
695 } else if (opts->num_accecn_fields == 1) {
696 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
697 ((e1b >> 8) & 0xffff));
698 leftover_highbyte = e1b & 0xff;
699 leftover_lowbyte = TCPOPT_NOP;
700 } else if (opts->num_accecn_fields == 0) {
701 leftover_highbyte = TCPOPT_ACCECN1;
702 leftover_lowbyte = len;
703 } else if (opts->num_accecn_fields == 3) {
704 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
705 ((e1b >> 8) & 0xffff));
706 *ptr++ = htonl(((e1b & 0xff) << 24) |
707 (ceb & 0xffffff));
708 *ptr++ = htonl(((e0b & 0xffffff) << 8) |
709 TCPOPT_NOP);
710 }
711 if (tp) {
^^
Here we assume tp can be NULL
712 tp->accecn_minlen = 0;
713 tp->accecn_opt_tstamp = tp->tcp_mstamp;
714 if (tp->accecn_opt_demand)
715 tp->accecn_opt_demand--;
716 }
717 }
718
719 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
720 *ptr++ = htonl((leftover_highbyte << 24) |
721 (leftover_lowbyte << 16) |
722 (TCPOPT_SACK_PERM << 8) |
723 TCPOLEN_SACK_PERM);
724 leftover_highbyte = TCPOPT_NOP;
725 leftover_lowbyte = TCPOPT_NOP;
726 }
727
728 if (unlikely(OPTION_WSCALE & options)) {
729 u8 highbyte = TCPOPT_NOP;
730
731 /* Do not split the leftover 2-byte to fit into a single
732 * NOP, i.e., replace this NOP only when 1 byte is leftover
733 * within leftover_highbyte.
734 */
735 if (unlikely(leftover_highbyte != TCPOPT_NOP &&
736 leftover_lowbyte == TCPOPT_NOP)) {
737 highbyte = leftover_highbyte;
738 leftover_highbyte = TCPOPT_NOP;
739 }
740 *ptr++ = htonl((highbyte << 24) |
741 (TCPOPT_WINDOW << 16) |
742 (TCPOLEN_WINDOW << 8) |
743 opts->ws);
744 }
745
746 if (unlikely(opts->num_sack_blocks)) {
--> 747 struct tcp_sack_block *sp = tp->rx_opt.dsack ?
^^^^^^^^^^^^^^^^
Unchecked dereference here.
748 tp->duplicate_sack : tp->selective_acks;
749 int this_sack;
750
751 *ptr++ = htonl((leftover_highbyte << 24) |
752 (leftover_lowbyte << 16) |
753 (TCPOPT_SACK << 8) |
754 (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
755 TCPOLEN_SACK_PERBLOCK)));
756 leftover_highbyte = TCPOPT_NOP;
757 leftover_lowbyte = TCPOPT_NOP;
758
759 for (this_sack = 0; this_sack < opts->num_sack_blocks;
760 ++this_sack) {
761 *ptr++ = htonl(sp[this_sack].start_seq);
762 *ptr++ = htonl(sp[this_sack].end_seq);
763 }
764
765 tp->rx_opt.dsack = 0;
766 } else if (unlikely(leftover_highbyte != TCPOPT_NOP ||
767 leftover_lowbyte != TCPOPT_NOP)) {
768 *ptr++ = htonl((leftover_highbyte << 24) |
769 (leftover_lowbyte << 16) |
770 (TCPOPT_NOP << 8) |
771 TCPOPT_NOP);
772 leftover_highbyte = TCPOPT_NOP;
773 leftover_lowbyte = TCPOPT_NOP;
774 }
775
776 if (unlikely(OPTION_FAST_OPEN_COOKIE & options)) {
777 struct tcp_fastopen_cookie *foc = opts->fastopen_cookie;
778 u8 *p = (u8 *)ptr;
779 u32 len; /* Fast Open option length */
780
781 if (foc->exp) {
782 len = TCPOLEN_EXP_FASTOPEN_BASE + foc->len;
783 *ptr = htonl((TCPOPT_EXP << 24) | (len << 16) |
784 TCPOPT_FASTOPEN_MAGIC);
785 p += TCPOLEN_EXP_FASTOPEN_BASE;
786 } else {
787 len = TCPOLEN_FASTOPEN_BASE + foc->len;
788 *p++ = TCPOPT_FASTOPEN;
789 *p++ = len;
790 }
791
792 memcpy(p, foc->val, foc->len);
793 if ((len & 3) == 2) {
794 p[foc->len] = TCPOPT_NOP;
795 p[foc->len + 1] = TCPOPT_NOP;
796 }
797 ptr += (len + 3) >> 2;
798 }
799
800 smc_options_write(ptr, &options);
801
802 mptcp_options_write(th, ptr, tp, opts);
^^
The last dereference is checked for NULL but the others aren't.
803 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] tcp: accecn: AccECN option
2025-09-23 11:27 [bug report] tcp: accecn: AccECN option Dan Carpenter
@ 2025-09-23 18:23 ` Ilpo Järvinen
2025-09-24 12:25 ` Chia-Yu Chang (Nokia)
0 siblings, 1 reply; 4+ messages in thread
From: Ilpo Järvinen @ 2025-09-23 18:23 UTC (permalink / raw)
To: Dan Carpenter, chia-yu.chang; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 11615 bytes --]
On Tue, 23 Sep 2025, Dan Carpenter wrote:
> Hello Ilpo Järvinen,
>
> Commit b5e74132dfbe ("tcp: accecn: AccECN option") from Sep 16, 2025
> (linux-next), leads to the following Smatch static checker warning:
>
> net/ipv4/tcp_output.c:747 tcp_options_write()
> error: we previously assumed 'tp' could be null (see line 711)
>
> net/ipv4/tcp_output.c
> 630 static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> 631 const struct tcp_request_sock *tcprsk,
> 632 struct tcp_out_options *opts,
> 633 struct tcp_key *key)
> 634 {
> 635 u8 leftover_highbyte = TCPOPT_NOP; /* replace 1st NOP if avail */
> 636 u8 leftover_lowbyte = TCPOPT_NOP; /* replace 2nd NOP in succession */
> 637 __be32 *ptr = (__be32 *)(th + 1);
> 638 u16 options = opts->options; /* mungable copy */
> 639
> 640 if (tcp_key_is_md5(key)) {
> 641 *ptr++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) |
> 642 (TCPOPT_MD5SIG << 8) | TCPOLEN_MD5SIG);
> 643 /* overload cookie hash location */
> 644 opts->hash_location = (__u8 *)ptr;
> 645 ptr += 4;
> 646 } else if (tcp_key_is_ao(key)) {
> 647 ptr = process_tcp_ao_options(tp, tcprsk, opts, key, ptr);
> ^^
> Sometimes dereferenced here.
>
> 648 }
> 649 if (unlikely(opts->mss)) {
> 650 *ptr++ = htonl((TCPOPT_MSS << 24) |
> 651 (TCPOLEN_MSS << 16) |
> 652 opts->mss);
> 653 }
> 654
> 655 if (likely(OPTION_TS & options)) {
> 656 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> 657 *ptr++ = htonl((TCPOPT_SACK_PERM << 24) |
> 658 (TCPOLEN_SACK_PERM << 16) |
> 659 (TCPOPT_TIMESTAMP << 8) |
> 660 TCPOLEN_TIMESTAMP);
> 661 options &= ~OPTION_SACK_ADVERTISE;
> 662 } else {
> 663 *ptr++ = htonl((TCPOPT_NOP << 24) |
> 664 (TCPOPT_NOP << 16) |
> 665 (TCPOPT_TIMESTAMP << 8) |
> 666 TCPOLEN_TIMESTAMP);
> 667 }
> 668 *ptr++ = htonl(opts->tsval);
> 669 *ptr++ = htonl(opts->tsecr);
> 670 }
> 671
> 672 if (OPTION_ACCECN & options) {
> 673 const u32 *ecn_bytes = opts->use_synack_ecn_bytes ?
> 674 synack_ecn_bytes :
> 675 tp->received_ecn_bytes;
> ^^^^
> Dereference
Hi Dan,
While it is long ago I made these changes (they might have changed a
little from that), I can say this part is going to be extremely tricky
for static checkers because TCP state machine(s) are quite complex.
TCP options can be written to a packet when tp has not yet been created
(during handshake) as well as after creation of tp using this same
function. Not all combinations are possible because handshake has to
complete before some things are enabled.
Without checking this myself, my assumption is that ->use_synack_ecn_bytes
is set when we don't have tp available yet as SYNACKs relate to handshake.
So the tp check is likely there even if not literally written.
Chia-Yu, could you please check these cases for the parts that new code
was introduced whether tp can be NULL? I think this particular line is the
most likely one to be wrong if something is, that is, can OPTION_ACCECN
be set while use_synack_ecn_bytes is not when tp is not yet there.
> 676 const u8 ect0_idx = INET_ECN_ECT_0 - 1;
> 677 const u8 ect1_idx = INET_ECN_ECT_1 - 1;
> 678 const u8 ce_idx = INET_ECN_CE - 1;
> 679 u32 e0b;
> 680 u32 e1b;
> 681 u32 ceb;
> 682 u8 len;
> 683
> 684 e0b = ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
> 685 e1b = ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
> 686 ceb = ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;
> 687 len = TCPOLEN_ACCECN_BASE +
> 688 opts->num_accecn_fields * TCPOLEN_ACCECN_PERFIELD;
> 689
> 690 if (opts->num_accecn_fields == 2) {
> 691 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> 692 ((e1b >> 8) & 0xffff));
> 693 *ptr++ = htonl(((e1b & 0xff) << 24) |
> 694 (ceb & 0xffffff));
> 695 } else if (opts->num_accecn_fields == 1) {
> 696 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> 697 ((e1b >> 8) & 0xffff));
> 698 leftover_highbyte = e1b & 0xff;
> 699 leftover_lowbyte = TCPOPT_NOP;
> 700 } else if (opts->num_accecn_fields == 0) {
> 701 leftover_highbyte = TCPOPT_ACCECN1;
> 702 leftover_lowbyte = len;
> 703 } else if (opts->num_accecn_fields == 3) {
> 704 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> 705 ((e1b >> 8) & 0xffff));
> 706 *ptr++ = htonl(((e1b & 0xff) << 24) |
> 707 (ceb & 0xffffff));
> 708 *ptr++ = htonl(((e0b & 0xffffff) << 8) |
> 709 TCPOPT_NOP);
> 710 }
> 711 if (tp) {
> ^^
> Here we assume tp can be NULL
>
> 712 tp->accecn_minlen = 0;
> 713 tp->accecn_opt_tstamp = tp->tcp_mstamp;
> 714 if (tp->accecn_opt_demand)
> 715 tp->accecn_opt_demand--;
> 716 }
> 717 }
> 718
> 719 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> 720 *ptr++ = htonl((leftover_highbyte << 24) |
> 721 (leftover_lowbyte << 16) |
> 722 (TCPOPT_SACK_PERM << 8) |
> 723 TCPOLEN_SACK_PERM);
> 724 leftover_highbyte = TCPOPT_NOP;
> 725 leftover_lowbyte = TCPOPT_NOP;
> 726 }
> 727
> 728 if (unlikely(OPTION_WSCALE & options)) {
> 729 u8 highbyte = TCPOPT_NOP;
> 730
> 731 /* Do not split the leftover 2-byte to fit into a single
> 732 * NOP, i.e., replace this NOP only when 1 byte is leftover
> 733 * within leftover_highbyte.
> 734 */
> 735 if (unlikely(leftover_highbyte != TCPOPT_NOP &&
> 736 leftover_lowbyte == TCPOPT_NOP)) {
> 737 highbyte = leftover_highbyte;
> 738 leftover_highbyte = TCPOPT_NOP;
> 739 }
> 740 *ptr++ = htonl((highbyte << 24) |
> 741 (TCPOPT_WINDOW << 16) |
> 742 (TCPOLEN_WINDOW << 8) |
> 743 opts->ws);
> 744 }
> 745
> 746 if (unlikely(opts->num_sack_blocks)) {
> --> 747 struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> ^^^^^^^^^^^^^^^^
> Unchecked dereference here.
>
> 748 tp->duplicate_sack : tp->selective_acks;
> 749 int this_sack;
> 750
> 751 *ptr++ = htonl((leftover_highbyte << 24) |
> 752 (leftover_lowbyte << 16) |
> 753 (TCPOPT_SACK << 8) |
> 754 (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
> 755 TCPOLEN_SACK_PERBLOCK)));
> 756 leftover_highbyte = TCPOPT_NOP;
> 757 leftover_lowbyte = TCPOPT_NOP;
> 758
> 759 for (this_sack = 0; this_sack < opts->num_sack_blocks;
> 760 ++this_sack) {
> 761 *ptr++ = htonl(sp[this_sack].start_seq);
> 762 *ptr++ = htonl(sp[this_sack].end_seq);
> 763 }
> 764
> 765 tp->rx_opt.dsack = 0;
> 766 } else if (unlikely(leftover_highbyte != TCPOPT_NOP ||
> 767 leftover_lowbyte != TCPOPT_NOP)) {
> 768 *ptr++ = htonl((leftover_highbyte << 24) |
> 769 (leftover_lowbyte << 16) |
> 770 (TCPOPT_NOP << 8) |
> 771 TCPOPT_NOP);
> 772 leftover_highbyte = TCPOPT_NOP;
> 773 leftover_lowbyte = TCPOPT_NOP;
> 774 }
> 775
> 776 if (unlikely(OPTION_FAST_OPEN_COOKIE & options)) {
> 777 struct tcp_fastopen_cookie *foc = opts->fastopen_cookie;
> 778 u8 *p = (u8 *)ptr;
> 779 u32 len; /* Fast Open option length */
> 780
> 781 if (foc->exp) {
> 782 len = TCPOLEN_EXP_FASTOPEN_BASE + foc->len;
> 783 *ptr = htonl((TCPOPT_EXP << 24) | (len << 16) |
> 784 TCPOPT_FASTOPEN_MAGIC);
> 785 p += TCPOLEN_EXP_FASTOPEN_BASE;
> 786 } else {
> 787 len = TCPOLEN_FASTOPEN_BASE + foc->len;
> 788 *p++ = TCPOPT_FASTOPEN;
> 789 *p++ = len;
> 790 }
> 791
> 792 memcpy(p, foc->val, foc->len);
> 793 if ((len & 3) == 2) {
> 794 p[foc->len] = TCPOPT_NOP;
> 795 p[foc->len + 1] = TCPOPT_NOP;
> 796 }
> 797 ptr += (len + 3) >> 2;
> 798 }
> 799
> 800 smc_options_write(ptr, &options);
> 801
> 802 mptcp_options_write(th, ptr, tp, opts);
> ^^
> The last dereference is checked for NULL but the others aren't.
--
i.
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [bug report] tcp: accecn: AccECN option
2025-09-23 18:23 ` Ilpo Järvinen
@ 2025-09-24 12:25 ` Chia-Yu Chang (Nokia)
2025-09-24 13:20 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Chia-Yu Chang (Nokia) @ 2025-09-24 12:25 UTC (permalink / raw)
To: Ilpo Järvinen, Dan Carpenter; +Cc: netdev@vger.kernel.org
> -----Original Message-----
> From: Ilpo Järvinen <ij@kernel.org>
> Sent: Tuesday, September 23, 2025 8:23 PM
> To: Dan Carpenter <dan.carpenter@linaro.org>; Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> Cc: netdev@vger.kernel.org
> Subject: Re: [bug report] tcp: accecn: AccECN option
>
>
> CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
>
>
>
> On Tue, 23 Sep 2025, Dan Carpenter wrote:
>
> > Hello Ilpo Järvinen,
> >
> > Commit b5e74132dfbe ("tcp: accecn: AccECN option") from Sep 16, 2025
> > (linux-next), leads to the following Smatch static checker warning:
> >
> > net/ipv4/tcp_output.c:747 tcp_options_write()
> > error: we previously assumed 'tp' could be null (see line 711)
> >
> > net/ipv4/tcp_output.c
> > 630 static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> > 631 const struct tcp_request_sock *tcprsk,
> > 632 struct tcp_out_options *opts,
> > 633 struct tcp_key *key)
> > 634 {
> > 635 u8 leftover_highbyte = TCPOPT_NOP; /* replace 1st NOP if avail */
> > 636 u8 leftover_lowbyte = TCPOPT_NOP; /* replace 2nd NOP in succession */
> > 637 __be32 *ptr = (__be32 *)(th + 1);
> > 638 u16 options = opts->options; /* mungable copy */
> > 639
> > 640 if (tcp_key_is_md5(key)) {
> > 641 *ptr++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) |
> > 642 (TCPOPT_MD5SIG << 8) | TCPOLEN_MD5SIG);
> > 643 /* overload cookie hash location */
> > 644 opts->hash_location = (__u8 *)ptr;
> > 645 ptr += 4;
> > 646 } else if (tcp_key_is_ao(key)) {
> > 647 ptr = process_tcp_ao_options(tp, tcprsk, opts, key, ptr);
> > ^^ Sometimes
> > dereferenced here.
> >
> > 648 }
> > 649 if (unlikely(opts->mss)) {
> > 650 *ptr++ = htonl((TCPOPT_MSS << 24) |
> > 651 (TCPOLEN_MSS << 16) |
> > 652 opts->mss);
> > 653 }
> > 654
> > 655 if (likely(OPTION_TS & options)) {
> > 656 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> > 657 *ptr++ = htonl((TCPOPT_SACK_PERM << 24) |
> > 658 (TCPOLEN_SACK_PERM << 16) |
> > 659 (TCPOPT_TIMESTAMP << 8) |
> > 660 TCPOLEN_TIMESTAMP);
> > 661 options &= ~OPTION_SACK_ADVERTISE;
> > 662 } else {
> > 663 *ptr++ = htonl((TCPOPT_NOP << 24) |
> > 664 (TCPOPT_NOP << 16) |
> > 665 (TCPOPT_TIMESTAMP << 8) |
> > 666 TCPOLEN_TIMESTAMP);
> > 667 }
> > 668 *ptr++ = htonl(opts->tsval);
> > 669 *ptr++ = htonl(opts->tsecr);
> > 670 }
> > 671
> > 672 if (OPTION_ACCECN & options) {
> > 673 const u32 *ecn_bytes = opts->use_synack_ecn_bytes ?
> > 674 synack_ecn_bytes :
> > 675 tp->received_ecn_bytes;
> > ^^^^ Dereference
>
> Hi Dan,
>
> While it is long ago I made these changes (they might have changed a little from that), I can say this part is going to be extremely tricky for static checkers because TCP state machine(s) are quite complex.
>
> TCP options can be written to a packet when tp has not yet been created (during handshake) as well as after creation of tp using this same function. Not all combinations are possible because handshake has to complete before some things are enabled.
>
> Without checking this myself, my assumption is that ->use_synack_ecn_bytes is set when we don't have tp available yet as SYNACKs relate to handshake.
> So the tp check is likely there even if not literally written.
>
> Chia-Yu, could you please check these cases for the parts that new code was introduced whether tp can be NULL? I think this particular line is the most likely one to be wrong if something is, that is, can OPTION_ACCECN be set while use_synack_ecn_bytes is not when tp is not yet there.
Hi Ilpo and Dan,
I've checked that OPTION_ACCECN and use_synack_ecn_bytes will always be set at the same time.
The case you said (OPTION_ACCECN is 1, but use_synack_ecn_bytes is 0) can only happen when tp is set, because this is already ESTABLISHED state (see tcp_established_options in tcp_output.c).
So, I would say this is ok.
But if this is indeed a concern for the checker, just add another "if (tp)".
Chia-Yu
>
> > 676 const u8 ect0_idx = INET_ECN_ECT_0 - 1;
> > 677 const u8 ect1_idx = INET_ECN_ECT_1 - 1;
> > 678 const u8 ce_idx = INET_ECN_CE - 1;
> > 679 u32 e0b;
> > 680 u32 e1b;
> > 681 u32 ceb;
> > 682 u8 len;
> > 683
> > 684 e0b = ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
> > 685 e1b = ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
> > 686 ceb = ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;
> > 687 len = TCPOLEN_ACCECN_BASE +
> > 688 opts->num_accecn_fields * TCPOLEN_ACCECN_PERFIELD;
> > 689
> > 690 if (opts->num_accecn_fields == 2) {
> > 691 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > 692 ((e1b >> 8) & 0xffff));
> > 693 *ptr++ = htonl(((e1b & 0xff) << 24) |
> > 694 (ceb & 0xffffff));
> > 695 } else if (opts->num_accecn_fields == 1) {
> > 696 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > 697 ((e1b >> 8) & 0xffff));
> > 698 leftover_highbyte = e1b & 0xff;
> > 699 leftover_lowbyte = TCPOPT_NOP;
> > 700 } else if (opts->num_accecn_fields == 0) {
> > 701 leftover_highbyte = TCPOPT_ACCECN1;
> > 702 leftover_lowbyte = len;
> > 703 } else if (opts->num_accecn_fields == 3) {
> > 704 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > 705 ((e1b >> 8) & 0xffff));
> > 706 *ptr++ = htonl(((e1b & 0xff) << 24) |
> > 707 (ceb & 0xffffff));
> > 708 *ptr++ = htonl(((e0b & 0xffffff) << 8) |
> > 709 TCPOPT_NOP);
> > 710 }
> > 711 if (tp) {
> > ^^
> > Here we assume tp can be NULL
> >
> > 712 tp->accecn_minlen = 0;
> > 713 tp->accecn_opt_tstamp = tp->tcp_mstamp;
> > 714 if (tp->accecn_opt_demand)
> > 715 tp->accecn_opt_demand--;
> > 716 }
> > 717 }
> > 718
> > 719 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> > 720 *ptr++ = htonl((leftover_highbyte << 24) |
> > 721 (leftover_lowbyte << 16) |
> > 722 (TCPOPT_SACK_PERM << 8) |
> > 723 TCPOLEN_SACK_PERM);
> > 724 leftover_highbyte = TCPOPT_NOP;
> > 725 leftover_lowbyte = TCPOPT_NOP;
> > 726 }
> > 727
> > 728 if (unlikely(OPTION_WSCALE & options)) {
> > 729 u8 highbyte = TCPOPT_NOP;
> > 730
> > 731 /* Do not split the leftover 2-byte to fit into a single
> > 732 * NOP, i.e., replace this NOP only when 1 byte is leftover
> > 733 * within leftover_highbyte.
> > 734 */
> > 735 if (unlikely(leftover_highbyte != TCPOPT_NOP &&
> > 736 leftover_lowbyte == TCPOPT_NOP)) {
> > 737 highbyte = leftover_highbyte;
> > 738 leftover_highbyte = TCPOPT_NOP;
> > 739 }
> > 740 *ptr++ = htonl((highbyte << 24) |
> > 741 (TCPOPT_WINDOW << 16) |
> > 742 (TCPOLEN_WINDOW << 8) |
> > 743 opts->ws);
> > 744 }
> > 745
> > 746 if (unlikely(opts->num_sack_blocks)) {
> > --> 747 struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> > ^^^^^^^^^^^^^^^^
> > Unchecked dereference here.
> >
> > 748 tp->duplicate_sack : tp->selective_acks;
> > 749 int this_sack;
> > 750
> > 751 *ptr++ = htonl((leftover_highbyte << 24) |
> > 752 (leftover_lowbyte << 16) |
> > 753 (TCPOPT_SACK << 8) |
> > 754 (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
> > 755 TCPOLEN_SACK_PERBLOCK)));
> > 756 leftover_highbyte = TCPOPT_NOP;
> > 757 leftover_lowbyte = TCPOPT_NOP;
> > 758
> > 759 for (this_sack = 0; this_sack < opts->num_sack_blocks;
> > 760 ++this_sack) {
> > 761 *ptr++ = htonl(sp[this_sack].start_seq);
> > 762 *ptr++ = htonl(sp[this_sack].end_seq);
> > 763 }
> > 764
> > 765 tp->rx_opt.dsack = 0;
> > 766 } else if (unlikely(leftover_highbyte != TCPOPT_NOP ||
> > 767 leftover_lowbyte != TCPOPT_NOP)) {
> > 768 *ptr++ = htonl((leftover_highbyte << 24) |
> > 769 (leftover_lowbyte << 16) |
> > 770 (TCPOPT_NOP << 8) |
> > 771 TCPOPT_NOP);
> > 772 leftover_highbyte = TCPOPT_NOP;
> > 773 leftover_lowbyte = TCPOPT_NOP;
> > 774 }
> > 775
> > 776 if (unlikely(OPTION_FAST_OPEN_COOKIE & options)) {
> > 777 struct tcp_fastopen_cookie *foc = opts->fastopen_cookie;
> > 778 u8 *p = (u8 *)ptr;
> > 779 u32 len; /* Fast Open option length */
> > 780
> > 781 if (foc->exp) {
> > 782 len = TCPOLEN_EXP_FASTOPEN_BASE + foc->len;
> > 783 *ptr = htonl((TCPOPT_EXP << 24) | (len << 16) |
> > 784 TCPOPT_FASTOPEN_MAGIC);
> > 785 p += TCPOLEN_EXP_FASTOPEN_BASE;
> > 786 } else {
> > 787 len = TCPOLEN_FASTOPEN_BASE + foc->len;
> > 788 *p++ = TCPOPT_FASTOPEN;
> > 789 *p++ = len;
> > 790 }
> > 791
> > 792 memcpy(p, foc->val, foc->len);
> > 793 if ((len & 3) == 2) {
> > 794 p[foc->len] = TCPOPT_NOP;
> > 795 p[foc->len + 1] = TCPOPT_NOP;
> > 796 }
> > 797 ptr += (len + 3) >> 2;
> > 798 }
> > 799
> > 800 smc_options_write(ptr, &options);
> > 801
> > 802 mptcp_options_write(th, ptr, tp, opts);
> > ^^ The last dereference
> > is checked for NULL but the others aren't.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] tcp: accecn: AccECN option
2025-09-24 12:25 ` Chia-Yu Chang (Nokia)
@ 2025-09-24 13:20 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2025-09-24 13:20 UTC (permalink / raw)
To: Chia-Yu Chang (Nokia); +Cc: Ilpo Järvinen, netdev@vger.kernel.org
On Wed, Sep 24, 2025 at 12:25:16PM +0000, Chia-Yu Chang (Nokia) wrote:
> > -----Original Message-----
> > From: Ilpo Järvinen <ij@kernel.org>
> > Sent: Tuesday, September 23, 2025 8:23 PM
> > To: Dan Carpenter <dan.carpenter@linaro.org>; Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > Cc: netdev@vger.kernel.org
> > Subject: Re: [bug report] tcp: accecn: AccECN option
> >
> >
> > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> >
> >
> >
> > On Tue, 23 Sep 2025, Dan Carpenter wrote:
> >
> > > Hello Ilpo Järvinen,
> > >
> > > Commit b5e74132dfbe ("tcp: accecn: AccECN option") from Sep 16, 2025
> > > (linux-next), leads to the following Smatch static checker warning:
> > >
> > > net/ipv4/tcp_output.c:747 tcp_options_write()
> > > error: we previously assumed 'tp' could be null (see line 711)
> > >
> > > net/ipv4/tcp_output.c
> > > 630 static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> > > 631 const struct tcp_request_sock *tcprsk,
> > > 632 struct tcp_out_options *opts,
> > > 633 struct tcp_key *key)
> > > 634 {
> > > 635 u8 leftover_highbyte = TCPOPT_NOP; /* replace 1st NOP if avail */
> > > 636 u8 leftover_lowbyte = TCPOPT_NOP; /* replace 2nd NOP in succession */
> > > 637 __be32 *ptr = (__be32 *)(th + 1);
> > > 638 u16 options = opts->options; /* mungable copy */
> > > 639
> > > 640 if (tcp_key_is_md5(key)) {
> > > 641 *ptr++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) |
> > > 642 (TCPOPT_MD5SIG << 8) | TCPOLEN_MD5SIG);
> > > 643 /* overload cookie hash location */
> > > 644 opts->hash_location = (__u8 *)ptr;
> > > 645 ptr += 4;
> > > 646 } else if (tcp_key_is_ao(key)) {
> > > 647 ptr = process_tcp_ao_options(tp, tcprsk, opts, key, ptr);
> > > ^^ Sometimes
> > > dereferenced here.
> > >
> > > 648 }
> > > 649 if (unlikely(opts->mss)) {
> > > 650 *ptr++ = htonl((TCPOPT_MSS << 24) |
> > > 651 (TCPOLEN_MSS << 16) |
> > > 652 opts->mss);
> > > 653 }
> > > 654
> > > 655 if (likely(OPTION_TS & options)) {
> > > 656 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> > > 657 *ptr++ = htonl((TCPOPT_SACK_PERM << 24) |
> > > 658 (TCPOLEN_SACK_PERM << 16) |
> > > 659 (TCPOPT_TIMESTAMP << 8) |
> > > 660 TCPOLEN_TIMESTAMP);
> > > 661 options &= ~OPTION_SACK_ADVERTISE;
> > > 662 } else {
> > > 663 *ptr++ = htonl((TCPOPT_NOP << 24) |
> > > 664 (TCPOPT_NOP << 16) |
> > > 665 (TCPOPT_TIMESTAMP << 8) |
> > > 666 TCPOLEN_TIMESTAMP);
> > > 667 }
> > > 668 *ptr++ = htonl(opts->tsval);
> > > 669 *ptr++ = htonl(opts->tsecr);
> > > 670 }
> > > 671
> > > 672 if (OPTION_ACCECN & options) {
> > > 673 const u32 *ecn_bytes = opts->use_synack_ecn_bytes ?
> > > 674 synack_ecn_bytes :
> > > 675 tp->received_ecn_bytes;
> > > ^^^^ Dereference
> >
> > Hi Dan,
> >
> > While it is long ago I made these changes (they might have changed a little from that), I can say this part is going to be extremely tricky for static checkers because TCP state machine(s) are quite complex.
> >
> > TCP options can be written to a packet when tp has not yet been created (during handshake) as well as after creation of tp using this same function. Not all combinations are possible because handshake has to complete before some things are enabled.
> >
> > Without checking this myself, my assumption is that ->use_synack_ecn_bytes is set when we don't have tp available yet as SYNACKs relate to handshake.
> > So the tp check is likely there even if not literally written.
> >
> > Chia-Yu, could you please check these cases for the parts that new code was introduced whether tp can be NULL? I think this particular line is the most likely one to be wrong if something is, that is, can OPTION_ACCECN be set while use_synack_ecn_bytes is not when tp is not yet there.
>
> Hi Ilpo and Dan,
>
> I've checked that OPTION_ACCECN and use_synack_ecn_bytes will always be set at the same time.
> The case you said (OPTION_ACCECN is 1, but use_synack_ecn_bytes is 0) can only happen when tp is set, because this is already ESTABLISHED state (see tcp_established_options in tcp_output.c).
>
> So, I would say this is ok.
Thanks!
> But if this is indeed a concern for the checker, just add another "if (tp)".
>
Please, don't add anything just for the checker. These are a one time
only email. Old warnings are false positives because kernel developers
are good about fixing actual issues.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-09-24 13:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-23 11:27 [bug report] tcp: accecn: AccECN option Dan Carpenter
2025-09-23 18:23 ` Ilpo Järvinen
2025-09-24 12:25 ` Chia-Yu Chang (Nokia)
2025-09-24 13:20 ` Dan Carpenter
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).