1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v3 0/2] Add TCP_MAXSEG socket option support
  4. View patch

[PATCH mptcp-next] mptcp: validate maxseg sockopt

Geliang Tang posted 2 patches 7 months, 2 weeks ago
[PATCH mptcp-next] mptcp: validate maxseg sockopt
Posted by Geliang Tang 7 months, 2 weeks ago 
This patch validates the newly added TCP_MAXSEG sockopt.

Signed-off-by: Geliang Tang <geliang@kernel.org>
---
 gtests/net/mptcp/sockopts/sockopt_maxseg.pkt | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 gtests/net/mptcp/sockopts/sockopt_maxseg.pkt

diff --git a/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt b/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
new file mode 100644
index 0000000..62492db
--- /dev/null
+++ b/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
@@ -0,0 +1,20 @@
+--tolerance_usecs=100000
+`../common/defaults.sh`
+
+0.0  socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
+
++0   bind(3, ..., ...) = 0
++0   listen(3, 1) = 0
+
++0   setsockopt(3, SOL_TCP, TCP_MAXSEG, [1000], 4) = 0
++0   getsockopt(3, SOL_TCP, TCP_MAXSEG, [1000], [4]) = 0
+
++0     <  S   0:0(0)         win 8000  <mss 1000, sackOK, nop, nop, nop, wscale 0, mpcapable v1 flags[flag_h] nokey>
++0     >  S.  0:0(0)  ack 1            <mss 1000, nop, nop, sackOK, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]>
++0.01  <   .  1:1(0)  ack 1  win 8000                                             <mpcapable v1 flags[flag_h] key[ckey=2, skey]>
++0   accept(3, ..., ...) = 4
+
++0   write(4, ..., 100) = 100
+
++0     >  P.  1:101(100)  ack 1             <dss dack4=1   dsn8=1 ssn=1 dll=100 nocs, nop, nop>
++0.01  <   .  1:1(0)      ack 101  win 256  <dss dack4=101 nocs>
-- 
2.43.0
Re: [PATCH mptcp-next] mptcp: validate maxseg sockopt
Posted by Matthieu Baerts 7 months, 2 weeks ago 
Hi Geliang,

On 30/04/2025 07:49, Geliang Tang wrote:
> This patch validates the newly added TCP_MAXSEG sockopt.

If you don't mind and have access, do you mind creating a PR on GitHub
instead please?

  https://github.com/multipath-tcp/packetdrill

When you send them here, the CI might think it is a kernel patch and try
to apply it (which can work with new files).

> Signed-off-by: Geliang Tang <geliang@kernel.org>
> ---
>  gtests/net/mptcp/sockopts/sockopt_maxseg.pkt | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
> 
> diff --git a/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt b/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
> new file mode 100644
> index 0000000..62492db
> --- /dev/null
> +++ b/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
> @@ -0,0 +1,20 @@
> +--tolerance_usecs=100000
> +`../common/defaults.sh`
> +
> +0.0  socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
> +
> ++0   bind(3, ..., ...) = 0
> ++0   listen(3, 1) = 0
> +

Here, similar to other sockopt tests, can you check the default value
please? It should be the same as the one for "plain" TCP, 536 I suppose
(min mss).

> ++0   setsockopt(3, SOL_TCP, TCP_MAXSEG, [1000], 4) = 0
> ++0   getsockopt(3, SOL_TCP, TCP_MAXSEG, [1000], [4]) = 0

Can you move this block before the bind()/listen()? It seems to make
more sense to set that before the listen(), no?

> ++0     <  S   0:0(0)         win 8000  <mss 1000, sackOK, nop, nop, nop, wscale 0, mpcapable v1 flags[flag_h] nokey>

Just to avoid confusions, in the inject packet here, please set
something else, e.g. 1460.

> ++0     >  S.  0:0(0)  ack 1            <mss 1000, nop, nop, sackOK, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]>
> ++0.01  <   .  1:1(0)  ack 1  win 8000                                             <mpcapable v1 flags[flag_h] key[ckey=2, skey]>
> ++0   accept(3, ..., ...) = 4

Can you do a getsockopt(4, ..., TCP_MAXSEG) here to check the value? It
should be similar to what you would get with "plain" TCP (is it 988? or
-20B reserved for MPTCP options?).

It might be better to introduce a new test doing the same thing with
"plain" TCP to compare if you don't mind.

> ++0   write(4, ..., 100) = 100

Here, please write more than the MSS you set, e.g. 1250B to see how the
stack will split that in two packets

> +
> ++0     >  P.  1:101(100)  ack 1             <dss dack4=1   dsn8=1 ssn=1 dll=100 nocs, nop, nop>
> ++0.01  <   .  1:1(0)      ack 101  win 256  <dss dack4=101 nocs>
Here, can you change TCP_MAXSEG on the accepted socket (fd = 4)?

e.g. set it to 800, then do a getsockopt(), then write(4, ..., 1250) and
see if you have 2 packets of the expected size.

Then, if you don't mind, please also create a new subflow by injecting
an MP_JOIN with a MSS of 1460, and check that the SYN+ACK+MP_JOIN
generated by the kernel has a MSS of 1000 as expected. For an example,
check mptcp/mp_join/mp_join_server.pkt.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.
Re: [PATCH mptcp-next] mptcp: validate maxseg sockopt
Posted by Geliang Tang 7 months, 1 week ago 
Hi Matt,

