Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SCTP: Support printing some latest chunk types #995

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

SCTP: Support printing some latest chunk types #995

wants to merge 5 commits into from

Conversation

CacheUseOnly
Copy link

@CacheUseOnly CacheUseOnly commented Jul 5, 2022 •
edited
Loading

Some new SCTP chunk types are implemented in Linux kernel but not yet supported by tcpdump to print them appropriately. These patches add corresponding supports for chunks below:

Signed-off-by: Yuxuan Luo <luoyuxuan.carl@gmail.com>

@guyharris guyharris mentioned this pull request Jul 6, 2022
Closed
guyharris
guyharris reviewed Jul 6, 2022
View reviewed changes
print-sctp.c Outdated Show resolved Hide resolved
@CacheUseOnly CacheUseOnly marked this pull request as ready for review July 6, 2022 14:04
@CacheUseOnly
Copy link
Author

@guyharris could you please tell me which part of the code stops the merging process? I will update it as soon as possible.

@lxin
Copy link

lxin commented Aug 15, 2022

From the SCTP side, this patchset looks good to me.

Reviewed-by: Xin Long lucien.xin@gmail.com

@CacheUseOnly
Copy link
Author

Hi tcpdump maintainers, this patch is reviewed and has passed all checks, is there any concerns stopping it going to master branch?

@fxlb fxlb force-pushed the revision4.2 branch 2 times, most recently from e36067f to 17da16b Compare May 1, 2023 08:40
@fxlb
Copy link
Member

fxlb commented May 3, 2023

Thanks for preparing this PR.
Each commit should have a pcap test file with eventually several output (-v, -vv, etc.).
(See CONTRIBUTING.md item 6.)

fxlb
fxlb reviewed May 5, 2023
View reviewed changes
print-sctp.c Show resolved Hide resolved
fxlb
fxlb reviewed May 5, 2023
View reviewed changes
print-sctp.c Show resolved Hide resolved
@CacheUseOnly CacheUseOnly force-pushed the revision4.2 branch 3 times, most recently from 5979459 to 798e39e Compare May 23, 2023 14:26
@fxlb
Copy link
Member

fxlb commented May 23, 2023

See CONTRIBUTING.md item 6 about TESTLIST usage.

@CacheUseOnly
Copy link
Author

A flaw has been found regarding this PR:

The the-tcpdump-group fuzzing process has found a long or infinite
loop in this code, with the following command and the attached pcap
file.
reading from file test_pb_extract_loop_sctp_1a.pcap, link-type EN10MB
(Ethernet), snapshot length 18546948
1 [Error converting time] IP (tos 0x0, ttl 206, id 256, offset 0,
flags [none], proto SCTP (132), length 16489, bad cksum ce29
(->5bfb)!)
13.20.64.1.53 > 1.1.1.0.1: sctp (1) [ASCONF-ACK] [SEQ: 0] [#0] [#0]
[#0] [#0] [#0] [#0] [#0] [#0] [#0] [#0] [#0] [#0] [#0] [#0] [#0] [#0]

Will work on fixing this issue later, please do not merge this PR right now.

@CacheUseOnly
Copy link
Author

This latest patch set has solved the bug mentioned above and fixed other miscellaneous issues.

@fxlb
Copy link
Member

fxlb commented Dec 11, 2023 •
edited
Loading

Fuzzing found a problem. Setting on draft. Private mail sent.

@fxlb fxlb marked this pull request as draft December 11, 2023 09:50
@CacheUseOnly
Copy link
Author

Fuzzing found a problem. Setting on draft. Private mail sent.

I have not received the email, do you mind send it again to luoyuxuan.carl@gmail.com? Appreciate that!

@fxlb
Copy link
Member

fxlb commented Jan 18, 2024

send it again

Done.

@CacheUseOnly CacheUseOnly marked this pull request as ready for review January 23, 2024 23:47
@CacheUseOnly
Copy link
Author

Solved a merge conflict with master branch.

@infrastation
Copy link
Member

Thank you. The master branch now uses -tttt timestamps, so the new tests will need a trivial update using update-test.sh <testname>.

Yuxuan Luo added 5 commits March 23, 2024 15:08
@CacheUseOnly
SCTP: Support I-DATA chunk
12bf71d 
Add support for printing SCTP I-DATA chunk based on RFC8260 section 2.1

Prints payload when vflag level is greater than 1.

Example:
[I-DATA] (B) [TSN: 1522458896] [SID: 0] [MID: 0] [PPID 0x0]
[I-DATA] (E) [TSN: 1522458897] [SID: 0] [MID: 0] [FSN: 0]
@CacheUseOnly
SCTP: Support I-FORWARD chunk
762ee2d 
Add support for printing I-FORWARD chunk based on RFC8260 section 2.3.

Remove REL_CTL (RFC3758) since it is obsolete and it uses the value '0xc2'
of I-FORWARD for CNTL_ACK.

Print stream IDs and message IDs with `-vv` set.

Example:
`-v`: [I-FORWARD-FSN] [TSN: 1584188225]
`-vv`: [I-FORWARD-FSN] [TSN: 2195717635] [SID: 0] [MID: 2]
@CacheUseOnly
SCTP: Support RE-CONFIG chunk
541b801 
Add support for printing RE-CONFIG chunk based on RFC6525 section3.1.

Prints all optional parameters when `-vv` is set.

Example:
[RE-CONFIG] [OUT SSN RESET: REQ SEQ:, RES SEQ:, Last TSN:, SID 0 1]
@CacheUseOnly
SCTP: Support ASCONF/-ACK chunk
fe1cb32 
Add support for printing ASCONF and ASCONF-ACK chunk based on RFC5061.

Remove REL_CNTL(0xc1) because it's obsolete and conflicts with ASCONF.

Prints all ASCONF parameters with `-vv` set.

Example:
`-v`: [ASCONF] [SEQ: .., ADDR: 192...] [DEL ADDR]
`-vv`:[ASCONF] [SEQ: .., ADDR: 192...] [DEL ADDR: C-ID: 0, ADDR: 192...]
[ASCONF-ACK] [SEQ: 4161214189]
@CacheUseOnly
SCTP: Support PAD chunk
be5667b 
Add support for printing PAD chunk based on RFC4820 section3.

Example:
[PAD]

Signed-off-by: Yuxuan Luo <yuxuan.luo@canonical.com>
@CacheUseOnly
Copy link
Author

Thank you. The master branch now uses -tttt timestamps, so the new tests will need a trivial update using update-test.sh <testname>.

Thanks for noticing. All the testcases .out files have been updated using ./update-test.sh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fxlb fxlb fxlb left review comments

@guyharris guyharris Awaiting requested review from guyharris

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@CacheUseOnly @lxin @fxlb @infrastation @guyharris