[PATCH mptcp-net v4 00/23] mptcp: fix endpoints with 'signal' and 'subflow' flags

Matthieu Baerts (NGI0) posted 23 patches 2 months, 3 weeks ago
Failed in applying to current master (apply log)
There is a newer version of this series
net/mptcp/options.c                             |   3 +-
net/mptcp/pm.c                                  |  24 ---
net/mptcp/pm_netlink.c                          | 210 +++++++++++++++---------
net/mptcp/pm_userspace.c                        |  19 +--
net/mptcp/protocol.h                            |  13 +-
net/mptcp/subflow.c                             |  29 ++--
tools/testing/selftests/net/mptcp/mptcp_join.sh | 131 ++++++++++++---
7 files changed, 258 insertions(+), 171 deletions(-)
[PATCH mptcp-net v4 00/23] mptcp: fix endpoints with 'signal' and 'subflow' flags
Posted by Matthieu Baerts (NGI0) 2 months, 3 weeks ago
When looking at improving the user experience around the MPTCP endpoints
setup, I noticed that setting an endpoint with both the 'signal' and the
'subflow' flags -- as it has been done in the past by users according to
bug reports we got -- were resulting on only announcing the endpoint,
but not using it to create subflows: the 'subflow' flag was then
ignored.

My initial thought was to modify IPRoute2 to warn the user when the two
flags were set, but it doesn't sound normal to ignore one of them. I
then looked at modifying the kernel not to allow having the two flags
set, but when discussing about that with Mat, we thought it was maybe
not ideal to do that, as there might be use-cases, we might break some
configs, and it was working before apparently. So instead, I fixed the
support on the kernel side (patch 5) using Paolo's suggestion. This also
includes a fix on the options side (patch 1), an explicit deny of some
options combinations (patch 2), and some refactoring (patches 3 and 4).

While at it, I added a new selftest (patch 7) to validate this case --
including a modification of the chk_add_nr helper to inverse the sides
were the counters are checked (patch 6) -- and allowed ADD_ADDR echo
just after the MP_JOIN 3WHS.

While working on that, I also noticed that re-using IDs were not
possible in some cases -- see patches 8, 10 and 12 -- and the accounting
was not correct in some other cases -- see patches 14 to 17.

The selftests modification have the same Fixes tag as the previous
commit, but they should not get the 'Cc: Stable' one later: if the
backport can work, that's not, if not, no need to worry, many CIs will
use the selftests from the last stable version to validate previous
stable releases.

The last patches don't have any modifications of the selftests attached
to them, because the current ones were producing the new WARN() that
have just been added.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v4:
- Patch 19: check for msk->first != NULL
- New patches 21-22
- Imported patch 23: might be easier to review all of them, then this
  single one alone, while it depends on the previous ones.
- Link to v3: https://lore.kernel.org/r/20240719-mptcp-pm-avail-v3-0-e96b5591ced3@kernel.org

Changes in v3:
- Small changes in patches 10 and 14, see individual changelog (Geliang)
- New patches 18-20: small fixes
- Link to v2: https://lore.kernel.org/r/20240715-mptcp-pm-avail-v2-0-fc5153bd1f6e@kernel.org

Changes in v2:
- Do not split id_avail_bitmap per target in patch 5 (Paolo)
- Explicit deny (patch 2), reduce indentation (patch 3), stop earlier
  (patch 4) (Paolo)
- New fixes and tests (patches 8-17).
- Link to v1: https://lore.kernel.org/r/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org

---
Matthieu Baerts (NGI0) (23):
      mptcp: fully established after ADD_ADDR echo on MPJ
      mptcp: pm: deny endp with signal + subflow + port
      mptcp: pm: reduce indentation blocks
      mptcp: pm: don't try to create sf if alloc failed
      mptcp: pm: do not ignore 'subflow' if 'signal' flag is also set
      selftests: mptcp: join: ability to invert ADD_ADDR check
      selftests: mptcp: join: test both signal & subflow
      mptcp: pm: re-using ID of unused removed ADD_ADDR
      selftests: mptcp: join: check re-using ID of unused ADD_ADDR
      mptcp: pm: re-using ID of unused removed subflows
      selftests: mptcp: join: check re-using ID of closed subflow
      mptcp: pm: re-using ID of unused flushed subflows
      selftests: mptcp: join: test for flush/re-add endpoints
      mptcp: pm: remove mptcp_pm_remove_subflow()
      mptcp: pm: only mark 'subflow' endp as available
      mptcp: pm: only decrement add_addr_accepted for MPJ req
      mptcp: pm: check add_addr_accept_max before accepting new ADD_ADDR
      mptcp: pm: only in-kernel cannot have entries with ID 0
      mptcp: pm: fullmesh: select the right ID later
      selftests: mptcp: join: validate fullmesh endp on 1st sf
      mptcp: pm: avoid possible UaF whend selecting endp
      mptcp: pm: reuse ID 0 after delete and re-add
      mptcp: pm: reduce entries iterations on connect

 net/mptcp/options.c                             |   3 +-
 net/mptcp/pm.c                                  |  24 ---
 net/mptcp/pm_netlink.c                          | 210 +++++++++++++++---------
 net/mptcp/pm_userspace.c                        |  19 +--
 net/mptcp/protocol.h                            |  13 +-
 net/mptcp/subflow.c                             |  29 ++--
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 131 ++++++++++++---
 7 files changed, 258 insertions(+), 171 deletions(-)
---
base-commit: 140ff27ee47286bb0a270f3aa275fc319724da8d
change-id: 20240620-mptcp-pm-avail-f5e3957be441

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>
Re: [PATCH mptcp-net v4 00/23] mptcp: fix endpoints with 'signal' and 'subflow' flags
Posted by Mat Martineau 2 months, 3 weeks ago
On Mon, 22 Jul 2024, Matthieu Baerts (NGI0) wrote:

