net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS?

net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS?

[Cambda Zhu]

I want to call setsockopt(TCP_MAXSEG) on a listen sock to let
all child socks have smaller MSS. And I found the child sock
MSS changed but getsockopt(TCP_MAXSEG) on the listen sock
returns 536 always.

It seems the tp->mss_cache is initialized with TCP_MSS_DEFAULT,
but getsockopt(TCP_MAXSEG) returns tp->rx_opt.user_mss only when
tp->mss_cache is 0. I don't understand the purpose of the mss_cache
check of TCP_MAXSEG. If getsockopt(TCP_MAXSEG) on listen sock makes
no sense, why does it have a branch for close/listen sock to return
user_mss? If getsockopt(TCP_MAXSEG) on listen sock is ok, why does
it check mss_cache for a listen sock?

I tried to find the commit log about TCP_MAXSEG, and found that
in commit 0c409e85f0ac ("Import 2.3.41pre2"), the mss_cache check
was added. No more detailed information found. Is this a bug or am
I misunderstanding something?

Regards,
Cambda

Re: net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS?

[Eric Dumazet]

On Wed, May 17, 2023 at 1:09 PM Cambda Zhu <cambda@linux.alibaba.com> wrote:
>
> I want to call setsockopt(TCP_MAXSEG) on a listen sock to let
> all child socks have smaller MSS. And I found the child sock
> MSS changed but getsockopt(TCP_MAXSEG) on the listen sock
> returns 536 always.
>

I think TCP_MAXSEG is not like a traditional option you can set and get later,
expecting to read back the value you set.

It is probably a bug.

Getting tp->mss_cache should have been a separate socket option, but
it is too late.


> It seems the tp->mss_cache is initialized with TCP_MSS_DEFAULT,
> but getsockopt(TCP_MAXSEG) returns tp->rx_opt.user_mss only when
> tp->mss_cache is 0. I don't understand the purpose of the mss_cache
> check of TCP_MAXSEG. If getsockopt(TCP_MAXSEG) on listen sock makes
> no sense, why does it have a branch for close/listen sock to return
> user_mss? If getsockopt(TCP_MAXSEG) on listen sock is ok, why does
> it check mss_cache for a listen sock?
>
> I tried to find the commit log about TCP_MAXSEG, and found that
> in commit 0c409e85f0ac ("Import 2.3.41pre2"), the mss_cache check
> was added. No more detailed information found. Is this a bug or am
> I misunderstanding something?

