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

Wireshark doesn't understand MP_CAPABLE C flag #190

Closed
2 tasks
mjmartineau opened this issue May 6, 2021 · 9 comments
Closed
2 tasks

Wireshark doesn't understand MP_CAPABLE C flag #190

mjmartineau opened this issue May 6, 2021 · 9 comments
Assignees
@RuiCunhaM
Labels
enhancement
Projects
MPTCP Next (5.15)

Comments

@mjmartineau
Copy link
Member

mjmartineau commented May 6, 2021 •
edited by matttbe
Loading

The RFC 8684 C flag is still interpreted as a reserved flag bit in Wireshark 3.4.4, needs to be updated for "don't join on initial subflow addr/port" C flag.

  • Wireshark support
  • TCPDump support
@matttbe matttbe added this to To do in MPTCP Next (5.14) via automation May 6, 2021
@matttbe matttbe removed this from To do in MPTCP Next (5.14) Jul 15, 2021
@matttbe matttbe added this to To do in MPTCP Next (5.15) via automation Jul 15, 2021
@RuiCunhaM
Copy link

Hi!
So I noticed this issue when doing some tests, and I was about to submit a fix for Wireshark, but I'm in doubt about the best designation for the flag. From the RFC I was going with "No more Subflows", but reading this issue and the

"don't join on initial subflow addr/port"

part I am no longer sure what is the most intuitive designation.
Ideas in which one is the best?

@matttbe
Copy link
Member

matttbe commented Aug 16, 2021

Hi @RuiCunhaM

Thank you for looking at that.

From the RFC I was going with "No more Subflows", but reading this issue (...) I am no longer sure what is the most intuitive designation.

Good point, indeed this option is not clear. When a host set this flag in the MP_CAPABLE option, it means "do NOT send an additional subflow request to this source IP and port":

The third bit, labeled "C", is set to 1 to indicate that the sender of this option will not accept additional MPTCP subflows to the source address and port, and therefore the receiver MUST NOT try to open any additional subflows toward this address and port.

I don't know how many words we can have here but it is not easy to only use a few words while still having a clear explanation :)
Maybe: No more subflows on initial addr/port if it is not too long!
If you have any other suggestions, feel free to share ;-)

@RuiCunhaM
Copy link

Hi @matttbe, thanks for answering.

To be honest in Wireshark there is a decent amount of room to work with, so I believe your suggestion fits perfectly. I definitely agree that saying "initial addr/port" it's important to reflect what is stated in the RFC.

In TCPDump however, now that I also looked at it, they seem to be a bit more conservative, so it will be a bit more difficult to fully describe the flag.

@matttbe
Copy link
Member

matttbe commented Aug 16, 2021

To be honest in Wireshark there is a decent amount of room to work with, so I believe your suggestion fits perfectly. I definitely agree that saying "initial addr/port" it's important to reflect what is stated in the RFC.

Great, let's try this then!

In TCPDump however, now that I also looked at it, they seem to be a bit more conservative, so it will be a bit more difficult to fully describe the flag.

Yes, I was thinking about that too. We might have to mention a very few words I guess, something like C flag or just C.

@RuiCunhaM
Copy link

Great, let's try this then!

https://gitlab.com/wireshark/wireshark/-/merge_requests/3890

something like C flag or just C

Probably, I don't think we can get a way with much more than that.

@matttbe
Copy link
Member

matttbe commented Aug 16, 2021

Thanks!

I hope you don't mind if I assign this task to you not to have someone else working on it (even if I hope people read comments before starting implementing something ;-) )

@RuiCunhaM
Copy link

I hope you don't mind if I assign this task to you not to have someone else working on it (even if I hope people read comments before starting implementing something ;-) )

Haha no problem at all

@RuiCunhaM RuiCunhaM mentioned this issue Aug 16, 2021
Closed
geraldcombs pushed a commit to wireshark/wireshark that referenced this issue Aug 16, 2021
@RuiCunhaM
MPTCP: Add support for MP_CAPABLE C Flag
f5dc703 
Interpret C Flag as described in section 3.1 RFC8684

Issue: multipath-tcp/mptcp_net-next#190
@RuiCunhaM
Copy link

Everything has been merged:

TCPDump: the-tcpdump-group/tcpdump#934

Wireshark: https://gitlab.com/wireshark/wireshark/-/merge_requests/3890

I think this can be closed.

@matttbe
Copy link
Member

matttbe commented Aug 19, 2021

Great, thank you, it looks good!

Do not hesitate if you see any missing features or issues ;-)

@matttbe matttbe closed this as completed Aug 19, 2021
MPTCP Next (5.15) automation moved this from To do to Done Aug 19, 2021
matttbe pushed a commit that referenced this issue Feb 13, 2023
Merge branch 'Support bpf trampoline for s390x'
c1a3daf 
Ilya Leoshkevich says:

====================

v2: https://lore.kernel.org/bpf/20230128000650.1516334-1-iii@linux.ibm.com/#t
v2 -> v3:
- Make __arch_prepare_bpf_trampoline static.
  (Reported-by: kernel test robot <lkp@intel.com>)
- Support both old- and new- style map definitions in sk_assign. (Alexei)
- Trim DENYLIST.s390x. (Alexei)
- Adjust s390x vmlinux path in vmtest.sh.
- Drop merged fixes.

v1: https://lore.kernel.org/bpf/20230125213817.1424447-1-iii@linux.ibm.com/#t
v1 -> v2:
- Fix core_read_macros, sk_assign, test_profiler, test_bpffs (24/31;
  I'm not quite happy with the fix, but don't have better ideas),
  and xdp_synproxy. (Andrii)
- Prettify liburandom_read and verify_pkcs7_sig fixes. (Andrii)
- Fix bpf_usdt_arg using barrier_var(); prettify barrier_var(). (Andrii)
- Change BPF_MAX_TRAMP_LINKS to enum and query it using BTF. (Andrii)
- Improve bpf_jit_supports_kfunc_call() description. (Alexei)
- Always check sign_extend() return value.
- Cc: Alexander Gordeev.

Hi,

This series implements poke, trampoline, kfunc, and mixing subprogs
and tailcalls on s390x.

The following failures still remain:

#82      get_stack_raw_tp:FAIL
get_stack_print_output:FAIL:user_stack corrupted user stack
Known issue:
We cannot reliably unwind userspace on s390x without DWARF.

#101     ksyms_module:FAIL
address of kernel function bpf_testmod_test_mod_kfunc is out of range
Known issue:
Kernel and modules are too far away from each other on s390x.

#190     stacktrace_build_id:FAIL
Known issue:
We cannot reliably unwind userspace on s390x without DWARF.

#281     xdp_metadata:FAIL
See patch 6.

None of these seem to be due to the new changes.

Best regards,
Ilya
====================

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
jenkins-tessares pushed a commit that referenced this issue Jul 6, 2023
@zhangyi089 @tytso
jbd2: skip reading super block if it has been verified
31464ab 
We got a NULL pointer dereference issue below while running generic/475
I/O failure pressure test.

 BUG: kernel NULL pointer dereference, address: 0000000000000000
 #PF: supervisor write access in kernel mode
 #PF: error_code(0x0002) - not-present page
 PGD 0 P4D 0
 Oops: 0002 [#1] PREEMPT SMP PTI
 CPU: 1 PID: 15600 Comm: fsstress Not tainted 6.4.0-rc5-xfstests-00055-gd3ab1bca26b4 #190
 RIP: 0010:jbd2_journal_set_features+0x13d/0x430
 ...
 Call Trace:
  <TASK>
  ? __die+0x23/0x60
  ? page_fault_oops+0xa4/0x170
  ? exc_page_fault+0x67/0x170
  ? asm_exc_page_fault+0x26/0x30
  ? jbd2_journal_set_features+0x13d/0x430
  jbd2_journal_revoke+0x47/0x1e0
  __ext4_forget+0xc3/0x1b0
  ext4_free_blocks+0x214/0x2f0
  ext4_free_branches+0xeb/0x270
  ext4_ind_truncate+0x2bf/0x320
  ext4_truncate+0x1e4/0x490
  ext4_handle_inode_extension+0x1bd/0x2a0
  ? iomap_dio_complete+0xaf/0x1d0

The root cause is the journal super block had been failed to write out
due to I/O fault injection, it's uptodate bit was cleared by
end_buffer_write_sync() and didn't reset yet in jbd2_write_superblock().
And it raced by journal_get_superblock()->bh_read(), unfortunately, the
read IO is also failed, so the error handling in
journal_fail_superblock() unexpectedly clear the journal->j_sb_buffer,
finally lead to above NULL pointer dereference issue.

If the journal super block had been read and verified, there is no need
to call bh_read() read it again even if it has been failed to written
out. So the fix could be simply move buffer_verified(bh) in front of
bh_read(). Also remove a stale comment left in
jbd2_journal_check_used_features().

Fixes: 51bacdba23d8 ("jbd2: factor out journal initialization from journal_get_superblock()")
Reported-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Link: https://lore.kernel.org/r/20230616015547.3155195-1-yi.zhang@huaweicloud.com
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RuiCunhaM RuiCunhaM

Labels
enhancement
Projects
No open projects
MPTCP Next (5.15)
Done
Milestone
No milestone
Development

No branches or pull requests
3 participants
@matttbe @mjmartineau @RuiCunhaM