> When looking at improving the user experience around the MPTCP endpoints
> setup, I noticed that setting an endpoint with both the 'signal' and the
> 'subflow' flags -- as it has been done in the past by users according to
> bug reports we got -- were resulting on only announcing the endpoint,
> but not using it to create subflows: the 'subflow' flag was then
> ignored.
>
> My initial thought was to modify IPRoute2 to warn the user when the two
> flags were set, but it doesn't sound normal to ignore one of them. I
> then looked at modifying the kernel not to allow having the two flags
> set, but when discussing about that with Mat, we thought it was maybe
> not ideal to do that, as there might be use-cases, we might break some
> configs, and it was working before apparently. So instead, I fixed the
> support on the kernel side (patch 5) using Paolo's suggestion. This also
> includes a fix on the options side (patch 1), an explicit deny of some
> options combinations (patch 2), and some refactoring (patches 3 and 4).
>
> While at it, I added a new selftest (patch 7) to validate this case --
> including a modification of the chk_add_nr helper to inverse the sides
> were the counters are checked (patch 6) -- and allowed ADD_ADDR echo
> just after the MP_JOIN 3WHS.
>
> While working on that, I also noticed that re-using IDs were not
> possible in some cases -- see patches 8, 10 and 12 -- and the accounting
> was not correct in some other cases -- see patches 14 to 17.
>
> The selftests modification have the same Fixes tag as the previous
> commit, but they should not get the 'Cc: Stable' one later: if the
> backport can work, that's not, if not, no need to worry, many CIs will
> use the selftests from the last stable version to validate previous
> stable releases.
>
> The last patches don't have any modifications of the selftests attached
> to them, because the current ones were producing the new WARN() that
> have just been added.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Changes in v4:
> - Patch 19: check for msk->first != NULL
> - New patches 21-22
> - Imported patch 23: might be easier to review all of them, then this
>  single one alone, while it depends on the previous ones.
> - Link to v3: https://lore.kernel.org/r/20240719-mptcp-pm-avail-v3-0-e96b5591ced3@kernel.org

Hi Matthieu -

I had a few typo comments, and one suggestion to split off some 
refactoring in to a net-next patch. But the functionality in the series 
looks good to me. I think we should discuss how to deal with the size of 
the series in the meeting tomorrow!

- Mat

>
> Changes in v3:
> - Small changes in patches 10 and 14, see individual changelog (Geliang)
> - New patches 18-20: small fixes
> - Link to v2: https://lore.kernel.org/r/20240715-mptcp-pm-avail-v2-0-fc5153bd1f6e@kernel.org
>
> Changes in v2:
> - Do not split id_avail_bitmap per target in patch 5 (Paolo)
> - Explicit deny (patch 2), reduce indentation (patch 3), stop earlier
>  (patch 4) (Paolo)
> - New fixes and tests (patches 8-17).
> - Link to v1: https://lore.kernel.org/r/20240621-mptcp-pm-avail-v1-0-b692d5eb89b5@kernel.org
>
> ---
> Matthieu Baerts (NGI0) (23):
>      mptcp: fully established after ADD_ADDR echo on MPJ
>      mptcp: pm: deny endp with signal + subflow + port
>      mptcp: pm: reduce indentation blocks
>      mptcp: pm: don't try to create sf if alloc failed
>      mptcp: pm: do not ignore 'subflow' if 'signal' flag is also set
>      selftests: mptcp: join: ability to invert ADD_ADDR check
>      selftests: mptcp: join: test both signal & subflow
>      mptcp: pm: re-using ID of unused removed ADD_ADDR
>      selftests: mptcp: join: check re-using ID of unused ADD_ADDR
>      mptcp: pm: re-using ID of unused removed subflows
>      selftests: mptcp: join: check re-using ID of closed subflow
>      mptcp: pm: re-using ID of unused flushed subflows
>      selftests: mptcp: join: test for flush/re-add endpoints
>      mptcp: pm: remove mptcp_pm_remove_subflow()
>      mptcp: pm: only mark 'subflow' endp as available
>      mptcp: pm: only decrement add_addr_accepted for MPJ req
>      mptcp: pm: check add_addr_accept_max before accepting new ADD_ADDR
>      mptcp: pm: only in-kernel cannot have entries with ID 0
>      mptcp: pm: fullmesh: select the right ID later
>      selftests: mptcp: join: validate fullmesh endp on 1st sf
>      mptcp: pm: avoid possible UaF whend selecting endp
>      mptcp: pm: reuse ID 0 after delete and re-add
>      mptcp: pm: reduce entries iterations on connect
>
> net/mptcp/options.c                             |   3 +-
> net/mptcp/pm.c                                  |  24 ---
> net/mptcp/pm_netlink.c                          | 210 +++++++++++++++---------
> net/mptcp/pm_userspace.c                        |  19 +--
> net/mptcp/protocol.h                            |  13 +-
> net/mptcp/subflow.c                             |  29 ++--
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 131 ++++++++++++---
> 7 files changed, 258 insertions(+), 171 deletions(-)
> ---
> base-commit: 140ff27ee47286bb0a270f3aa275fc319724da8d
> change-id: 20240620-mptcp-pm-avail-f5e3957be441
>
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
>
Re: [PATCH mptcp-net v4 00/23] mptcp: fix endpoints with 'signal' and 'subflow' flags
Posted by MPTCP CI 2 months, 3 weeks ago
Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10047466924

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/86bf23fada47
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=873047


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)