I wonder if we should simply do:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 4d6392c16b7a5a9a853c27e3a4b258d000738304..cb526257a06a6c7a4e65e710fff1770bd382ed2d
100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4080,9 +4080,10 @@ int do_tcp_getsockopt(struct sock *sk, int level,

        switch (optname) {
        case TCP_MAXSEG:
-               val = tp->mss_cache;
-               if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
+               if (((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
                        val = tp->rx_opt.user_mss;
+               else
+                       val = tp->mss_cache;
                if (tp->repair)
                        val = tp->rx_opt.mss_clamp;
                break;

Re: net: getsockopt(TCP_MAXSEG) on listen sock returns wrong MSS?

[Cambda Zhu]

> On May 17, 2023, at 23:58, Eric Dumazet <edumazet@google.com> wrote:
> 
> On Wed, May 17, 2023 at 1:09 PM Cambda Zhu <cambda@linux.alibaba.com> wrote:
>> 
>> I want to call setsockopt(TCP_MAXSEG) on a listen sock to let
>> all child socks have smaller MSS. And I found the child sock
>> MSS changed but getsockopt(TCP_MAXSEG) on the listen sock
>> returns 536 always.
>> 
> 
> I think TCP_MAXSEG is not like a traditional option you can set and get later,
> expecting to read back the value you set.
> 
> It is probably a bug.
> 
> Getting tp->mss_cache should have been a separate socket option, but
> it is too late.
> 

I understand now. It seems setting/getting TCP_MAXSEG has different meaning. :)

> 
>> It seems the tp->mss_cache is initialized with TCP_MSS_DEFAULT,
>> but getsockopt(TCP_MAXSEG) returns tp->rx_opt.user_mss only when
>> tp->mss_cache is 0. I don't understand the purpose of the mss_cache
>> check of TCP_MAXSEG. If getsockopt(TCP_MAXSEG) on listen sock makes
>> no sense, why does it have a branch for close/listen sock to return
>> user_mss? If getsockopt(TCP_MAXSEG) on listen sock is ok, why does
>> it check mss_cache for a listen sock?
>> 
>> I tried to find the commit log about TCP_MAXSEG, and found that
>> in commit 0c409e85f0ac ("Import 2.3.41pre2"), the mss_cache check
>> was added. No more detailed information found. Is this a bug or am
>> I misunderstanding something?
> 
> I wonder if we should simply do:
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 4d6392c16b7a5a9a853c27e3a4b258d000738304..cb526257a06a6c7a4e65e710fff1770bd382ed2d
> 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4080,9 +4080,10 @@ int do_tcp_getsockopt(struct sock *sk, int level,
> 
>        switch (optname) {
>        case TCP_MAXSEG:
> -               val = tp->mss_cache;
> -               if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> +               if (((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
>                        val = tp->rx_opt.user_mss;
> +               else
> +                       val = tp->mss_cache;
>                if (tp->repair)
>                        val = tp->rx_opt.mss_clamp;
>                break;

This fix is a good solution for me. Will you send a patch later?

Thanks,
Cambda

[PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state

[Cambda Zhu]

This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
TCP_MAXSEG of sock in other state will still return tp->mss_cache.

Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
Reported-by: Jack Yang <mingliang@linux.alibaba.com>
---
 net/ipv4/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3d18e295bb2f..713854f1f061 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -4083,7 +4083,7 @@ int do_tcp_getsockopt(struct sock *sk, int level,
 	switch (optname) {
 	case TCP_MAXSEG:
 		val = tp->mss_cache;
-		if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
+		if ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN))
 			val = tp->rx_opt.user_mss;
 		if (tp->repair)
 			val = tp->rx_opt.mss_clamp;
-- 
2.16.6

Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state

[Paolo Abeni]

On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
> > This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
> > CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
> > since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
> > TCP_MAXSEG of sock in other state will still return tp->mss_cache.
> > 
> > Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
> > Reported-by: Jack Yang <mingliang@linux.alibaba.com>

It's a bit strange not seeing Eric being mentioned above:

https://lore.kernel.org/netdev/CANn89i+3kL9pYtkxkwxwNMzvC_w3LNUum_2=3u+UyLBmGmifHA@mail.gmail.com/#t

Paolo

Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state

[Eric Dumazet]

On Tue, May 23, 2023 at 12:28 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
> > > This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
> > > CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
> > > since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
> > > TCP_MAXSEG of sock in other state will still return tp->mss_cache.
> > >
> > > Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
> > > Reported-by: Jack Yang <mingliang@linux.alibaba.com>
>
> It's a bit strange not seeing Eric being mentioned above:
>

...

Usual way of dealing with this would be to add

Suggested-by: Eric Dumazet <edumazet@google.com>
Link: https://lore.kernel.org/netdev/CANn89i+3kL9pYtkxkwxwNMzvC_w3LNUum_2=3u+UyLBmGmifHA@mail.gmail.com/#t


> https://lore.kernel.org/netdev/CANn89i+3kL9pYtkxkwxwNMzvC_w3LNUum_2=3u+UyLBmGmifHA@mail.gmail.com/#t
>
> Paolo
>

Patch SGTM, thanks.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state

[Paolo Abeni]

On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
> This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
> CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
> since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
> TCP_MAXSEG of sock in other state will still return tp->mss_cache.
> 
> Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
> Reported-by: Jack Yang <mingliang@linux.alibaba.com>

Could you please re-submit including the Eric's tags?

Thanks!

Paolo

Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state

[Cambda Zhu]

> On May 23, 2023, at 21:45, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
>> This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
>> CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
>> since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
>> TCP_MAXSEG of sock in other state will still return tp->mss_cache.
>> 
>> Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
>> Reported-by: Jack Yang <mingliang@linux.alibaba.com>
> 
> Could you please re-submit including the Eric's tags?
> 
> Thanks!
> 
> Paolo

Sorry for no Eric's tag. I don't know the 'Suggested-by' tag before,
and I'll re-submit the patch.

Should I add Eric's 'Reviewed-by' to the patch? Or this should be added
by maintainer? Sorry for my ignorance again. :)

Thanks,
Cambda

Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state

[Jason Xing]

On Wed, May 24, 2023 at 9:05 AM Cambda Zhu <cambda@linux.alibaba.com> wrote:
>
>
> > On May 23, 2023, at 21:45, Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On Fri, 2023-05-19 at 16:01 +0800, Cambda Zhu wrote:
> >> This patch removes the tp->mss_cache check in getting TCP_MAXSEG of
> >> CLOSE/LISTEN sock. Checking if tp->mss_cache is zero is probably a bug,
> >> since tp->mss_cache is initialized with TCP_MSS_DEFAULT. Getting
> >> TCP_MAXSEG of sock in other state will still return tp->mss_cache.
> >>
> >> Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
> >> Reported-by: Jack Yang <mingliang@linux.alibaba.com>
> >
> > Could you please re-submit including the Eric's tags?
> >
> > Thanks!
> >
> > Paolo
>
> Sorry for no Eric's tag. I don't know the 'Suggested-by' tag before,
> and I'll re-submit the patch.
>
> Should I add Eric's 'Reviewed-by' to the patch? Or this should be added
> by maintainer? Sorry for my ignorance again. :)

Yes. I think the next submission should include a Reviewed-by label as
the previous email said from Eric. Oh, don't forget to include your
previous discussion link :)

Thanks,
Jason

>
> Thanks,
> Cambda
>

Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state

[Cambda Zhu]

After I did a deep search about the TCP_MAXSEG, I found this is a bit
more complicated than I think before.

I don't know whether the TCP_MAXSEG is from BSD or not, but if the
"UNIX Network Programming" is right, getting TCP_MAXSEG returns default
MSS before connecting is as expect, that's what FreeBSD does. If we
simply remove the !val check for it, getting TCP_MAXSEG will return zero
before connecting, because tp->rx_opt.user_mss is initialized with zero
on Linux, while tp->t_maxseg is initialized with default MSS on FreeBSD.

I googled to see how it's used by developers now, and I think getting
TCP_MAXSEG should return default MSS if before connecting and user_mss
not set, for backward compatibility.

But the !val check is a bug also, and the problem is not discovered for
the first time.
https://stackoverflow.com/questions/25996741/why-getsockopt-does-not-return-the-expected-value-for-tcp-maxseg

I think it should be:

- if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
+ if (tp->rx_opt.user_mss && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))

With this change, getting TCP_MAXSEG will return default MSS as book
described, and return user_mss if user_mss is set and before connecting.
The tp->t_maxseg will only decrease on FreeBSD and we don't. I think
our solution is better.

I'll send a new patch later.


Regards,
Cambda

Re: [PATCH net-next] net: Return user_mss for TCP_MAXSEG in CLOSE/LISTEN state

[Jason Xing]

On Wed, May 24, 2023 at 3:10 PM Cambda Zhu <cambda@linux.alibaba.com> wrote:
>
> After I did a deep search about the TCP_MAXSEG, I found this is a bit
> more complicated than I think before.
>
> I don't know whether the TCP_MAXSEG is from BSD or not, but if the
> "UNIX Network Programming" is right, getting TCP_MAXSEG returns default
> MSS before connecting is as expect, that's what FreeBSD does. If we
> simply remove the !val check for it, getting TCP_MAXSEG will return zero
> before connecting, because tp->rx_opt.user_mss is initialized with zero
> on Linux, while tp->t_maxseg is initialized with default MSS on FreeBSD.
>
> I googled to see how it's used by developers now, and I think getting
> TCP_MAXSEG should return default MSS if before connecting and user_mss
> not set, for backward compatibility.
>
> But the !val check is a bug also, and the problem is not discovered for
> the first time.
> https://stackoverflow.com/questions/25996741/why-getsockopt-does-not-return-the-expected-value-for-tcp-maxseg
>
> I think it should be:
>
> - if (!val && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> + if (tp->rx_opt.user_mss && ((1 << sk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))

Good catch, It makes sense to me. With this modification, it could
prevent getsockopt returning zero with TCP_MAXSEG flag when the socket
stays in the close/listen state.

Thanks,
Jason

>
> With this change, getting TCP_MAXSEG will return default MSS as book
> described, and return user_mss if user_mss is set and before connecting.
> The tp->t_maxseg will only decrease on FreeBSD and we don't. I think
> our solution is better.
>
> I'll send a new patch later.
>
>
> Regards,
> Cambda