Thanks for the review.

On Wed, 2025-04-30 at 11:42 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 30/04/2025 07:49, Geliang Tang wrote:
> > This patch validates the newly added TCP_MAXSEG sockopt.
> 
> If you don't mind and have access, do you mind creating a PR on
> GitHub
> instead please?

Here's the PR:

https://github.com/multipath-tcp/packetdrill/pull/161

> 
>   https://github.com/multipath-tcp/packetdrill
> 
> When you send them here, the CI might think it is a kernel patch and
> try
> to apply it (which can work with new files).
> 
> > Signed-off-by: Geliang Tang <geliang@kernel.org>
> > ---
> >  gtests/net/mptcp/sockopts/sockopt_maxseg.pkt | 20
> > ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >  create mode 100644 gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
> > 
> > diff --git a/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
> > b/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
> > new file mode 100644
> > index 0000000..62492db
> > --- /dev/null
> > +++ b/gtests/net/mptcp/sockopts/sockopt_maxseg.pkt
> > @@ -0,0 +1,20 @@
> > +--tolerance_usecs=100000
> > +`../common/defaults.sh`
> > +
> > +0.0  socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
> > +
> > ++0   bind(3, ..., ...) = 0
> > ++0   listen(3, 1) = 0
> > +
> 
> Here, similar to other sockopt tests, can you check the default value
> please? It should be the same as the one for "plain" TCP, 536 I
> suppose
> (min mss).
> 
> > ++0   setsockopt(3, SOL_TCP, TCP_MAXSEG, [1000], 4) = 0
> > ++0   getsockopt(3, SOL_TCP, TCP_MAXSEG, [1000], [4]) = 0
> 
> Can you move this block before the bind()/listen()? It seems to make
> more sense to set that before the listen(), no?
> 
> > ++0     <  S   0:0(0)         win 8000  <mss 1000, sackOK, nop,
> > nop, nop, wscale 0, mpcapable v1 flags[flag_h] nokey>
> 
> Just to avoid confusions, in the inject packet here, please set
> something else, e.g. 1460.
> 
> > ++0     >  S.  0:0(0)  ack 1            <mss 1000, nop, nop,
> > sackOK, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]>
> > ++0.01  <   .  1:1(0)  ack 1  win
> > 8000                                             <mpcapable v1
> > flags[flag_h] key[ckey=2, skey]>
> > ++0   accept(3, ..., ...) = 4
> 
> Can you do a getsockopt(4, ..., TCP_MAXSEG) here to check the value?
> It
> should be similar to what you would get with "plain" TCP (is it 988?
> or
> -20B reserved for MPTCP options?).
> 
> It might be better to introduce a new test doing the same thing with
> "plain" TCP to compare if you don't mind.
> 
> > ++0   write(4, ..., 100) = 100
> 
> Here, please write more than the MSS you set, e.g. 1250B to see how
> the
> stack will split that in two packets
> 
> > +
> > ++0     >  P.  1:101(100)  ack 1             <dss dack4=1   dsn8=1
> > ssn=1 dll=100 nocs, nop, nop>
> > ++0.01  <   .  1:1(0)      ack 101  win 256  <dss dack4=101 nocs>
> Here, can you change TCP_MAXSEG on the accepted socket (fd = 4)?
> 
> e.g. set it to 800, then do a getsockopt(), then write(4, ..., 1250)
> and
> see if you have 2 packets of the expected size.
> 

In this PR, I almost addressed all your comments above ...

> Then, if you don't mind, please also create a new subflow by
> injecting
> an MP_JOIN with a MSS of 1460, and check that the SYN+ACK+MP_JOIN
> generated by the kernel has a MSS of 1000 as expected. For an
> example,
> check mptcp/mp_join/mp_join_server.pkt.

But I still have some problems when creating a new subflow, which will
be added later.

Thanks,
-Geliang

> 
> Cheers,
> Matt
Re: [PATCH mptcp-next] mptcp: validate maxseg sockopt
Posted by Matthieu Baerts 7 months, 1 week ago 
Hi Geliang,

On 08/05/2025 09:21, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for the review.
> 
> On Wed, 2025-04-30 at 11:42 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 30/04/2025 07:49, Geliang Tang wrote:
>>> This patch validates the newly added TCP_MAXSEG sockopt.
>>
>> If you don't mind and have access, do you mind creating a PR on
>> GitHub
>> instead please?
> 
> Here's the PR:
> 
> https://github.com/multipath-tcp/packetdrill/pull/161

Thanks, I just did a review:

  https://github.com/multipath-tcp/packetdrill/pull/161/files

(...)

> In this PR, I almost addressed all your comments above ...
> 
>> Then, if you don't mind, please also create a new subflow by
>> injecting
>> an MP_JOIN with a MSS of 1460, and check that the SYN+ACK+MP_JOIN
>> generated by the kernel has a MSS of 1000 as expected. For an
>> example,
>> check mptcp/mp_join/mp_join_server.pkt.
> 
> But I still have some problems when creating a new subflow, which will
> be added later.

Do not hesitate to share this WIP code and the error you have on the PR.
I also added a few pointers there.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.